[PATCH v2 0/3] media: imx-jpeg: Fix some motion-jpeg decoding issues

[PATCH v2 0/3] media: imx-jpeg: Fix some motion-jpeg decoding issues

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

To support decoding motion-jpeg without DHT, driver will try to decode a
pattern jpeg before actual jpeg frame by use of linked descriptors
(This is called "repeat mode"), then the DHT in the pattern jpeg can be
used for decoding the motion-jpeg.

But there is some hardware limitation in the repeat mode, that may cause
corruption or decoding timeout.

Try to make workaround for these limitation in this patchset.

Ming Qian (3):
  media: imx-jpeg: Enhance error handling in buffer allocation
  media: imx-jpeg: Change the pattern size to 128x64
  media: imx-jpeg: Check decoding is ongoing for motion-jpeg

 .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |   1 +
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 109 +++++++++++++-----
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |   5 +
 3 files changed, 86 insertions(+), 29 deletions(-)

-- 
2.43.0-rc1

[PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
buffer, but only return error if certain allocation failed.

Without cleanup the allocation failure, the next time it will return
success directly, but let some buffer be uninitialized.
It may result in accessing a null pointer.

Clean up if error occurs in the allocation.

Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
v2
- Add the Fixes tag

 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 0e6ee997284b..12661c177f5a 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
 	return -1;
 }
 
+static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
+{
+	/* free descriptor for decoding/encoding phase */
+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
+			  jpeg->slot_data.desc,
+			  jpeg->slot_data.desc_handle);
+	jpeg->slot_data.desc = NULL;
+	jpeg->slot_data.desc_handle = 0;
+
+	/* free descriptor for encoder configuration phase / decoder DHT */
+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
+			  jpeg->slot_data.cfg_desc,
+			  jpeg->slot_data.cfg_desc_handle);
+	jpeg->slot_data.cfg_desc_handle = 0;
+	jpeg->slot_data.cfg_desc = NULL;
+
+	/* free configuration stream */
+	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
+			  jpeg->slot_data.cfg_stream_vaddr,
+			  jpeg->slot_data.cfg_stream_handle);
+	jpeg->slot_data.cfg_stream_vaddr = NULL;
+	jpeg->slot_data.cfg_stream_handle = 0;
+
+	jpeg->slot_data.used = false;
+}
+
 static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
 {
 	struct mxc_jpeg_desc *desc;
@@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
 	return true;
 err:
 	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
+	mxc_jpeg_free_slot_data(jpeg);
 
 	return false;
 }
 
-static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
-{
-	/* free descriptor for decoding/encoding phase */
-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
-			  jpeg->slot_data.desc,
-			  jpeg->slot_data.desc_handle);
-
-	/* free descriptor for encoder configuration phase / decoder DHT */
-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
-			  jpeg->slot_data.cfg_desc,
-			  jpeg->slot_data.cfg_desc_handle);
-
-	/* free configuration stream */
-	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
-			  jpeg->slot_data.cfg_stream_vaddr,
-			  jpeg->slot_data.cfg_stream_handle);
-
-	jpeg->slot_data.used = false;
-}
-
 static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
 					       struct vb2_v4l2_buffer *src_buf,
 					       struct vb2_v4l2_buffer *dst_buf)
-- 
2.43.0-rc1

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

[Frank Li]

On Fri, Mar 28, 2025 at 02:30:50PM +0800, ming.qian@oss.nxp.com wrote:
> From: Ming Qian <ming.qian@oss.nxp.com>
>
> In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
> buffer, but only return error if certain allocation failed.
>
> Without cleanup the allocation failure, the next time it will return
> success directly, but let some buffer be uninitialized.
> It may result in accessing a null pointer.
>
> Clean up if error occurs in the allocation.
>
> Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
> v2
> - Add the Fixes tag
>
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
>  1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 0e6ee997284b..12661c177f5a 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
>  	return -1;
>  }
>
> +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> +{
> +	/* free descriptor for decoding/encoding phase */
> +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> +			  jpeg->slot_data.desc,
> +			  jpeg->slot_data.desc_handle);
> +	jpeg->slot_data.desc = NULL;
> +	jpeg->slot_data.desc_handle = 0;
> +
> +	/* free descriptor for encoder configuration phase / decoder DHT */
> +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> +			  jpeg->slot_data.cfg_desc,
> +			  jpeg->slot_data.cfg_desc_handle);
> +	jpeg->slot_data.cfg_desc_handle = 0;
> +	jpeg->slot_data.cfg_desc = NULL;
> +
> +	/* free configuration stream */
> +	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> +			  jpeg->slot_data.cfg_stream_vaddr,
> +			  jpeg->slot_data.cfg_stream_handle);
> +	jpeg->slot_data.cfg_stream_vaddr = NULL;
> +	jpeg->slot_data.cfg_stream_handle = 0;
> +
> +	jpeg->slot_data.used = false;
> +}
> +
>  static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>  {
>  	struct mxc_jpeg_desc *desc;
> @@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>  	return true;
>  err:
>  	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
> +	mxc_jpeg_free_slot_data(jpeg);
>
>  	return false;
>  }
>
> -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> -{
> -	/* free descriptor for decoding/encoding phase */
> -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -			  jpeg->slot_data.desc,
> -			  jpeg->slot_data.desc_handle);
> -
> -	/* free descriptor for encoder configuration phase / decoder DHT */
> -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -			  jpeg->slot_data.cfg_desc,
> -			  jpeg->slot_data.cfg_desc_handle);
> -
> -	/* free configuration stream */
> -	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> -			  jpeg->slot_data.cfg_stream_vaddr,
> -			  jpeg->slot_data.cfg_stream_handle);
> -
> -	jpeg->slot_data.used = false;
> -}
> -
>  static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
>  					       struct vb2_v4l2_buffer *src_buf,
>  					       struct vb2_v4l2_buffer *dst_buf)
> --
> 2.43.0-rc1
>

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

[Sebastian Fricke]

Hey Ming,

In the title I'd suggest:
media: imx-jpeg: Cleanup after an allocation error

To be a bit more concrete, enhance error handling can mean pretty much
anything.

On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>From: Ming Qian <ming.qian@oss.nxp.com>
>
>In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
>buffer, but only return error if certain allocation failed.
>
>Without cleanup the allocation failure, the next time it will return
>success directly, but let some buffer be uninitialized.
>It may result in accessing a null pointer.
>
>Clean up if error occurs in the allocation.

I'd suggest:

When allocation failures are not cleaned up by the driver, further allocation
errors will be false-positives, which will cause buffers to remain
uninitialized and cause NULL pointer dereferences.
Clean up the errors accordingly.

>
>Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
>Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>---
>v2
>- Add the Fixes tag
>
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
> 1 file changed, 27 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>index 0e6ee997284b..12661c177f5a 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>@@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
> 	return -1;
> }
>
>+static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>+{
>+	/* free descriptor for decoding/encoding phase */
>+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>+			  jpeg->slot_data.desc,
>+			  jpeg->slot_data.desc_handle);
>+	jpeg->slot_data.desc = NULL;
>+	jpeg->slot_data.desc_handle = 0;
>+
>+	/* free descriptor for encoder configuration phase / decoder DHT */
>+	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>+			  jpeg->slot_data.cfg_desc,
>+			  jpeg->slot_data.cfg_desc_handle);
>+	jpeg->slot_data.cfg_desc_handle = 0;
>+	jpeg->slot_data.cfg_desc = NULL;
>+
>+	/* free configuration stream */
>+	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>+			  jpeg->slot_data.cfg_stream_vaddr,
>+			  jpeg->slot_data.cfg_stream_handle);
>+	jpeg->slot_data.cfg_stream_vaddr = NULL;
>+	jpeg->slot_data.cfg_stream_handle = 0;
>+
>+	jpeg->slot_data.used = false;
>+}
>+
> static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> {
> 	struct mxc_jpeg_desc *desc;
>@@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> 	return true;
> err:
> 	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
>+	mxc_jpeg_free_slot_data(jpeg);
>
> 	return false;
> }
>
>-static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>-{
>-	/* free descriptor for decoding/encoding phase */
>-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>-			  jpeg->slot_data.desc,
>-			  jpeg->slot_data.desc_handle);
>-
>-	/* free descriptor for encoder configuration phase / decoder DHT */
>-	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>-			  jpeg->slot_data.cfg_desc,
>-			  jpeg->slot_data.cfg_desc_handle);
>-
>-	/* free configuration stream */
>-	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>-			  jpeg->slot_data.cfg_stream_vaddr,
>-			  jpeg->slot_data.cfg_stream_handle);
>-
>-	jpeg->slot_data.used = false;
>-}

Can you split the moving of the code from the changes you do?
Otherwise the reviewer is forced to get the diff manually.

Regards,
Sebastian Fricke

Re: [PATCH v2 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

[Ming Qian(OSS)]

Hi Sebastian,


On 2025/4/5 19:39, Sebastian Fricke wrote:
> Hey Ming,
> 
> In the title I'd suggest:
> media: imx-jpeg: Cleanup after an allocation error
> 
> To be a bit more concrete, enhance error handling can mean pretty much
> anything.
> 

Thanks, I'll apply your suggestion

> On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
>> buffer, but only return error if certain allocation failed.
>>
>> Without cleanup the allocation failure, the next time it will return
>> success directly, but let some buffer be uninitialized.
>> It may result in accessing a null pointer.
>>
>> Clean up if error occurs in the allocation.
> 
> I'd suggest:
> 
> When allocation failures are not cleaned up by the driver, further 
> allocation
> errors will be false-positives, which will cause buffers to remain
> uninitialized and cause NULL pointer dereferences.
> Clean up the errors accordingly.
> 

Thanks, I'll apply your suggestion

>>
>> Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG 
>> Encoder/Decoder")
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> v2
>> - Add the Fixes tag
>>
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
>> 1 file changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 0e6ee997284b..12661c177f5a 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct 
>> mxc_jpeg_slot_data *slot_data)
>>     return -1;
>> }
>>
>> +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>> +{
>> +    /* free descriptor for decoding/encoding phase */
>> +    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> +              jpeg->slot_data.desc,
>> +              jpeg->slot_data.desc_handle);
>> +    jpeg->slot_data.desc = NULL;
>> +    jpeg->slot_data.desc_handle = 0;
>> +
>> +    /* free descriptor for encoder configuration phase / decoder DHT */
>> +    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> +              jpeg->slot_data.cfg_desc,
>> +              jpeg->slot_data.cfg_desc_handle);
>> +    jpeg->slot_data.cfg_desc_handle = 0;
>> +    jpeg->slot_data.cfg_desc = NULL;
>> +
>> +    /* free configuration stream */
>> +    dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>> +              jpeg->slot_data.cfg_stream_vaddr,
>> +              jpeg->slot_data.cfg_stream_handle);
>> +    jpeg->slot_data.cfg_stream_vaddr = NULL;
>> +    jpeg->slot_data.cfg_stream_handle = 0;
>> +
>> +    jpeg->slot_data.used = false;
>> +}
>> +
>> static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>> {
>>     struct mxc_jpeg_desc *desc;
>> @@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct 
>> mxc_jpeg_dev *jpeg)
>>     return true;
>> err:
>>     dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", 
>> jpeg->slot_data.slot);
>> +    mxc_jpeg_free_slot_data(jpeg);
>>
>>     return false;
>> }
>>
>> -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>> -{
>> -    /* free descriptor for decoding/encoding phase */
>> -    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> -              jpeg->slot_data.desc,
>> -              jpeg->slot_data.desc_handle);
>> -
>> -    /* free descriptor for encoder configuration phase / decoder DHT */
>> -    dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> -              jpeg->slot_data.cfg_desc,
>> -              jpeg->slot_data.cfg_desc_handle);
>> -
>> -    /* free configuration stream */
>> -    dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>> -              jpeg->slot_data.cfg_stream_vaddr,
>> -              jpeg->slot_data.cfg_stream_handle);
>> -
>> -    jpeg->slot_data.used = false;
>> -}
> 
> Can you split the moving of the code from the changes you do?
> Otherwise the reviewer is forced to get the diff manually.

