Re: [PATCH v5 2/4] media: rockchip: Add JPEG decoder driver

["Sascha Hauer" <s.hauer@pengutronix.de>]

On 2026-09-29 15:34, Nicolas Dufresne wrote:
> > > > +static int vdpu720_fill_chroma(struct rkjpegd_ctx *ctx,
> > > > +			       struct vb2_v4l2_buffer *dst_buf)
> > > > +{
> > > > +	struct rkjpegd_dev *jpegd = ctx->dev;
> > > > +	struct vb2_buffer *vb = &dst_buf->vb2_buf;
> > > > +	u32 y_size, size;
> > > > +	void *dst_cpu;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&ctx->fmt_lock);
> > > 
> > > I keep seeing this fmt lock over and over and can't get my head around why you
> > > would need that. There is implicit locking in the VIDIOC system that do protect
> > > these. Again, can you in your own word explain your choices ?
> > 
> > It comes with dynamic resolution support. The check for a new resolution
> > has to be done in device_run() to make sure the resolution change takes
> > place on that exact frame. Doing it in buf_queue() would mean we get the
> > frames still in the queue wrong. We can't take vdev_lock in
> > device_run() which is why sashiko continuously stumbled upon missing
> > locking of ctx->dst_fmt.
> > 
> > I cannot judge how propable an actual race or how serious this missing
> > locking is. But yes, the fmt_lock is a direct result of my LLM fighting
> > against Sashiko.
> > 
> > So I could drop dynamic resolution support (which I don't need
> > currently), or we could ignore Sashiko here.
> 
> That's the tougher one to solve I suppose. This is probably the first jpeg
> decoder implementing DYN_RESOLUTION, which is valid, just a feature more recent
> then most jpeg decoders today.
> 
> I was mostly triggered by the fact  called fmt lock, since device_run() only
> occurs while the device is streaming, and the format cannot change while
> streaming.
> 
> On dynamic resolution change, to avoid the need for this locking, I believe you
> want to delay the update of the dst_fmt to when the draining have completed and
> userspace stop streaming. Only then it is safe, as in order to resume decoding,
> userspace will have to call streamon, which will fail if the allocated buffers
> are too small. There is no other validation of the buffer size in place, so
> otherwise you can just do CMD_START and let the HW write all over the place.

Ok, with the next version the fmt_lock will be gone. I'll update dst_fmt
only under vdev_lock. On a format change the decoder stops at that
frame. In g_fmt I can now report either the dst_fmt, or, when the
decoder has stopped, the format of the frame at the head of the queue.

> > > > +	irqreturn_t ret = IRQ_NONE;
> > > > +	u32 status;
> > > > +
> > > > +	/* The registers are only clocked while the device is runtime active. */
> > > > +	if (pm_runtime_get_if_active(jpegd->dev) <= 0)
> > > > +		return IRQ_NONE;
> > > 
> > > Was that sashiko asking you to do that ?
> > 
> > Yes, several times:
> > 
> >   ▎ [High] The interrupt handler reads hardware registers without checking if the device is active via 
> >   ▎ pm_runtime, leading to crashes on spurious interrupts.
> > 
> > 
> >   ▎ [Severity: High]
> >   ▎ This reads the VDPU720_REG_INT register unconditionally upon entry. If a spurious interrupt or an 
> >   ▎ irqpoll event occurs while the device is in a runtime-suspended state (with clocks and power domains 
> >   ▎ gated off), will this read trigger a synchronous external abort on ARM? Should it use 
> >   ▎ pm_runtime_get_if_active() to verify the power state first?
> > 
> >   ▎ If a spurious interrupt fires while the IP block is held in reset, the rkjpegd_vdpu720_irq() handler 
> >   ▎ will run. Because PM runtime is still active, pm_runtime_get_if_active() will succeed, and the handler
> >   ▎ will try to read from the hardware (VDPU720_REG_INT), which can cause a bus stall or kernel panic.
> > 
> > > To start with, if you clock off the
> > > device, you won't get the IRQ. If you clock off the device between the start of
> > > this function and here in a race, you have some bigger problems in your driver.
> > > 
> > > This type of IP is not free-running, its trigger based. When its triggered, it
> > > should be busy and a PM ref should be held. Be careful with sashiko remarks, it
> > > does not differentiate free-running IP from triggered IP.
> > > 
> > > I'm also a little worried that maybe you don't actually understand this, since
> > > its quite possible you simply fed sashiko into your llm to produce this v5.
> > 
> > Ok, shows I have to think a bit more before taking Sashiko things for
> > granted.
> 
> Yes, though I was off a little, what I mean is that for triggered devices like
> this, which don't have known spurious IRQ hw bugs, you want the PM runtime to be
> held for the duration of job, which imply it will be running on IRQ. The method
> you have now would be useful if there was a shared IRQ or some known hardware
> bugs to deal with.

No shared IRQ and no known hardware bugs currently, so I can drop the
runtime PM from the IRQ handler.

