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

When an output buffer contains error frame header,
v4l2_jpeg_parse_header() will return error, then driver will mark this
buffer and a capture buffer done with error flag in device_run().

But if the error occurs in the first frames, before setup the capture
queue, there is no chance to schedule device_run(), and there may be no
capture to mark error.

So we need to drop this buffer with error flag, and make the decoding
can continue.

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 1221b309a916..0e6ee997284b 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -1921,6 +1921,15 @@ static void mxc_jpeg_buf_queue(struct vb2_buffer *vb)
 	if (ret)
 		jpeg_src_buf->jpeg_parse_error = true;
 
+	/*
+	 * if the capture queue is not setup, the device_run() won't be scheduled,
+	 * need to drop the error buffer, so that the decoding can continue
+	 */
+	if (jpeg_src_buf->jpeg_parse_error &&
+	    !vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) {
+		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+		return;
+	}
 end:
 	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
 }
-- 
2.43.0-rc1

Le mercredi 05 mars 2025 à 10:25 +0800, ming.qian@oss.nxp.com a écrit :
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> When an output buffer contains error frame header,
> v4l2_jpeg_parse_header() will return error, then driver will mark this
> buffer and a capture buffer done with error flag in device_run().
> 
> But if the error occurs in the first frames, before setup the capture
> queue, there is no chance to schedule device_run(), and there may be no
> capture to mark error.
> 
> So we need to drop this buffer with error flag, and make the decoding
> can continue.
> 
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

This seems like a relatively important bug fix to be, perhaps you can
offer "Fixes" tag to this commit ?

> ---
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 1221b309a916..0e6ee997284b 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -1921,6 +1921,15 @@ static void mxc_jpeg_buf_queue(struct vb2_buffer *vb)
>  	if (ret)
>  		jpeg_src_buf->jpeg_parse_error = true;
>  
> +	/*
> +	 * if the capture queue is not setup, the device_run() won't be scheduled,
> +	 * need to drop the error buffer, so that the decoding can continue
> +	 */
> +	if (jpeg_src_buf->jpeg_parse_error &&
> +	    !vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) {
> +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +		return;
> +	}

Looks good to me, an alternative implementation could have been to
merged into the error branch above.

 	if (ret) {
 		jpeg_src_buf->jpeg_parse_error = true;
 
		/*
	 	 * If the capture queue is not setup, the device_run() won't
		 * be scheduled, drop the error buffer so that the decoding
		 * can continue.
	 	 */
		if (!vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) {
			v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
			return;
		}
	}

With or without this suggestion, but with the Fixes tag, you can include in your v2:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

>  end:
>  	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
>  }

Hi Nicolas,

On 2025/4/18 21:12, Nicolas Dufresne wrote:
> Le mercredi 05 mars 2025 à 10:25 +0800, ming.qian@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> When an output buffer contains error frame header,
>> v4l2_jpeg_parse_header() will return error, then driver will mark this
>> buffer and a capture buffer done with error flag in device_run().
>>
>> But if the error occurs in the first frames, before setup the capture
>> queue, there is no chance to schedule device_run(), and there may be no
>> capture to mark error.
>>
>> So we need to drop this buffer with error flag, and make the decoding
>> can continue.
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> 
> This seems like a relatively important bug fix to be, perhaps you can
> offer "Fixes" tag to this commit ?
> 

thanks for the reminder

>> ---
>>   drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 1221b309a916..0e6ee997284b 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -1921,6 +1921,15 @@ static void mxc_jpeg_buf_queue(struct vb2_buffer *vb)
>>   	if (ret)
>>   		jpeg_src_buf->jpeg_parse_error = true;
>>   
>> +	/*
>> +	 * if the capture queue is not setup, the device_run() won't be scheduled,
>> +	 * need to drop the error buffer, so that the decoding can continue
>> +	 */
>> +	if (jpeg_src_buf->jpeg_parse_error &&
>> +	    !vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) {
>> +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>> +		return;
>> +	}
> 
> Looks good to me, an alternative implementation could have been to
> merged into the error branch above.
> 
>   	if (ret) {
>   		jpeg_src_buf->jpeg_parse_error = true;
>   
> 		/*
> 	 	 * If the capture queue is not setup, the device_run() won't
> 		 * be scheduled, drop the error buffer so that the decoding
> 		 * can continue.
> 	 	 */
> 		if (!vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) {
> 			v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> 			return;
> 		}
> 	}
> 
> With or without this suggestion, but with the Fixes tag, you can include in your v2:
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 

I'll apply your suggestion and with the Fixes tag in V2

Regards,
Ming

>>   end:
>>   	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
>>   }