Sure, this will be done in v3

> 
> Regards,
> Sebastian Fricke

[PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

To support decoding motion-jpeg without DHT, driver will try to decode a
pattern jpeg before actual jpeg frame by use of linked descriptors
(This is called "repeat mode"), then the DHT in the pattern jpeg can be
used for decoding the motion-jpeg.

To avoid performance loss, use the smallest supported resolution 64x64
as the pattern jpeg size.

But there is a hardware issue: when the JPEG decoded frame with a
resolution that is no larger than 64x64 and it is followed by a next
decoded frame with a larger resolution but not 64 aligned, then this
next decoded frame may be corrupted.

To avoid corruption of the decoded image, we change the pattern jpeg
size to 128x64, as we confirmed with the hardware designer that this is
a safe size.

Besides, we also need to allocate a dma buffer to store the decoded
picture for the pattern image.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 42 +++++++++++++++----
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  5 +++
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 12661c177f5a..45705c606769 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = {
 };
 
 static const unsigned char jpeg_image_red[] = {
-	0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE,
+	0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28,
+	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
+	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
+	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
+	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
+	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
+	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
+	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
+	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
+	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
+	0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9,
+	0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0,
 	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
 	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
 	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
@@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = {
 	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
 	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
 	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
-	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00
+	0x00, 0x28, 0xA0, 0x0F
 };
 
 static const unsigned char jpeg_eoi[] = {
@@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
 	jpeg->slot_data.cfg_stream_vaddr = NULL;
 	jpeg->slot_data.cfg_stream_handle = 0;
 
+	dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size,
+			  jpeg->slot_data.cfg_dec_vaddr,
+			  jpeg->slot_data.cfg_dec_daddr);
+	jpeg->slot_data.cfg_dec_size = 0;
+	jpeg->slot_data.cfg_dec_vaddr = NULL;
+	jpeg->slot_data.cfg_dec_daddr = 0;
+
 	jpeg->slot_data.used = false;
 }
 
@@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
 		goto err;
 	jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
 
+	jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2;
+	jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev,
+							   jpeg->slot_data.cfg_dec_size,
+							   &jpeg->slot_data.cfg_dec_daddr,
+							   GFP_ATOMIC);
+	if (!jpeg->slot_data.cfg_dec_vaddr)
+		goto err;
+
 skip_alloc:
 	jpeg->slot_data.used = true;
 
@@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf,
 	 */
 	*cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
 					      V4L2_PIX_FMT_YUYV,
-					      MXC_JPEG_MIN_WIDTH,
-					      MXC_JPEG_MIN_HEIGHT);
+					      MXC_JPEG_PATTERN_WIDTH,
+					      MXC_JPEG_PATTERN_HEIGHT);
 	cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
-	cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
+	cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr;
 	cfg_desc->buf_base1 = 0;
-	cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16;
-	cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT;
-	cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2;
+	cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16;
+	cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT;
+	cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2;
 	cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422);
 	cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
 	cfg_desc->stm_bufbase = cfg_stream_handle;
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
index 86e324b21aed..fdde45f7e163 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
@@ -28,6 +28,8 @@
 #define MXC_JPEG_W_ALIGN		3
 #define MXC_JPEG_MAX_SIZEIMAGE		0xFFFFFC00
 #define MXC_JPEG_MAX_PLANES		2
