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

[Marek Vasut <marek.vasut@mailbox.org>]

On 8/21/25 11:24 AM, Ming Qian(OSS) wrote:

Hello Ming,

sorry for my late reply.

> 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@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.
I can't seem to be able to reproduce the issue on MX95 A0 with the SRAM 
patch in place, I will keep an eye on this and possibly revisit this if 
it shows up again.

Thank you for your help.