[PATCH] media: imx-jpeg: Fix JPEG encoder ready race condition

[Ming Qian(OSS)]

Hi Marek,

On 8/21/2025 4:59 PM, Marek Vasut wrote:
> On 8/21/25 9:06 AM, Ming Qian(OSS) wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 8/21/2025 12:29 AM, Marek Vasut wrote:
>>> [You don't often get email from marek.vasut at mailbox.org. Learn why 
>>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> The current mxc_jpeg_job_ready() implementation works for JPEG decode
>>> side of this IP, it does not work at all for the JPEG encode side. The
>>> JPEG encode side does not change ctx->source_change at all, therefore
>>> the mxc_jpeg_source_change() always returns right away and the encode
>>> side somehow works.
>>>
>>> However, this is susceptible to a race condition between 
>>> mxc_jpeg_dec_irq()
>>> and mxc_jpeg_start_streaming(), where mxc_jpeg_start_streaming() might
>>> start decoding another frame before mxc_jpeg_dec_irq() indicates 
>>> completion
>>> of encoding of current frame. Add new state, MXC_JPEG_ENC_DONE, which is
>>> set in three locations, first when streaming starts to indicate the 
>>> encoder
>>> is ready to start processing a frame, second in mxc_jpeg_dec_irq() 
>>> when the
>>> encoder finishes encoding current frame, and third in 
>>> mxc_jpeg_dec_irq() in
>>> case of an error so the encoder can proceed with encoding another frame.
>>>
>>> Update mxc_jpeg_job_ready() to check whether the encoder is in this new
>>> MXC_JPEG_ENC_DONE state before reporting the encoder is ready to encode
>>> new frame.
>>>
>>
>> The jpeg encoder and jpeg decoder are 2 different devices, so they won't
>> compete with each other.
> 
> I know.
> 
>> For encoder, we always want mxc_jpeg_job_ready() to return true.
> 
> For encoder, mxc_jpeg_job_ready() currently returns !ctx- 
>  >source_change , which is only set by decoder side of the driver. This 
> seems wrong.
> 

That is what we want, the ctx->source_change of encoder is never set, so
mxc_jpeg_job_ready() will always return true.
If you think this is not clear enough, maybe the following code is
enough:

	if (jpeg->mode == MXC_JPEG_ENCODE)
		return 1;


>> And v4l2_m2m has ensured that there will be no race condition between
>> mxc_jpeg_dec_irq() and mxc_jpeg_start_streaming().
>>
>> After v4l2_m2m_job_finish() is called in the mxc_jpeg_dec_irq(),
>> then it is possible to start encoding the next frame.
> This part would be true if the IRQ function couldn't be called by 
> anything except end of configuration or end of encoding, but it can be 
> triggered by other things, like AXI READ error IRQ.

The mxc_jpeg_dec_irq() has filtered out the irrelevant interrupts:

	ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
	if (WARN_ON(!ctx))
		goto job_unlock;

	if (slot != ctx->slot) {
		/* TODO investigate when adding multi-instance support */
		dev_warn(dev, "IRQ slot %d != context slot %d.\n",
			 slot, ctx->slot);
		goto job_unlock;
	}

	if (!jpeg->slot_data.used)
		goto job_unlock;


In other cases, it means the v4l2 m2m job has been started, we just need
to call v4l2_m2m_job_finish()

If CONFIG ERROR occurs, driver still call
v4l2_m2m_job_finish().

So I don't think this patch changes anything.

Regards,
Ming