+#define MXC_JPEG_PATTERN_WIDTH		128
+#define MXC_JPEG_PATTERN_HEIGHT		64
 
 enum mxc_jpeg_enc_state {
 	MXC_JPEG_ENCODING	= 0, /* jpeg encode phase */
@@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data {
 	dma_addr_t desc_handle;
 	dma_addr_t cfg_desc_handle; // configuration descriptor dma address
 	dma_addr_t cfg_stream_handle; // configuration bitstream dma address
+	dma_addr_t cfg_dec_size;
+	void *cfg_dec_vaddr;
+	dma_addr_t cfg_dec_daddr;
 };
 
 struct mxc_jpeg_dev {
-- 
2.43.0-rc1

Re: [PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64

[Frank Li]

On Fri, Mar 28, 2025 at 02:30:51PM +0800, ming.qian@oss.nxp.com wrote:
> From: Ming Qian <ming.qian@oss.nxp.com>
>
> To support decoding motion-jpeg without DHT, driver will try to decode a
> pattern jpeg before actual jpeg frame by use of linked descriptors
> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
> used for decoding the motion-jpeg.
>
> To avoid performance loss, use the smallest supported resolution 64x64
> as the pattern jpeg size.
>
> But there is a hardware issue: when the JPEG decoded frame with a
> resolution that is no larger than 64x64 and it is followed by a next
> decoded frame with a larger resolution but not 64 aligned, then this
> next decoded frame may be corrupted.
>
> To avoid corruption of the decoded image, we change the pattern jpeg
> size to 128x64, as we confirmed with the hardware designer that this is
> a safe size.
>
> Besides, we also need to allocate a dma buffer to store the decoded
> picture for the pattern image.
>
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 42 +++++++++++++++----
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  5 +++
>  2 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 12661c177f5a..45705c606769 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = {
>  };
>
>  static const unsigned char jpeg_image_red[] = {
> -	0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE,
> +	0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28,
> +	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
> +	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
> +	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
> +	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
> +	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
> +	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
> +	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
> +	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
> +	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
> +	0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9,
> +	0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0,
>  	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>  	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>  	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
> @@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = {
>  	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>  	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>  	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
> -	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00
> +	0x00, 0x28, 0xA0, 0x0F
>  };
>
>  static const unsigned char jpeg_eoi[] = {
> @@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>  	jpeg->slot_data.cfg_stream_vaddr = NULL;
>  	jpeg->slot_data.cfg_stream_handle = 0;
>
> +	dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size,
> +			  jpeg->slot_data.cfg_dec_vaddr,
> +			  jpeg->slot_data.cfg_dec_daddr);
> +	jpeg->slot_data.cfg_dec_size = 0;
> +	jpeg->slot_data.cfg_dec_vaddr = NULL;
> +	jpeg->slot_data.cfg_dec_daddr = 0;
> +
>  	jpeg->slot_data.used = false;
>  }
>
> @@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>  		goto err;
>  	jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
>
> +	jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2;
> +	jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev,
> +							   jpeg->slot_data.cfg_dec_size,
> +							   &jpeg->slot_data.cfg_dec_daddr,
> +							   GFP_ATOMIC);
> +	if (!jpeg->slot_data.cfg_dec_vaddr)
> +		goto err;
> +
>  skip_alloc:
>  	jpeg->slot_data.used = true;
>
> @@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf,
>  	 */
>  	*cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>  					      V4L2_PIX_FMT_YUYV,
> -					      MXC_JPEG_MIN_WIDTH,
> -					      MXC_JPEG_MIN_HEIGHT);
> +					      MXC_JPEG_PATTERN_WIDTH,
> +					      MXC_JPEG_PATTERN_HEIGHT);
>  	cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
> -	cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
> +	cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr;
>  	cfg_desc->buf_base1 = 0;
> -	cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16;
> -	cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT;
> -	cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2;
> +	cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16;
> +	cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT;
> +	cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2;
>  	cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422);
>  	cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
>  	cfg_desc->stm_bufbase = cfg_stream_handle;
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 86e324b21aed..fdde45f7e163 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -28,6 +28,8 @@
>  #define MXC_JPEG_W_ALIGN		3
>  #define MXC_JPEG_MAX_SIZEIMAGE		0xFFFFFC00
>  #define MXC_JPEG_MAX_PLANES		2
> +#define MXC_JPEG_PATTERN_WIDTH		128
> +#define MXC_JPEG_PATTERN_HEIGHT		64
>
>  enum mxc_jpeg_enc_state {
>  	MXC_JPEG_ENCODING	= 0, /* jpeg encode phase */
> @@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data {
>  	dma_addr_t desc_handle;
>  	dma_addr_t cfg_desc_handle; // configuration descriptor dma address
>  	dma_addr_t cfg_stream_handle; // configuration bitstream dma address
> +	dma_addr_t cfg_dec_size;
> +	void *cfg_dec_vaddr;
> +	dma_addr_t cfg_dec_daddr;
>  };
>
>  struct mxc_jpeg_dev {
> --
> 2.43.0-rc1
>

Re: [PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64

[Sebastian Fricke]

Hey Ming,

On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>From: Ming Qian <ming.qian@oss.nxp.com>
>
>To support decoding motion-jpeg without DHT, driver will try to decode a
>pattern jpeg before actual jpeg frame by use of linked descriptors
>(This is called "repeat mode"), then the DHT in the pattern jpeg can be
>used for decoding the motion-jpeg.
>
>To avoid performance loss, use the smallest supported resolution 64x64
>as the pattern jpeg size.
>
>But there is a hardware issue: when the JPEG decoded frame with a
>resolution that is no larger than 64x64 and it is followed by a next
>decoded frame with a larger resolution but not 64 aligned, then this
>next decoded frame may be corrupted.
>
>To avoid corruption of the decoded image, we change the pattern jpeg
>size to 128x64, as we confirmed with the hardware designer that this is
>a safe size.
>
>Besides, we also need to allocate a dma buffer to store the decoded
>picture for the pattern image.

Why is that related to the change of the pattern size? Like why wasn't
that needed for 64x64? And if this solves a different issue, can you put
that into an extra patch?

This is a bit hard to understand, maybe this is better:

In order to decode a motion-jpeg bitstream, which doesn't provide a DHT,
the driver will first decode a pattern jpeg and use the DHT found in the
pattern to decode the first actual frame. This mode is called
"repeat-mode" and it utilizes linked descriptors.
The smallest supported resolution of 64x64 was used for that pattern to
not cause unneeded performance delay. This choice, however, can cause a
corrupted decoded picture of the first frame after the pattern, when the
resolution of that frame is larger than the pattern and is not aligned
to 64.
By altering the pattern size to 128x64, this corruption can be avoided.
That size has been confirmed to be safe by the hardware designers.
Additionally, a DMA buffer needs to be allocated to store the decoded
picture of the pattern image.

The rest looks good.

Regards,
Sebastian

>
>Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>---
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 42 +++++++++++++++----
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  5 +++
> 2 files changed, 39 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>index 12661c177f5a..45705c606769 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>@@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = {
> };
>
> static const unsigned char jpeg_image_red[] = {
>-	0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE,
>+	0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28,
>+	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>+	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
>+	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>+	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>+	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>+	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>+	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
>+	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>+	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>+	0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9,
>+	0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0,
> 	0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
> 	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
> 	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>@@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = {
> 	0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
> 	0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
> 	0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>-	0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00
>+	0x00, 0x28, 0xA0, 0x0F
> };
>
> static const unsigned char jpeg_eoi[] = {
>@@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> 	jpeg->slot_data.cfg_stream_vaddr = NULL;
> 	jpeg->slot_data.cfg_stream_handle = 0;
>
>+	dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size,
>+			  jpeg->slot_data.cfg_dec_vaddr,
>+			  jpeg->slot_data.cfg_dec_daddr);
>+	jpeg->slot_data.cfg_dec_size = 0;
>+	jpeg->slot_data.cfg_dec_vaddr = NULL;
>+	jpeg->slot_data.cfg_dec_daddr = 0;
>+
> 	jpeg->slot_data.used = false;
> }
>
>@@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> 		goto err;
> 	jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
>
>+	jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2;
>+	jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev,
>+							   jpeg->slot_data.cfg_dec_size,
>+							   &jpeg->slot_data.cfg_dec_daddr,
>+							   GFP_ATOMIC);
>+	if (!jpeg->slot_data.cfg_dec_vaddr)
>+		goto err;
>+
> skip_alloc:
> 	jpeg->slot_data.used = true;
>
>@@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf,
> 	 */
> 	*cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> 					      V4L2_PIX_FMT_YUYV,
>-					      MXC_JPEG_MIN_WIDTH,
>-					      MXC_JPEG_MIN_HEIGHT);
>+					      MXC_JPEG_PATTERN_WIDTH,
>+					      MXC_JPEG_PATTERN_HEIGHT);
> 	cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
>-	cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
>+	cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr;
> 	cfg_desc->buf_base1 = 0;
>-	cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16;
>-	cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT;
>-	cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2;
>+	cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16;
>+	cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT;
>+	cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2;
> 	cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422);
> 	cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
> 	cfg_desc->stm_bufbase = cfg_stream_handle;
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>index 86e324b21aed..fdde45f7e163 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>@@ -28,6 +28,8 @@
> #define MXC_JPEG_W_ALIGN		3
> #define MXC_JPEG_MAX_SIZEIMAGE		0xFFFFFC00
> #define MXC_JPEG_MAX_PLANES		2
>+#define MXC_JPEG_PATTERN_WIDTH		128
>+#define MXC_JPEG_PATTERN_HEIGHT		64
>
> enum mxc_jpeg_enc_state {
> 	MXC_JPEG_ENCODING	= 0, /* jpeg encode phase */
>@@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data {
> 	dma_addr_t desc_handle;
> 	dma_addr_t cfg_desc_handle; // configuration descriptor dma address
> 	dma_addr_t cfg_stream_handle; // configuration bitstream dma address
>+	dma_addr_t cfg_dec_size;
>+	void *cfg_dec_vaddr;
>+	dma_addr_t cfg_dec_daddr;
> };
>
> struct mxc_jpeg_dev {
>-- 
>2.43.0-rc1
>
>

Re: [PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64

[Ming Qian(OSS)]

Hi Sebastian,

On 2025/4/5 23:26, Sebastian Fricke wrote:
> Hey Ming,
> 
> On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> To support decoding motion-jpeg without DHT, driver will try to decode a
>> pattern jpeg before actual jpeg frame by use of linked descriptors
>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>> used for decoding the motion-jpeg.
>>
>> To avoid performance loss, use the smallest supported resolution 64x64
>> as the pattern jpeg size.
>>
>> But there is a hardware issue: when the JPEG decoded frame with a
>> resolution that is no larger than 64x64 and it is followed by a next
>> decoded frame with a larger resolution but not 64 aligned, then this
>> next decoded frame may be corrupted.
>>
>> To avoid corruption of the decoded image, we change the pattern jpeg
>> size to 128x64, as we confirmed with the hardware designer that this is
>> a safe size.
>>
>> Besides, we also need to allocate a dma buffer to store the decoded
>> picture for the pattern image.
> 
> Why is that related to the change of the pattern size? Like why wasn't
> that needed for 64x64? And if this solves a different issue, can you put
> that into an extra patch?
> 
> This is a bit hard to understand, maybe this is better:
> 
> In order to decode a motion-jpeg bitstream, which doesn't provide a DHT,
> the driver will first decode a pattern jpeg and use the DHT found in the
> pattern to decode the first actual frame. This mode is called
> "repeat-mode" and it utilizes linked descriptors.
> The smallest supported resolution of 64x64 was used for that pattern to
> not cause unneeded performance delay. This choice, however, can cause a
> corrupted decoded picture of the first frame after the pattern, when the
> resolution of that frame is larger than the pattern and is not aligned
> to 64.
> By altering the pattern size to 128x64, this corruption can be avoided.
> That size has been confirmed to be safe by the hardware designers.
> Additionally, a DMA buffer needs to be allocated to store the decoded
> picture of the pattern image.
> 
> The rest looks good.
> 
> Regards,
> Sebastian
> 

You're right, and your description is clearer, I'll apply it.
Thanks very much.

Regards,
Ming

>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 42 +++++++++++++++----
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  5 +++
>> 2 files changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 12661c177f5a..45705c606769 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = {
>> };
>>
>> static const unsigned char jpeg_image_red[] = {
>> -    0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE,
>> +    0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28,
>> +    0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>> +    0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
>> +    0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>> +    0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>> +    0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>> +    0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>> +    0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0,
>> +    0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>> +    0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>> +    0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9,
>> +    0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0,
>>     0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00,
>>     0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>>     0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>> @@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = {
>>     0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02,
>>     0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28,
>>     0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A,
>> -    0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00
>> +    0x00, 0x28, 0xA0, 0x0F
>> };
>>
>> static const unsigned char jpeg_eoi[] = {
>> @@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct 
>> mxc_jpeg_dev *jpeg)
>>     jpeg->slot_data.cfg_stream_vaddr = NULL;
>>     jpeg->slot_data.cfg_stream_handle = 0;
>>
>> +    dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size,
>> +              jpeg->slot_data.cfg_dec_vaddr,
>> +              jpeg->slot_data.cfg_dec_daddr);
>> +    jpeg->slot_data.cfg_dec_size = 0;
>> +    jpeg->slot_data.cfg_dec_vaddr = NULL;
>> +    jpeg->slot_data.cfg_dec_daddr = 0;
>> +
>>     jpeg->slot_data.used = false;
>> }
>>
>> @@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct 
>> mxc_jpeg_dev *jpeg)
>>         goto err;
>>     jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
>>
>> +    jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * 
>> MXC_JPEG_PATTERN_HEIGHT * 2;
>> +    jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev,
>> +                               jpeg->slot_data.cfg_dec_size,
>> +                               &jpeg->slot_data.cfg_dec_daddr,
>> +                               GFP_ATOMIC);
>> +    if (!jpeg->slot_data.cfg_dec_vaddr)
>> +        goto err;
>> +
>> skip_alloc:
>>     jpeg->slot_data.used = true;
>>
>> @@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct 
>> vb2_buffer *out_buf,
>>      */
>>     *cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>>                           V4L2_PIX_FMT_YUYV,
>> -                          MXC_JPEG_MIN_WIDTH,
>> -                          MXC_JPEG_MIN_HEIGHT);
>> +                          MXC_JPEG_PATTERN_WIDTH,
>> +                          MXC_JPEG_PATTERN_HEIGHT);
>>     cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
>> -    cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
>> +    cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr;
>>     cfg_desc->buf_base1 = 0;
>> -    cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16;
>> -    cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT;
>> -    cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2;
>> +    cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16;
>> +    cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT;
>> +    cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2;
>>     cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422);
>>     cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1);
>>     cfg_desc->stm_bufbase = cfg_stream_handle;
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> index 86e324b21aed..fdde45f7e163 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> @@ -28,6 +28,8 @@
>> #define MXC_JPEG_W_ALIGN        3
>> #define MXC_JPEG_MAX_SIZEIMAGE        0xFFFFFC00
>> #define MXC_JPEG_MAX_PLANES        2
>> +#define MXC_JPEG_PATTERN_WIDTH        128
>> +#define MXC_JPEG_PATTERN_HEIGHT        64
>>
>> enum mxc_jpeg_enc_state {
>>     MXC_JPEG_ENCODING    = 0, /* jpeg encode phase */
>> @@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data {
>>     dma_addr_t desc_handle;
>>     dma_addr_t cfg_desc_handle; // configuration descriptor dma address
>>     dma_addr_t cfg_stream_handle; // configuration bitstream dma address
>> +    dma_addr_t cfg_dec_size;
>> +    void *cfg_dec_vaddr;
>> +    dma_addr_t cfg_dec_daddr;
>> };
>>
>> struct mxc_jpeg_dev {
>> -- 
>> 2.43.0-rc1
>>
>>

[PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

To support decoding motion-jpeg without DHT, driver will try to decode a
pattern jpeg before actual jpeg frame by use of linked descriptors
(This is called "repeat mode"), then the DHT in the pattern jpeg can be
used for decoding the motion-jpeg.

In other words, 2 frame done interrupts will be triggered, driver will
ignore the first interrupt, and wait for the second interrupt.
If the resolution is small, and the 2 interrupts may be too close,
when driver is handling the first interrupt, two frames are done, then
driver will fail to wait for the second interrupt.

In such case, driver can check whether the decoding is still ongoing,
if not, just done the current decoding.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
 .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
index d579c804b047..adb93e977be9 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
@@ -89,6 +89,7 @@
 /* SLOT_STATUS fields for slots 0..3 */
 #define SLOT_STATUS_FRMDONE			(0x1 << 3)
 #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
+#define SLOT_STATUS_ONGOING			(0x1 << 31)
 
 /* SLOT_IRQ_EN fields TBD */
 
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 45705c606769..e6bb45633a19 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
 	return size;
 }
 
+static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
+{
+	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
+	u32 curr_desc;
+	u32 slot_status;
+
+	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
+	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
+
+	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
+		return true;
+	if (slot_status & SLOT_STATUS_ONGOING)
+		return true;
+
+	return false;
+}
+
 static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 {
 	struct mxc_jpeg_dev *jpeg = priv;
@@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
 		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
 		goto job_unlock;
 	}
-	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
+	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
+	    mxc_dec_is_ongoing(ctx)) {
 		jpeg_src_buf->dht_needed = false;
 		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
 		goto job_unlock;
-- 
2.43.0-rc1

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Frank Li]

On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
> From: Ming Qian <ming.qian@oss.nxp.com>
>
> To support decoding motion-jpeg without DHT, driver will try to decode a
> pattern jpeg before actual jpeg frame by use of linked descriptors
> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
> used for decoding the motion-jpeg.
>
> In other words, 2 frame done interrupts will be triggered, driver will
> ignore the first interrupt,

Does any field in linked descriptors to control if issue irq? Generally
you needn't enable first descriptors's irq and only enable second one.

> and wait for the second interrupt.
> If the resolution is small, and the 2 interrupts may be too close,

It also possible two irqs combine to 1 irqs. If I understand correct, your
irq handle only handle one descriptors per irq.

Another words,

If second irq already happen just before 1,

1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */

Does your driver wait forever because no second irq happen?

Frank
>
> when driver is handling the first interrupt, two frames are done, then
> driver will fail to wait for the second interrupt.
>
> In such case, driver can check whether the decoding is still ongoing,
> if not, just done the current decoding.
>
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> index d579c804b047..adb93e977be9 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> @@ -89,6 +89,7 @@
>  /* SLOT_STATUS fields for slots 0..3 */
>  #define SLOT_STATUS_FRMDONE			(0x1 << 3)
>  #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
> +#define SLOT_STATUS_ONGOING			(0x1 << 31)
>
>  /* SLOT_IRQ_EN fields TBD */
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 45705c606769..e6bb45633a19 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
>  	return size;
>  }
>
> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
> +{
> +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> +	u32 curr_desc;
> +	u32 slot_status;
> +
> +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> +
> +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
> +		return true;
> +	if (slot_status & SLOT_STATUS_ONGOING)
> +		return true;
> +
> +	return false;
> +}
> +
>  static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>  {
>  	struct mxc_jpeg_dev *jpeg = priv;
> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>  		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>  		goto job_unlock;
>  	}
> -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
> +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
> +	    mxc_dec_is_ongoing(ctx)) {
>  		jpeg_src_buf->dht_needed = false;
>  		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>  		goto job_unlock;
> --
> 2.43.0-rc1
>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Ming Qian(OSS)]

Hi Frank,

On 2025/3/28 22:45, Frank Li wrote:
> On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> To support decoding motion-jpeg without DHT, driver will try to decode a
>> pattern jpeg before actual jpeg frame by use of linked descriptors
>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>> used for decoding the motion-jpeg.
>>
>> In other words, 2 frame done interrupts will be triggered, driver will
>> ignore the first interrupt,
> 
> Does any field in linked descriptors to control if issue irq? Generally
> you needn't enable first descriptors's irq and only enable second one.
> 

Unfortunately, we cannot configure interrupts for each descriptor.
So we can't only enable the second irq.


>> and wait for the second interrupt.
>> If the resolution is small, and the 2 interrupts may be too close,
> 
> It also possible two irqs combine to 1 irqs. If I understand correct, your
> irq handle only handle one descriptors per irq.
> 
> Another words,
> 
> If second irq already happen just before 1,
> 
> 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
> 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
> 
> Does your driver wait forever because no second irq happen?
> 
> Frank

Before this patch, the answer is yes, the driver will wait 2 seconds
until timeout.
In fact, this is the problem that this patch wants to avoid.
Now I think there are 3 cases for motion-jpeg decoding:
1. The second irq happens before the first irq status check, the 
on-going check
help to hanle this case.
2. The second irq happens after the first irq status check, but before
on-going check, this on-going check can help handle it, fisnish the
current decoding and reset the jpeg decoder.
3. The second irq happens after the on-going check, this is the normal
process before. No additional processing required.

Thanks,
Ming

>>
>> when driver is handling the first interrupt, two frames are done, then
>> driver will fail to wait for the second interrupt.
>>
>> In such case, driver can check whether the decoding is still ongoing,
>> if not, just done the current decoding.
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>>   2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> index d579c804b047..adb93e977be9 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> @@ -89,6 +89,7 @@
>>   /* SLOT_STATUS fields for slots 0..3 */
>>   #define SLOT_STATUS_FRMDONE			(0x1 << 3)
>>   #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>> +#define SLOT_STATUS_ONGOING			(0x1 << 31)
>>
>>   /* SLOT_IRQ_EN fields TBD */
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 45705c606769..e6bb45633a19 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
>>   	return size;
>>   }
>>
>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>> +{
>> +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> +	u32 curr_desc;
>> +	u32 slot_status;
>> +
>> +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>> +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>> +
>> +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>> +		return true;
>> +	if (slot_status & SLOT_STATUS_ONGOING)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>>   static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>   {
>>   	struct mxc_jpeg_dev *jpeg = priv;
>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>   		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>>   		goto job_unlock;
>>   	}
>> -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>> +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>> +	    mxc_dec_is_ongoing(ctx)) {
>>   		jpeg_src_buf->dht_needed = false;
>>   		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>   		goto job_unlock;
>> --
>> 2.43.0-rc1
>>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Frank Li]

On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
>
> Hi Frank,
>
> On 2025/3/28 22:45, Frank Li wrote:
> > On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
> > > From: Ming Qian <ming.qian@oss.nxp.com>
> > >
> > > To support decoding motion-jpeg without DHT, driver will try to decode a
> > > pattern jpeg before actual jpeg frame by use of linked descriptors
> > > (This is called "repeat mode"), then the DHT in the pattern jpeg can be
> > > used for decoding the motion-jpeg.
> > >
> > > In other words, 2 frame done interrupts will be triggered, driver will
> > > ignore the first interrupt,
> >
> > Does any field in linked descriptors to control if issue irq? Generally
> > you needn't enable first descriptors's irq and only enable second one.
> >
>
> Unfortunately, we cannot configure interrupts for each descriptor.
> So we can't only enable the second irq.
>
>
> > > and wait for the second interrupt.
> > > If the resolution is small, and the 2 interrupts may be too close,
> >
> > It also possible two irqs combine to 1 irqs. If I understand correct, your
> > irq handle only handle one descriptors per irq.
> >
> > Another words,
> >
> > If second irq already happen just before 1,
> >
> > 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
> > 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
> >
> > Does your driver wait forever because no second irq happen?
> >
> > Frank
>
> Before this patch, the answer is yes, the driver will wait 2 seconds
> until timeout.
> In fact, this is the problem that this patch wants to avoid.
> Now I think there are 3 cases for motion-jpeg decoding:
> 1. The second irq happens before the first irq status check, the on-going
> check
> help to hanle this case.
> 2. The second irq happens after the first irq status check, but before
> on-going check, this on-going check can help handle it, fisnish the
> current decoding and reset the jpeg decoder.
> 3. The second irq happens after the on-going check, this is the normal
> process before. No additional processing required.

Okay, not sure if hardware provide current_descript position. Generally
descriptor queue irq handle is like

cur = queue_header;
while(cur != read_hardware_currunt_pos())
{
	handle(cur);
	cur = cur->next;
	queue_header = cur;
}

with above logic, even you queue new request during irq handler, it should
work correctly.

But it is depend on if hardware can indicate current working queue
position, some time, hardware stop last queue posistion when handle last
one.

Frank

>
> Thanks,
> Ming
>
> > >
> > > when driver is handling the first interrupt, two frames are done, then
> > > driver will fail to wait for the second interrupt.
> > >
> > > In such case, driver can check whether the decoding is still ongoing,
> > > if not, just done the current decoding.
> > >
> > > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > > ---
> > >   .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> > >   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
> > >   2 files changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > index d579c804b047..adb93e977be9 100644
> > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > @@ -89,6 +89,7 @@
> > >   /* SLOT_STATUS fields for slots 0..3 */
> > >   #define SLOT_STATUS_FRMDONE			(0x1 << 3)
> > >   #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
> > > +#define SLOT_STATUS_ONGOING			(0x1 << 31)
> > >
> > >   /* SLOT_IRQ_EN fields TBD */
> > >
> > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > index 45705c606769..e6bb45633a19 100644
> > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
> > >   	return size;
> > >   }
> > >
> > > +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
> > > +{
> > > +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> > > +	u32 curr_desc;
> > > +	u32 slot_status;
> > > +
> > > +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> > > +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> > > +
> > > +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
> > > +		return true;
> > > +	if (slot_status & SLOT_STATUS_ONGOING)
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > >   static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > >   {
> > >   	struct mxc_jpeg_dev *jpeg = priv;
> > > @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > >   		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
> > >   		goto job_unlock;
> > >   	}
> > > -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
> > > +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
> > > +	    mxc_dec_is_ongoing(ctx)) {
> > >   		jpeg_src_buf->dht_needed = false;
> > >   		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
> > >   		goto job_unlock;
> > > --
> > > 2.43.0-rc1
> > >

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Ming Qian(OSS)]

Hi Frank,

On 2025/3/31 22:32, Frank Li wrote:
> On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
>>
>> Hi Frank,
>>
>> On 2025/3/28 22:45, Frank Li wrote:
>>> On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
>>>> From: Ming Qian <ming.qian@oss.nxp.com>
>>>>
>>>> To support decoding motion-jpeg without DHT, driver will try to decode a
>>>> pattern jpeg before actual jpeg frame by use of linked descriptors
>>>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>>>> used for decoding the motion-jpeg.
>>>>
>>>> In other words, 2 frame done interrupts will be triggered, driver will
>>>> ignore the first interrupt,
>>>
>>> Does any field in linked descriptors to control if issue irq? Generally
>>> you needn't enable first descriptors's irq and only enable second one.
>>>
>>
>> Unfortunately, we cannot configure interrupts for each descriptor.
>> So we can't only enable the second irq.
>>
>>
>>>> and wait for the second interrupt.
>>>> If the resolution is small, and the 2 interrupts may be too close,
>>>
>>> It also possible two irqs combine to 1 irqs. If I understand correct, your
>>> irq handle only handle one descriptors per irq.
>>>
>>> Another words,
>>>
>>> If second irq already happen just before 1,
>>>
>>> 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
>>> 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
>>>
>>> Does your driver wait forever because no second irq happen?
>>>
>>> Frank
>>
>> Before this patch, the answer is yes, the driver will wait 2 seconds
>> until timeout.
>> In fact, this is the problem that this patch wants to avoid.
>> Now I think there are 3 cases for motion-jpeg decoding:
>> 1. The second irq happens before the first irq status check, the on-going
>> check
>> help to hanle this case.
>> 2. The second irq happens after the first irq status check, but before
>> on-going check, this on-going check can help handle it, fisnish the
>> current decoding and reset the jpeg decoder.
>> 3. The second irq happens after the on-going check, this is the normal
>> process before. No additional processing required.
> 
> Okay, not sure if hardware provide current_descript position. Generally
> descriptor queue irq handle is like
> 
> cur = queue_header;
> while(cur != read_hardware_currunt_pos())
> {
> 	handle(cur);
> 	cur = cur->next;
> 	queue_header = cur;
> }
> 
> with above logic, even you queue new request during irq handler, it should
> work correctly.
> 
> But it is depend on if hardware can indicate current working queue
> position, some time, hardware stop last queue posistion when handle last
> one.
> 
> Frank
> 

I think it doesn't matter, the 2 descriptors are the cfg descriptor and
then the image descriptor.
If the current descriptor register remains the last image descriptor,
the ongoing check works.

And I guess your concern is as below.
If the current descriptor register is still the cfg descriptor, but the
hardware has finished decoding the next image descriptor.

I confirmed with our hardware engineer. This can't happen.
The first cfg decriptor has a next_descpt_ptr that is pointing to the
image descriptor, when the hardware read tne next_descpt_ptr, the
current descriptor register is updated, before the actual decoding.

Thanks,
Ming

>>
>> Thanks,
>> Ming
>>
>>>>
>>>> when driver is handling the first interrupt, two frames are done, then
>>>> driver will fail to wait for the second interrupt.
>>>>
>>>> In such case, driver can check whether the decoding is still ongoing,
>>>> if not, just done the current decoding.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>> ---
>>>>    .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>>>>    .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>>>>    2 files changed, 20 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>> index d579c804b047..adb93e977be9 100644
>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>> @@ -89,6 +89,7 @@
>>>>    /* SLOT_STATUS fields for slots 0..3 */
>>>>    #define SLOT_STATUS_FRMDONE			(0x1 << 3)
>>>>    #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>>>> +#define SLOT_STATUS_ONGOING			(0x1 << 31)
>>>>
>>>>    /* SLOT_IRQ_EN fields TBD */
>>>>
>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>> index 45705c606769..e6bb45633a19 100644
>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
>>>>    	return size;
>>>>    }
>>>>
>>>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>>>> +{
>>>> +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>>>> +	u32 curr_desc;
>>>> +	u32 slot_status;
>>>> +
>>>> +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>>>> +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>>>> +
>>>> +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>>>> +		return true;
>>>> +	if (slot_status & SLOT_STATUS_ONGOING)
>>>> +		return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>    static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>    {
>>>>    	struct mxc_jpeg_dev *jpeg = priv;
>>>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>    		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>>>>    		goto job_unlock;
>>>>    	}
>>>> -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>>>> +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>>>> +	    mxc_dec_is_ongoing(ctx)) {
>>>>    		jpeg_src_buf->dht_needed = false;
>>>>    		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>>>    		goto job_unlock;
>>>> --
>>>> 2.43.0-rc1
>>>>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Frank Li]

On Tue, Apr 01, 2025 at 11:17:36AM +0800, Ming Qian(OSS) wrote:
>
> Hi Frank,
>
> On 2025/3/31 22:32, Frank Li wrote:
> > On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
> > >
> > > Hi Frank,
> > >
> > > On 2025/3/28 22:45, Frank Li wrote:
> > > > On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
> > > > > From: Ming Qian <ming.qian@oss.nxp.com>
> > > > >
> > > > > To support decoding motion-jpeg without DHT, driver will try to decode a
> > > > > pattern jpeg before actual jpeg frame by use of linked descriptors
> > > > > (This is called "repeat mode"), then the DHT in the pattern jpeg can be
> > > > > used for decoding the motion-jpeg.
> > > > >
> > > > > In other words, 2 frame done interrupts will be triggered, driver will
> > > > > ignore the first interrupt,
> > > >
> > > > Does any field in linked descriptors to control if issue irq? Generally
> > > > you needn't enable first descriptors's irq and only enable second one.
> > > >
> > >
> > > Unfortunately, we cannot configure interrupts for each descriptor.
> > > So we can't only enable the second irq.
> > >
> > >
> > > > > and wait for the second interrupt.
> > > > > If the resolution is small, and the 2 interrupts may be too close,
> > > >
> > > > It also possible two irqs combine to 1 irqs. If I understand correct, your
> > > > irq handle only handle one descriptors per irq.
> > > >
> > > > Another words,
> > > >
> > > > If second irq already happen just before 1,
> > > >
> > > > 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
> > > > 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
> > > >
> > > > Does your driver wait forever because no second irq happen?
> > > >
> > > > Frank
> > >
> > > Before this patch, the answer is yes, the driver will wait 2 seconds
> > > until timeout.
> > > In fact, this is the problem that this patch wants to avoid.
> > > Now I think there are 3 cases for motion-jpeg decoding:
> > > 1. The second irq happens before the first irq status check, the on-going
> > > check
> > > help to hanle this case.
> > > 2. The second irq happens after the first irq status check, but before
> > > on-going check, this on-going check can help handle it, fisnish the
> > > current decoding and reset the jpeg decoder.
> > > 3. The second irq happens after the on-going check, this is the normal
> > > process before. No additional processing required.
> >
> > Okay, not sure if hardware provide current_descript position. Generally
> > descriptor queue irq handle is like
> >
> > cur = queue_header;
> > while(cur != read_hardware_currunt_pos())
> > {
> > 	handle(cur);
> > 	cur = cur->next;
> > 	queue_header = cur;
> > }
> >
> > with above logic, even you queue new request during irq handler, it should
> > work correctly.
> >
> > But it is depend on if hardware can indicate current working queue
> > position, some time, hardware stop last queue posistion when handle last
> > one.
> >
> > Frank
> >
>
> I think it doesn't matter, the 2 descriptors are the cfg descriptor and
> then the image descriptor.
> If the current descriptor register remains the last image descriptor,
> the ongoing check works.
>
> And I guess your concern is as below.
> If the current descriptor register is still the cfg descriptor, but the
> hardware has finished decoding the next image descriptor.
>
> I confirmed with our hardware engineer. This can't happen.
> The first cfg decriptor has a next_descpt_ptr that is pointing to the
> image descriptor, when the hardware read tne next_descpt_ptr, the
> current descriptor register is updated, before the actual decoding.

Maybe off topic,

CFG->next = Image

Image->next = NULL;

If hardware finish image descriptior, current descriptor is 'Image' or 'NULL'

If it is 'Image', need extra status bit show 'done'

1:	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));

I suppose it should be DONE status if just finish CFG description.

2: 	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));

It is possible curr_desc already was 'Image' after 1.

 if (curr_desc == jpeg->slot_data.cfg_desc_handle)  //not hit this
        return true;

 if (slot_status & SLOT_STATUS_ONGOING)  // not hit this
        return true;

fake false may return.

check two aync condition "slot_status" and "curr_desc" always be risk. But
I don't know what's happen if fake false return here.

for this type check
	do {
		slot_status = readl();
		curr_desc = readl();
	} while (slot_status != read());

to make sure slot_status and cur_desc indicate the hardware status
correctly.

Frank
>
> Thanks,
> Ming
>
> > >
> > > Thanks,
> > > Ming
> > >
> > > > >
> > > > > when driver is handling the first interrupt, two frames are done, then
> > > > > driver will fail to wait for the second interrupt.
> > > > >
> > > > > In such case, driver can check whether the decoding is still ongoing,
> > > > > if not, just done the current decoding.
> > > > >
> > > > > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > > > > ---
> > > > >    .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> > > > >    .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
> > > > >    2 files changed, 20 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > index d579c804b047..adb93e977be9 100644
> > > > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > @@ -89,6 +89,7 @@
> > > > >    /* SLOT_STATUS fields for slots 0..3 */
> > > > >    #define SLOT_STATUS_FRMDONE			(0x1 << 3)
> > > > >    #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
> > > > > +#define SLOT_STATUS_ONGOING			(0x1 << 31)
> > > > >
> > > > >    /* SLOT_IRQ_EN fields TBD */
> > > > >
> > > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > index 45705c606769..e6bb45633a19 100644
> > > > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
> > > > >    	return size;
> > > > >    }
> > > > >
> > > > > +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
> > > > > +{
> > > > > +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> > > > > +	u32 curr_desc;
> > > > > +	u32 slot_status;
> > > > > +
> > > > > +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> > > > > +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> > > > > +
> > > > > +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
> > > > > +		return true;
> > > > > +	if (slot_status & SLOT_STATUS_ONGOING)
> > > > > +		return true;
> > > > > +
> > > > > +	return false;
> > > > > +}
> > > > > +
> > > > >    static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > > > >    {
> > > > >    	struct mxc_jpeg_dev *jpeg = priv;
> > > > > @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > > > >    		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
> > > > >    		goto job_unlock;
> > > > >    	}
> > > > > -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
> > > > > +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
> > > > > +	    mxc_dec_is_ongoing(ctx)) {
> > > > >    		jpeg_src_buf->dht_needed = false;
> > > > >    		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
> > > > >    		goto job_unlock;
> > > > > --
> > > > > 2.43.0-rc1
> > > > >

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Ming Qian(OSS)]

Hi Frank,


On 2025/4/1 22:37, Frank Li wrote:
> On Tue, Apr 01, 2025 at 11:17:36AM +0800, Ming Qian(OSS) wrote:
>>
>> Hi Frank,
>>
>> On 2025/3/31 22:32, Frank Li wrote:
>>> On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
>>>>
>>>> Hi Frank,
>>>>
>>>> On 2025/3/28 22:45, Frank Li wrote:
>>>>> On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
>>>>>> From: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>
>>>>>> To support decoding motion-jpeg without DHT, driver will try to decode a
>>>>>> pattern jpeg before actual jpeg frame by use of linked descriptors
>>>>>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>>>>>> used for decoding the motion-jpeg.
>>>>>>
>>>>>> In other words, 2 frame done interrupts will be triggered, driver will
>>>>>> ignore the first interrupt,
>>>>>
>>>>> Does any field in linked descriptors to control if issue irq? Generally
>>>>> you needn't enable first descriptors's irq and only enable second one.
>>>>>
>>>>
>>>> Unfortunately, we cannot configure interrupts for each descriptor.
>>>> So we can't only enable the second irq.
>>>>
>>>>
>>>>>> and wait for the second interrupt.
>>>>>> If the resolution is small, and the 2 interrupts may be too close,
>>>>>
>>>>> It also possible two irqs combine to 1 irqs. If I understand correct, your
>>>>> irq handle only handle one descriptors per irq.
>>>>>
>>>>> Another words,
>>>>>
>>>>> If second irq already happen just before 1,
>>>>>
>>>>> 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
>>>>> 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
>>>>>
>>>>> Does your driver wait forever because no second irq happen?
>>>>>
>>>>> Frank
>>>>
>>>> Before this patch, the answer is yes, the driver will wait 2 seconds
>>>> until timeout.
>>>> In fact, this is the problem that this patch wants to avoid.
>>>> Now I think there are 3 cases for motion-jpeg decoding:
>>>> 1. The second irq happens before the first irq status check, the on-going
>>>> check
>>>> help to hanle this case.
>>>> 2. The second irq happens after the first irq status check, but before
>>>> on-going check, this on-going check can help handle it, fisnish the
>>>> current decoding and reset the jpeg decoder.
>>>> 3. The second irq happens after the on-going check, this is the normal
>>>> process before. No additional processing required.
>>>
>>> Okay, not sure if hardware provide current_descript position. Generally
>>> descriptor queue irq handle is like
>>>
>>> cur = queue_header;
>>> while(cur != read_hardware_currunt_pos())
>>> {
>>> 	handle(cur);
>>> 	cur = cur->next;
>>> 	queue_header = cur;
>>> }
>>>
>>> with above logic, even you queue new request during irq handler, it should
>>> work correctly.
>>>
>>> But it is depend on if hardware can indicate current working queue
>>> position, some time, hardware stop last queue posistion when handle last
>>> one.
>>>
>>> Frank
>>>
>>
>> I think it doesn't matter, the 2 descriptors are the cfg descriptor and
>> then the image descriptor.
>> If the current descriptor register remains the last image descriptor,
>> the ongoing check works.
>>
>> And I guess your concern is as below.
>> If the current descriptor register is still the cfg descriptor, but the
>> hardware has finished decoding the next image descriptor.
>>
>> I confirmed with our hardware engineer. This can't happen.
>> The first cfg decriptor has a next_descpt_ptr that is pointing to the
>> image descriptor, when the hardware read tne next_descpt_ptr, the
>> current descriptor register is updated, before the actual decoding.
> 
> Maybe off topic,
> 
> CFG->next = Image
> 
> Image->next = NULL;
> 
> If hardware finish image descriptior, current descriptor is 'Image' or 'NULL'
> 
> If it is 'Image', need extra status bit show 'done'
> 
> 1:	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> 
> I suppose it should be DONE status if just finish CFG description.
> 
> 2: 	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> 
> It is possible curr_desc already was 'Image' after 1.
> 
>   if (curr_desc == jpeg->slot_data.cfg_desc_handle)  //not hit this
>          return true;
> 
>   if (slot_status & SLOT_STATUS_ONGOING)  // not hit this
>          return true;
> 
> fake false may return.
> 
> check two aync condition "slot_status" and "curr_desc" always be risk. But
> I don't know what's happen if fake false return here.
> 
> for this type check
> 	do {
> 		slot_status = readl();
> 		curr_desc = readl();
> 	} while (slot_status != read());
> 
> to make sure slot_status and cur_desc indicate the hardware status
> correctly.

I confirmed with the hardware engineer, the curr_desc register is set
when hardware load next_descpt_ptr, but the ongoing bit is set when
hardware finish loading the content of the descriptor, the size is 32
bytes.
So you are right, the slot_status and curr_desc is not synchronous,
but the gap time will be very short

This patch is a workaround that hardware finish 2 descriptors too soon,
irq() is not scheduled on time, the driver keeps waiting until timeout.

And I agree there is still some risk that the ongoing check may return
fake false, even if the probability of occurrence is extremely low.

When the fake false return, the driver will finish current decoding
early, and the decoded image is incomplete.

But we don't want to change to poll the done status.

Considering the probability of occurrence and the respective
consequences, I think this patch still makes sense.

Maybe we can check the slot_status register twice and add a short delay
in between. Then the probability of returning fake false is basically
reduced to 0.

Thanks,
Ming

> 
> Frank
>>
>> Thanks,
>> Ming
>>
>>>>
>>>> Thanks,
>>>> Ming
>>>>
>>>>>>
>>>>>> when driver is handling the first interrupt, two frames are done, then
>>>>>> driver will fail to wait for the second interrupt.
>>>>>>
>>>>>> In such case, driver can check whether the decoding is still ongoing,
>>>>>> if not, just done the current decoding.
>>>>>>
>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>>> ---
>>>>>>     .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>>>>>>     .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>>>>>>     2 files changed, 20 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>> index d579c804b047..adb93e977be9 100644
>>>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>> @@ -89,6 +89,7 @@
>>>>>>     /* SLOT_STATUS fields for slots 0..3 */
>>>>>>     #define SLOT_STATUS_FRMDONE			(0x1 << 3)
>>>>>>     #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>>>>>> +#define SLOT_STATUS_ONGOING			(0x1 << 31)
>>>>>>
>>>>>>     /* SLOT_IRQ_EN fields TBD */
>>>>>>
>>>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>> index 45705c606769..e6bb45633a19 100644
>>>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
>>>>>>     	return size;
>>>>>>     }
>>>>>>
>>>>>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>>>>>> +{
>>>>>> +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>>>>>> +	u32 curr_desc;
>>>>>> +	u32 slot_status;
>>>>>> +
>>>>>> +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>>>>>> +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>>>>>> +
>>>>>> +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>>>>>> +		return true;
>>>>>> +	if (slot_status & SLOT_STATUS_ONGOING)
>>>>>> +		return true;
>>>>>> +
>>>>>> +	return false;
>>>>>> +}
>>>>>> +
>>>>>>     static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>>>     {
>>>>>>     	struct mxc_jpeg_dev *jpeg = priv;
>>>>>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>>>     		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>>>>>>     		goto job_unlock;
>>>>>>     	}
>>>>>> -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>>>>>> +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>>>>>> +	    mxc_dec_is_ongoing(ctx)) {
>>>>>>     		jpeg_src_buf->dht_needed = false;
>>>>>>     		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>>>>>     		goto job_unlock;
>>>>>> --
>>>>>> 2.43.0-rc1
>>>>>>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Frank Li]

On Wed, Apr 02, 2025 at 01:34:00PM +0800, Ming Qian(OSS) wrote:
> Hi Frank,
>
>
> On 2025/4/1 22:37, Frank Li wrote:
> > On Tue, Apr 01, 2025 at 11:17:36AM +0800, Ming Qian(OSS) wrote:
> > >
> > > Hi Frank,
> > >
> > > On 2025/3/31 22:32, Frank Li wrote:
> > > > On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
> > > > >
> > > > > Hi Frank,
> > > > >
> > > > > On 2025/3/28 22:45, Frank Li wrote:
> > > > > > On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
> > > > > > > From: Ming Qian <ming.qian@oss.nxp.com>
> > > > > > >
> > > > > > > To support decoding motion-jpeg without DHT, driver will try to decode a
> > > > > > > pattern jpeg before actual jpeg frame by use of linked descriptors
> > > > > > > (This is called "repeat mode"), then the DHT in the pattern jpeg can be
> > > > > > > used for decoding the motion-jpeg.
> > > > > > >
> > > > > > > In other words, 2 frame done interrupts will be triggered, driver will
> > > > > > > ignore the first interrupt,
> > > > > >
> > > > > > Does any field in linked descriptors to control if issue irq? Generally
> > > > > > you needn't enable first descriptors's irq and only enable second one.
> > > > > >
> > > > >
> > > > > Unfortunately, we cannot configure interrupts for each descriptor.
> > > > > So we can't only enable the second irq.
> > > > >
> > > > >
> > > > > > > and wait for the second interrupt.
> > > > > > > If the resolution is small, and the 2 interrupts may be too close,
> > > > > >
> > > > > > It also possible two irqs combine to 1 irqs. If I understand correct, your
> > > > > > irq handle only handle one descriptors per irq.
> > > > > >
> > > > > > Another words,
> > > > > >
> > > > > > If second irq already happen just before 1,
> > > > > >
> > > > > > 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
> > > > > > 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
> > > > > >
> > > > > > Does your driver wait forever because no second irq happen?
> > > > > >
> > > > > > Frank
> > > > >
> > > > > Before this patch, the answer is yes, the driver will wait 2 seconds
> > > > > until timeout.
> > > > > In fact, this is the problem that this patch wants to avoid.
> > > > > Now I think there are 3 cases for motion-jpeg decoding:
> > > > > 1. The second irq happens before the first irq status check, the on-going
> > > > > check
> > > > > help to hanle this case.
> > > > > 2. The second irq happens after the first irq status check, but before
> > > > > on-going check, this on-going check can help handle it, fisnish the
> > > > > current decoding and reset the jpeg decoder.
> > > > > 3. The second irq happens after the on-going check, this is the normal
> > > > > process before. No additional processing required.
> > > >
> > > > Okay, not sure if hardware provide current_descript position. Generally
> > > > descriptor queue irq handle is like
> > > >
> > > > cur = queue_header;
> > > > while(cur != read_hardware_currunt_pos())
> > > > {
> > > > 	handle(cur);
> > > > 	cur = cur->next;
> > > > 	queue_header = cur;
> > > > }
> > > >
> > > > with above logic, even you queue new request during irq handler, it should
> > > > work correctly.
> > > >
> > > > But it is depend on if hardware can indicate current working queue
> > > > position, some time, hardware stop last queue posistion when handle last
> > > > one.
> > > >
> > > > Frank
> > > >
> > >
> > > I think it doesn't matter, the 2 descriptors are the cfg descriptor and
> > > then the image descriptor.
> > > If the current descriptor register remains the last image descriptor,
> > > the ongoing check works.
> > >
> > > And I guess your concern is as below.
> > > If the current descriptor register is still the cfg descriptor, but the
> > > hardware has finished decoding the next image descriptor.
> > >
> > > I confirmed with our hardware engineer. This can't happen.
> > > The first cfg decriptor has a next_descpt_ptr that is pointing to the
> > > image descriptor, when the hardware read tne next_descpt_ptr, the
> > > current descriptor register is updated, before the actual decoding.
> >
> > Maybe off topic,
> >
> > CFG->next = Image
> >
> > Image->next = NULL;
> >
> > If hardware finish image descriptior, current descriptor is 'Image' or 'NULL'
> >
> > If it is 'Image', need extra status bit show 'done'
> >
> > 1:	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> >
> > I suppose it should be DONE status if just finish CFG description.
> >
> > 2: 	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> >
> > It is possible curr_desc already was 'Image' after 1.
> >
> >   if (curr_desc == jpeg->slot_data.cfg_desc_handle)  //not hit this
> >          return true;
> >
> >   if (slot_status & SLOT_STATUS_ONGOING)  // not hit this
> >          return true;
> >
> > fake false may return.
> >
> > check two aync condition "slot_status" and "curr_desc" always be risk. But
> > I don't know what's happen if fake false return here.
> >
> > for this type check
> > 	do {
> > 		slot_status = readl();
> > 		curr_desc = readl();
> > 	} while (slot_status != read());
> >
> > to make sure slot_status and cur_desc indicate the hardware status
> > correctly.
>
> I confirmed with the hardware engineer, the curr_desc register is set
> when hardware load next_descpt_ptr,

The key is last one, does last one set curr_desc to NULL when it done.
If yes, the whole logic will be much simple. You just need check
curr_desc.

> but the ongoing bit is set when
> hardware finish loading the content of the descriptor, the size is 32
> bytes.
> So you are right, the slot_status and curr_desc is not synchronous,
> but the gap time will be very short

Yes, it will be really hard to debug it if met once.

>
> This patch is a workaround that hardware finish 2 descriptors too soon,
> irq() is not scheduled on time, the driver keeps waiting until timeout.
>
> And I agree there is still some risk that the ongoing check may return
> fake false, even if the probability of occurrence is extremely low.
>
> When the fake false return, the driver will finish current decoding
> early, and the decoded image is incomplete.
>
> But we don't want to change to poll the done status.

It is not poll the done. It's compare if status is the same by twice read.
Most time needn't retry.

The basic idea is the same as
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clocksource/arm_global_timer.c#L73

>
> Considering the probability of occurrence and the respective
> consequences, I think this patch still makes sense.
>
> Maybe we can check the slot_status register twice and add a short delay
> in between. Then the probability of returning fake false is basically
> reduced to 0.
>
> Thanks,
> Ming
>
> >
> > Frank
> > >
> > > Thanks,
> > > Ming
> > >
> > > > >
> > > > > Thanks,
> > > > > Ming
> > > > >
> > > > > > >
> > > > > > > when driver is handling the first interrupt, two frames are done, then
> > > > > > > driver will fail to wait for the second interrupt.
> > > > > > >
> > > > > > > In such case, driver can check whether the decoding is still ongoing,
> > > > > > > if not, just done the current decoding.
> > > > > > >
> > > > > > > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > > > > > > ---
> > > > > > >     .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> > > > > > >     .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
> > > > > > >     2 files changed, 20 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > > > index d579c804b047..adb93e977be9 100644
> > > > > > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > > > > > > @@ -89,6 +89,7 @@
> > > > > > >     /* SLOT_STATUS fields for slots 0..3 */
> > > > > > >     #define SLOT_STATUS_FRMDONE			(0x1 << 3)
> > > > > > >     #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
> > > > > > > +#define SLOT_STATUS_ONGOING			(0x1 << 31)
> > > > > > >
> > > > > > >     /* SLOT_IRQ_EN fields TBD */
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > > > index 45705c606769..e6bb45633a19 100644
> > > > > > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > > > > > > @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
> > > > > > >     	return size;
> > > > > > >     }
> > > > > > >
> > > > > > > +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
> > > > > > > +{
> > > > > > > +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> > > > > > > +	u32 curr_desc;
> > > > > > > +	u32 slot_status;
> > > > > > > +
> > > > > > > +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
> > > > > > > +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
> > > > > > > +
> > > > > > > +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
> > > > > > > +		return true;
> > > > > > > +	if (slot_status & SLOT_STATUS_ONGOING)
> > > > > > > +		return true;
> > > > > > > +
> > > > > > > +	return false;
> > > > > > > +}
> > > > > > > +
> > > > > > >     static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > > > > > >     {
> > > > > > >     	struct mxc_jpeg_dev *jpeg = priv;
> > > > > > > @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> > > > > > >     		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
> > > > > > >     		goto job_unlock;
> > > > > > >     	}
> > > > > > > -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
> > > > > > > +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
> > > > > > > +	    mxc_dec_is_ongoing(ctx)) {
> > > > > > >     		jpeg_src_buf->dht_needed = false;
> > > > > > >     		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
> > > > > > >     		goto job_unlock;
> > > > > > > --
> > > > > > > 2.43.0-rc1
> > > > > > >

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Ming Qian(OSS)]

Hi Frank,

On 2025/4/3 2:03, Frank Li wrote:
> On Wed, Apr 02, 2025 at 01:34:00PM +0800, Ming Qian(OSS) wrote:
>> Hi Frank,
>>
>>
>> On 2025/4/1 22:37, Frank Li wrote:
>>> On Tue, Apr 01, 2025 at 11:17:36AM +0800, Ming Qian(OSS) wrote:
>>>>
>>>> Hi Frank,
>>>>
>>>> On 2025/3/31 22:32, Frank Li wrote:
>>>>> On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote:
>>>>>>
>>>>>> Hi Frank,
>>>>>>
>>>>>> On 2025/3/28 22:45, Frank Li wrote:
>>>>>>> On Fri, Mar 28, 2025 at 02:30:52PM +0800, ming.qian@oss.nxp.com wrote:
>>>>>>>> From: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>>>
>>>>>>>> To support decoding motion-jpeg without DHT, driver will try to decode a
>>>>>>>> pattern jpeg before actual jpeg frame by use of linked descriptors
>>>>>>>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>>>>>>>> used for decoding the motion-jpeg.
>>>>>>>>
>>>>>>>> In other words, 2 frame done interrupts will be triggered, driver will
>>>>>>>> ignore the first interrupt,
>>>>>>>
>>>>>>> Does any field in linked descriptors to control if issue irq? Generally
>>>>>>> you needn't enable first descriptors's irq and only enable second one.
>>>>>>>
>>>>>>
>>>>>> Unfortunately, we cannot configure interrupts for each descriptor.
>>>>>> So we can't only enable the second irq.
>>>>>>
>>>>>>
>>>>>>>> and wait for the second interrupt.
>>>>>>>> If the resolution is small, and the 2 interrupts may be too close,
>>>>>>>
>>>>>>> It also possible two irqs combine to 1 irqs. If I understand correct, your
>>>>>>> irq handle only handle one descriptors per irq.
>>>>>>>
>>>>>>> Another words,
>>>>>>>
>>>>>>> If second irq already happen just before 1,
>>>>>>>
>>>>>>> 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS));
>>>>>>> 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */
>>>>>>>
>>>>>>> Does your driver wait forever because no second irq happen?
>>>>>>>
>>>>>>> Frank
>>>>>>
>>>>>> Before this patch, the answer is yes, the driver will wait 2 seconds
>>>>>> until timeout.
>>>>>> In fact, this is the problem that this patch wants to avoid.
>>>>>> Now I think there are 3 cases for motion-jpeg decoding:
>>>>>> 1. The second irq happens before the first irq status check, the on-going
>>>>>> check
>>>>>> help to hanle this case.
>>>>>> 2. The second irq happens after the first irq status check, but before
>>>>>> on-going check, this on-going check can help handle it, fisnish the
>>>>>> current decoding and reset the jpeg decoder.
>>>>>> 3. The second irq happens after the on-going check, this is the normal
>>>>>> process before. No additional processing required.
>>>>>
>>>>> Okay, not sure if hardware provide current_descript position. Generally
>>>>> descriptor queue irq handle is like
>>>>>
>>>>> cur = queue_header;
>>>>> while(cur != read_hardware_currunt_pos())
>>>>> {
>>>>> 	handle(cur);
>>>>> 	cur = cur->next;
>>>>> 	queue_header = cur;
>>>>> }
>>>>>
>>>>> with above logic, even you queue new request during irq handler, it should
>>>>> work correctly.
>>>>>
>>>>> But it is depend on if hardware can indicate current working queue
>>>>> position, some time, hardware stop last queue posistion when handle last
>>>>> one.
>>>>>
>>>>> Frank
>>>>>
>>>>
>>>> I think it doesn't matter, the 2 descriptors are the cfg descriptor and
>>>> then the image descriptor.
>>>> If the current descriptor register remains the last image descriptor,
>>>> the ongoing check works.
>>>>
>>>> And I guess your concern is as below.
>>>> If the current descriptor register is still the cfg descriptor, but the
>>>> hardware has finished decoding the next image descriptor.
>>>>
>>>> I confirmed with our hardware engineer. This can't happen.
>>>> The first cfg decriptor has a next_descpt_ptr that is pointing to the
>>>> image descriptor, when the hardware read tne next_descpt_ptr, the
>>>> current descriptor register is updated, before the actual decoding.
>>>
>>> Maybe off topic,
>>>
>>> CFG->next = Image
>>>
>>> Image->next = NULL;
>>>
>>> If hardware finish image descriptior, current descriptor is 'Image' or 'NULL'
>>>
>>> If it is 'Image', need extra status bit show 'done'
>>>
>>> 1:	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>>>
>>> I suppose it should be DONE status if just finish CFG description.
>>>
>>> 2: 	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>>>
>>> It is possible curr_desc already was 'Image' after 1.
>>>
>>>    if (curr_desc == jpeg->slot_data.cfg_desc_handle)  //not hit this
>>>           return true;
>>>
>>>    if (slot_status & SLOT_STATUS_ONGOING)  // not hit this
>>>           return true;
>>>
>>> fake false may return.
>>>
>>> check two aync condition "slot_status" and "curr_desc" always be risk. But
>>> I don't know what's happen if fake false return here.
>>>
>>> for this type check
>>> 	do {
>>> 		slot_status = readl();
>>> 		curr_desc = readl();
>>> 	} while (slot_status != read());
>>>
>>> to make sure slot_status and cur_desc indicate the hardware status
>>> correctly.
>>
>> I confirmed with the hardware engineer, the curr_desc register is set
>> when hardware load next_descpt_ptr,
> 
> The key is last one, does last one set curr_desc to NULL when it done.
> If yes, the whole logic will be much simple. You just need check
> curr_desc.

No, the curr_desc will remain the last image descripor even after done.

> 
>> but the ongoing bit is set when
>> hardware finish loading the content of the descriptor, the size is 32
>> bytes.
>> So you are right, the slot_status and curr_desc is not synchronous,
>> but the gap time will be very short
> 
> Yes, it will be really hard to debug it if met once.
> 

I think we can read the slot_status twice to reduce the probability to
almost 0.



>>
>> This patch is a workaround that hardware finish 2 descriptors too soon,
>> irq() is not scheduled on time, the driver keeps waiting until timeout.
>>
>> And I agree there is still some risk that the ongoing check may return
>> fake false, even if the probability of occurrence is extremely low.
>>
>> When the fake false return, the driver will finish current decoding
>> early, and the decoded image is incomplete.
>>
>> But we don't want to change to poll the done status.
> 
> It is not poll the done. It's compare if status is the same by twice read.
> Most time needn't retry.
> 
> The basic idea is the same as
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clocksource/arm_global_timer.c#L73
> 

I checked the _gt_counter_read(), But I think its usage scenario is
different from here.

In _gt_counter_read(), its purpose is to ensure that the upper 32-bits
do not change when reading the lower 32-bits.
1. Read the upper 32-bit timer counter register
2. Read the lower 32-bit timer counter register
3. Read the upper 32-bit timer counter register again.
If the value in step 1 is equal to step 3, it means the upper 32 bits
are not changed, and the lower 32-bits are right.

But here, we don't expect the slot_status is the same by twice read.
Now the case is:
1. curr_desc is switched to the image descriptor.
2. the ongoing bit is set after the 32 bytes descriptor is loaded.
The 2 steps are not synchronized.

If slot_status is read twice between step 1 and 2, the value is same,
but still return fake false.

I'd like to check the slot_status twice and add a delay:

slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
SLOT_STATUS));
if (slot_status & SLOT_STATUS_ONGOING)
	return true;

udelay(10);

slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
SLOT_STATUS));
if (slot_status & SLOT_STATUS_ONGOING)
	return true;

return false;

This delay should be able to avoid stepping into the intermediate state
between steps 1 and 2.

Thanks,
Ming

>>
>> Considering the probability of occurrence and the respective
>> consequences, I think this patch still makes sense.
>>
>> Maybe we can check the slot_status register twice and add a short delay
>> in between. Then the probability of returning fake false is basically
>> reduced to 0.
>>
>> Thanks,
>> Ming
>>
>>>
>>> Frank
>>>>
>>>> Thanks,
>>>> Ming
>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Ming
>>>>>>
>>>>>>>>
>>>>>>>> when driver is handling the first interrupt, two frames are done, then
>>>>>>>> driver will fail to wait for the second interrupt.
>>>>>>>>
>>>>>>>> In such case, driver can check whether the decoding is still ongoing,
>>>>>>>> if not, just done the current decoding.
>>>>>>>>
>>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>>> ---
>>>>>>>>      .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>>>>>>>>      .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>>>>>>>>      2 files changed, 20 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>>>> index d579c804b047..adb93e977be9 100644
>>>>>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>>>>>>>> @@ -89,6 +89,7 @@
>>>>>>>>      /* SLOT_STATUS fields for slots 0..3 */
>>>>>>>>      #define SLOT_STATUS_FRMDONE			(0x1 << 3)
>>>>>>>>      #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>>>>>>>> +#define SLOT_STATUS_ONGOING			(0x1 << 31)
>>>>>>>>
>>>>>>>>      /* SLOT_IRQ_EN fields TBD */
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>>>> index 45705c606769..e6bb45633a19 100644
>>>>>>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>>>>>>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
>>>>>>>>      	return size;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>>>>>>>> +{
>>>>>>>> +	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>>>>>>>> +	u32 curr_desc;
>>>>>>>> +	u32 slot_status;
>>>>>>>> +
>>>>>>>> +	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>>>>>>>> +	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>>>>>>>> +
>>>>>>>> +	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>>>>>>>> +		return true;
>>>>>>>> +	if (slot_status & SLOT_STATUS_ONGOING)
>>>>>>>> +		return true;
>>>>>>>> +
>>>>>>>> +	return false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>>>>>      {
>>>>>>>>      	struct mxc_jpeg_dev *jpeg = priv;
>>>>>>>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>>>>>>>>      		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
>>>>>>>>      		goto job_unlock;
>>>>>>>>      	}
>>>>>>>> -	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>>>>>>>> +	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>>>>>>>> +	    mxc_dec_is_ongoing(ctx)) {
>>>>>>>>      		jpeg_src_buf->dht_needed = false;
>>>>>>>>      		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>>>>>>>      		goto job_unlock;
>>>>>>>> --
>>>>>>>> 2.43.0-rc1
>>>>>>>>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Sebastian Fricke]

Hey Ming,

On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>From: Ming Qian <ming.qian@oss.nxp.com>
>
>To support decoding motion-jpeg without DHT, driver will try to decode a
>pattern jpeg before actual jpeg frame by use of linked descriptors
>(This is called "repeat mode"), then the DHT in the pattern jpeg can be
>used for decoding the motion-jpeg.

Hmm do we need to repeat the description from the previous patch?
>
>In other words, 2 frame done interrupts will be triggered, driver will
>ignore the first interrupt, and wait for the second interrupt.
>If the resolution is small, and the 2 interrupts may be too close,
>when driver is handling the first interrupt, two frames are done, then
>driver will fail to wait for the second interrupt.

Okay this first part is a bit hard to understand, how about:

As the first frame in "repeat-mode" is the pattern, the first interrupt
is ignored by the driver. With small resolution bitstreams, the
interrupts might fire too quickly and thus the driver might miss the
second interrupt from the first actual frame.

Is that what you mean?

>
>In such case, driver can check whether the decoding is still ongoing,
>if not, just done the current decoding.

That doesn't answer to me why this solves the issue of missing the
second interrupt, can you elaborate your solution a bit so that the
reader of the commit description understands why this is needed?

Regards,
Sebastian

>
>Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>---
> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>index d579c804b047..adb93e977be9 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>@@ -89,6 +89,7 @@
> /* SLOT_STATUS fields for slots 0..3 */
> #define SLOT_STATUS_FRMDONE			(0x1 << 3)
> #define SLOT_STATUS_ENC_CONFIG_ERR		(0x1 << 8)
>+#define SLOT_STATUS_ONGOING			(0x1 << 31)
>
> /* SLOT_IRQ_EN fields TBD */
>
>diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>index 45705c606769..e6bb45633a19 100644
>--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>@@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no)
> 	return size;
> }
>
>+static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>+{
>+	struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>+	u32 curr_desc;
>+	u32 slot_status;
>+
>+	slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS));
>+	curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR));
>+
>+	if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>+		return true;
>+	if (slot_status & SLOT_STATUS_ONGOING)
>+		return true;
>+
>+	return false;
>+}
>+
> static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> {
> 	struct mxc_jpeg_dev *jpeg = priv;
>@@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 		mxc_jpeg_enc_mode_go(dev, reg, mxc_jpeg_is_extended_sequential(q_data->fmt));
> 		goto job_unlock;
> 	}
>-	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>+	if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>+	    mxc_dec_is_ongoing(ctx)) {
> 		jpeg_src_buf->dht_needed = false;
> 		dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
> 		goto job_unlock;
>-- 
>2.43.0-rc1
>
>

Re: [PATCH v2 3/3] media: imx-jpeg: Check decoding is ongoing for motion-jpeg

[Ming Qian(OSS)]

Hi Sebastian,

On 2025/4/5 23:37, Sebastian Fricke wrote:
> Hey Ming,
> 
> On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> To support decoding motion-jpeg without DHT, driver will try to decode a
>> pattern jpeg before actual jpeg frame by use of linked descriptors
>> (This is called "repeat mode"), then the DHT in the pattern jpeg can be
>> used for decoding the motion-jpeg.
> 
> Hmm do we need to repeat the description from the previous patch?

This issue is also caused by the repeat mode, I thought I should explain
it, but as you said, this does make it redundant.

>>
>> In other words, 2 frame done interrupts will be triggered, driver will
>> ignore the first interrupt, and wait for the second interrupt.
>> If the resolution is small, and the 2 interrupts may be too close,
>> when driver is handling the first interrupt, two frames are done, then
>> driver will fail to wait for the second interrupt.
> 
> Okay this first part is a bit hard to understand, how about:
> 
> As the first frame in "repeat-mode" is the pattern, the first interrupt
> is ignored by the driver. With small resolution bitstreams, the
> interrupts might fire too quickly and thus the driver might miss the
> second interrupt from the first actual frame.
> 
> Is that what you mean?
Yes, you're right, and it's better now.

> 
>>
>> In such case, driver can check whether the decoding is still ongoing,
>> if not, just done the current decoding.
> 
> That doesn't answer to me why this solves the issue of missing the
> second interrupt, can you elaborate your solution a bit so that the
> reader of the commit description understands why this is needed?
> 

When the first frame-done interrupt is received, we need to figure out
if we can get the second interrupt.
So we check the curr_desc register first,
if it is still pointing to the pattern descripor, the second actual
frame is not started, we can wait for its frame-doen interrupt.
if the curr_desc has pointed to the frame descriptor, then we check the
ongoing bit of slot_status register.
if the ongoing bit is set to 1, the decoding of the actual frame is not
finished, we can wait for its frame-doen interrupt.
if the ongoing bit is set o 0, the decoding of the actual frame is
finished, we can't wait for the second interrupt, but mark it as done.

But there is still a small problem, that the curr_desc and slot_status
registers are not synchronous. curr_desc is updated when the
next_descpt_ptr is loaded, but the ongoing bit of slot_status is set
after the 32 bytes descriptor is loaded, there will be a short time
interval in between, which may cause fake faluse. Reading slot_status
twice can basically reduce the probability of fake false to 0.

Regards,
Ming

> Regards,
> Sebastian
> 
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>> 2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> index d579c804b047..adb93e977be9 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> @@ -89,6 +89,7 @@
>> /* SLOT_STATUS fields for slots 0..3 */
>> #define SLOT_STATUS_FRMDONE            (0x1 << 3)
>> #define SLOT_STATUS_ENC_CONFIG_ERR        (0x1 << 8)
>> +#define SLOT_STATUS_ONGOING            (0x1 << 31)
>>
>> /* SLOT_IRQ_EN fields TBD */
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 45705c606769..e6bb45633a19 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct 
>> mxc_jpeg_q_data *q_data, u32 plane_no)
>>     return size;
>> }
>>
>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>> +{
>> +    struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> +    u32 curr_desc;
>> +    u32 slot_status;
>> +
>> +    slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
>> SLOT_STATUS));
>> +    curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
>> SLOT_CUR_DESCPT_PTR));
>> +
>> +    if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>> +        return true;
>> +    if (slot_status & SLOT_STATUS_ONGOING)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>> {
>>     struct mxc_jpeg_dev *jpeg = priv;
>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void 
>> *priv)
>>         mxc_jpeg_enc_mode_go(dev, reg, 
>> mxc_jpeg_is_extended_sequential(q_data->fmt));
>>         goto job_unlock;
>>     }
>> -    if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>> +    if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>> +        mxc_dec_is_ongoing(ctx)) {
>>         jpeg_src_buf->dht_needed = false;
>>         dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>         goto job_unlock;
>> -- 
>> 2.43.0-rc1
>>
>>