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

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

[Marek Vasut]

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.

Fixes: b4e1fb8643da ("media: imx-jpeg: Support dynamic resolution change")
Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Ming Qian <ming.qian@oss.nxp.com>
Cc: Mirela Rabulea <mirela.rabulea@nxp.com>
Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-media@vger.kernel.org
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 18 ++++++++++++++++--
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index df3ccdf767baf..aef1d6473eb8d 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -1009,6 +1009,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 
 		dev_err(dev, "Encoder/decoder error, dec_ret = 0x%08x, status=0x%08x",
 			dec_ret, ret);
+		ctx->enc_state = MXC_JPEG_ENC_DONE;
 		mxc_jpeg_clr_desc(reg, slot);
 		mxc_jpeg_sw_reset(reg);
 		buf_state = VB2_BUF_STATE_ERROR;
@@ -1062,9 +1063,16 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 
 buffers_done:
 	mxc_jpeg_job_finish(ctx, buf_state, false);
-	spin_unlock(&jpeg->hw_lock);
 	cancel_delayed_work(&ctx->task_timer);
+
+	if (jpeg->mode == MXC_JPEG_ENCODE && ctx->enc_state == MXC_JPEG_ENCODING)
+		ctx->enc_state = MXC_JPEG_ENC_DONE;
+
 	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
+
+	spin_unlock(&jpeg->hw_lock);
+
+
 	return IRQ_HANDLED;
 job_unlock:
 	spin_unlock(&jpeg->hw_lock);
@@ -1488,8 +1496,12 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
 static int mxc_jpeg_job_ready(void *priv)
 {
 	struct mxc_jpeg_ctx *ctx = priv;
+	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
 
-	return ctx->source_change ? 0 : 1;
+	if (jpeg->mode == MXC_JPEG_ENCODE)
+		return ctx->enc_state == MXC_JPEG_ENC_DONE;
+	else
+		return ctx->source_change ? 0 : 1;
 }
 
 static void mxc_jpeg_device_run_timeout(struct work_struct *work)
@@ -1713,6 +1725,8 @@ static int mxc_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
 
 	if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE && V4L2_TYPE_IS_CAPTURE(q->type))
 		ctx->source_change = 0;
+	if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE)
+		ctx->enc_state = MXC_JPEG_ENC_DONE;
 	dev_dbg(ctx->mxc_jpeg->dev, "Start streaming ctx=%p", ctx);
 	q_data->sequence = 0;
 
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
index 44e46face6d1d..7f0910fc9b47e 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
@@ -35,6 +35,7 @@
 enum mxc_jpeg_enc_state {
 	MXC_JPEG_ENCODING	= 0, /* jpeg encode phase */
 	MXC_JPEG_ENC_CONF	= 1, /* jpeg encoder config phase */
+	MXC_JPEG_ENC_DONE	= 2, /* jpeg encoder done/ready phase */
 };
 
 enum mxc_jpeg_mode {
-- 
2.50.1

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

[Ming Qian(OSS)]

Hi Marek,

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.

For encoder, we always want mxc_jpeg_job_ready() to return true.

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.

So I don't think it's necessary to add a new state MXC_JPEG_ENC_DONE,
as It is almost equivalent to the completion status of the v4l2 m2m job.

Regards,
Ming

> Fixes: b4e1fb8643da ("media: imx-jpeg: Support dynamic resolution change")
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Ming Qian <ming.qian@oss.nxp.com>
> Cc: Mirela Rabulea <mirela.rabulea@nxp.com>
> Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-media@vger.kernel.org
> ---
>   drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 18 ++++++++++++++++--
>   drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h |  1 +
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index df3ccdf767baf..aef1d6473eb8d 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -1009,6 +1009,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 
>                  dev_err(dev, "Encoder/decoder error, dec_ret = 0x%08x, status=0x%08x",
>                          dec_ret, ret);
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
>                  mxc_jpeg_clr_desc(reg, slot);
>                  mxc_jpeg_sw_reset(reg);
>                  buf_state = VB2_BUF_STATE_ERROR;
> @@ -1062,9 +1063,16 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 
>   buffers_done:
>          mxc_jpeg_job_finish(ctx, buf_state, false);
> -       spin_unlock(&jpeg->hw_lock);
>          cancel_delayed_work(&ctx->task_timer);
> +
> +       if (jpeg->mode == MXC_JPEG_ENCODE && ctx->enc_state == MXC_JPEG_ENCODING)
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
> +
>          v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +
> +       spin_unlock(&jpeg->hw_lock);
> +
> +
>          return IRQ_HANDLED;
>   job_unlock:
>          spin_unlock(&jpeg->hw_lock);
> @@ -1488,8 +1496,12 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
>   static int mxc_jpeg_job_ready(void *priv)
>   {
>          struct mxc_jpeg_ctx *ctx = priv;
> +       struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> 
> -       return ctx->source_change ? 0 : 1;
> +       if (jpeg->mode == MXC_JPEG_ENCODE)
> +               return ctx->enc_state == MXC_JPEG_ENC_DONE;
> +       else
> +               return ctx->source_change ? 0 : 1;
>   }
> 
>   static void mxc_jpeg_device_run_timeout(struct work_struct *work)
> @@ -1713,6 +1725,8 @@ static int mxc_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
> 
>          if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE && V4L2_TYPE_IS_CAPTURE(q->type))
>                  ctx->source_change = 0;
> +       if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE)
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
>          dev_dbg(ctx->mxc_jpeg->dev, "Start streaming ctx=%p", ctx);
>          q_data->sequence = 0;
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 44e46face6d1d..7f0910fc9b47e 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -35,6 +35,7 @@
>   enum mxc_jpeg_enc_state {
>          MXC_JPEG_ENCODING       = 0, /* jpeg encode phase */
>          MXC_JPEG_ENC_CONF       = 1, /* jpeg encoder config phase */
> +       MXC_JPEG_ENC_DONE       = 2, /* jpeg encoder done/ready phase */
>   };
> 
>   enum mxc_jpeg_mode {
> --
> 2.50.1
> 

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

[Marek Vasut]

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.

> 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.

Re: [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@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