> > > > +			rkjpegd_last_buffer_done(ctx, vbuf);
> > > > +			v4l2_m2m_mark_stopped(ctx->fh.m2m_ctx);
> > > > +			v4l2_event_queue_fh(&ctx->fh, &rkjpegd_eos_event);
> > > > +			return;
> > > > +		}
> > > > +
> > > > +		v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	rkjpegd_parse_src_buf(ctx, vb);
> > > 
> > > This function can fail, its not clear once you ignore the return value how the
> > > error will propagade to device_run() and produce a matching error dst buffer.
> > 
> > The error travels through src_buf->parsed and rkjpegd_vdpu720_run()
> > bails out with an error when the frame couldn't be parsed. I can make
> > that clearer by returning an error from rkjpegd_parse_src_buf() and
> > setting the variable here instead.
> 
> Not sure I completely followed, but the main point is either drop
> rkjpegd_parse_src_buf() return value and related code if not needed, or use the
> return value to propagate the error.

rkjpegd_parse_src_buf() returns void in this version. Anyway, will
change to return an error and use that value.

> > > > +static void rkjpegd_stop_streaming(struct vb2_queue *vq)
> > > > +{
> > > > +	struct rkjpegd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > +	struct vb2_v4l2_buffer *vbuf;
> > > > +
> > > > +	for (;;) {
> > > > +		if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > > > +			vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > > > +		else
> > > > +			vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > > > +		if (!vbuf)
> > > > +			break;
> > > > +		if (V4L2_TYPE_IS_CAPTURE(vq->type))
> > > > +			vb2_set_plane_payload(&vbuf->vb2_buf, 0, 0);
> > > > +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> > > > +	}
> > > > +
> > > > +	v4l2_m2m_update_stop_streaming_state(ctx->fh.m2m_ctx, vq);
> > > > +
> > > > +	/* A seek also ends a drain a resolution change had cut short. */
> > > > +	if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > > > +		ctx->fh.m2m_ctx->last_src_buf = NULL;
> > > > +	else
> > > > +		rkjpegd_resume_drain(ctx);
> > > > +
> > > > +	if (V4L2_TYPE_IS_OUTPUT(vq->type) &&
> > > > +	    v4l2_m2m_has_stopped(ctx->fh.m2m_ctx))
> > > > +		v4l2_event_queue_fh(&ctx->fh, &rkjpegd_eos_event);
> > > 
> > > That is strange, STREAMOFF will flush the queues, so adding an event to the
> > > event queue seems odd, I never seen that before.
> > 
> > The same pattern is in the vicodec since [1], was added to mxc-jpeg in
> > [2] and went into this driver from there.
> > 
> > Sending EOS here is deprecated anyway:
> > 
> >       For backwards compatibility, the decoder will signal a ``V4L2_EVENT_EOS``
> >       event when the last frame has been decoded and all frames are ready to be
> >       dequeued. It is a deprecated behavior and the client must not rely on it.
> >       The ``V4L2_BUF_FLAG_LAST`` buffer flag should be used instead.
> > 
> > Maybe we can just drop it for a new driver.
> 
> But that's different, its about sending EOS event when the LAST buffer is queued
> into the capture pool. Driver that strictly want to implement this backward
> compatibility must resort to queuing an empty LAST buffer. Doing this in
> streamoff seems odd, I would need to further dig into. I must admit, I never
> seen that event being used, so I don't really know how it was historically used.
> 
> You can always leave it, I'm just not convince of the usefulness, as we should
> be flushing the queues when we stop streaming.

I'll drop it.

> > > > +module_platform_driver(rkjpegd_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("Rockchip JPEG decoder driver");
> > > > +MODULE_AUTHOR("Lucas Sinn <lucas.sinn@wolfvision.net>");
> > > > +MODULE_LICENSE("GPL");
> > > > +MODULE_IMPORT_NS("DMA_BUF");
> > > 
> > > 
> > > Looking forward your feedback, I'm quite surprised of "your" choices, and how
> > > you possibly have needed this complexity.
> > 
> > Thanks for the thorough and honest review.
> > 
> > When I first worked with v4l2_m2m many years ago I was excited how easy
> > it has become to write such an m2m driver. I am also shocked to see how
> > many possible races sashiko found and through which hoops I had to go to
> > make sashiko happy (I still haven't accomplished that it seems).
> > 
> > Much of the complexity comes from two things: The watchdog and the
> > dynamic resolution handling.
> > 
> > Sashiko claims the watchdog has to protect itself against a race with
> > the irq handler. That of course misses that the watchdog only triggers
> > because the IRQ doesn't come which makes it quite unlikely that the IRQ
> > comes in the precise moment the watchdog triggers.
> 
> That should be solved using the watchdog API alone. On a theoretical basis
> (cause I don't program this every day):
>
> irq_func()
> {
>   if (!cancel_delayed_work(&dev->watchdog_work))
> 		return IRQ_HANDLED;
>   ...
> }
> 
> watchdog_func()
> {
>   reset_so_that_no_possible_irq_occures_passed_this_line();
>   return;
> }
> 
> So in the IRQ handler, you use the delayed work API to guaranty that your
> whatchdog won't be called. If its being called, or has been called, then it does
> an early return.
> 
> For the watchdog, the delayed worker code ensures that once called,
> cancel_delayed_work() will return false. You have to make sure you synchronously
> guaranty that no more IRQ will occur, hense why we operate a HW reset. Its also
> usually the only way to recover a wedged IP.

Done that.

Sascha

--
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |