linux-kernel - Re: [PATCH] media: imx-jpeg: notify source chagne event when the first picture parsed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <46b3af72-d458-439d-9f2c-11a707acafa9@xs4all.nl>
Date:   Mon, 2 Oct 2023 12:35:31 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Ming Qian <ming.qian@....com>, mchehab@...nel.org,
        mirela.rabulea@....nxp.com
Cc:     shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
        festevam@...il.com, xiahong.bao@....com, eagle.zhou@....com,
        tao.jiang_2@....com, linux-imx@....com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: imx-jpeg: notify source chagne event when the
 first picture parsed

On 28/09/2023 07:37, Ming Qian wrote:
> After gstreamer rework the dynamic resolution change handling, gstreamer
> stop doing capture buffer allocation based on guesses and wait for the
> source change event when available. It requires driver always notify
> source change event in the initialization, even if the size parsed is
> equal to the size set on capture queue. otherwise, the pipeline will be
> stalled.
> 
> Currently driver may not notify source change event if the parsed format
> and size are equal to those previously established, but it may stall the
> gstreamer pipeline.
> 
> The link of gstreamer patch is
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4437
> 
> Fixes: b4e1fb8643da ("media: imx-jpeg: Support dynamic resolution change")
> Signed-off-by: Ming Qian <ming.qian@....com>
> ---
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 7 ++++++-
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 3af0af8ac07b..372f3007ff43 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -1348,7 +1348,8 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
>  	q_data_cap = mxc_jpeg_get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	if (mxc_jpeg_compare_format(q_data_cap->fmt, jpeg_src_buf->fmt))
>  		jpeg_src_buf->fmt = q_data_cap->fmt;
> -	if (q_data_cap->fmt != jpeg_src_buf->fmt ||
> +	if (!ctx->source_change_cnt ||
> +	    q_data_cap->fmt != jpeg_src_buf->fmt ||
>  	    q_data_cap->w != jpeg_src_buf->w ||
>  	    q_data_cap->h != jpeg_src_buf->h) {
>  		dev_dbg(dev, "Detected jpeg res=(%dx%d)->(%dx%d), pixfmt=%c%c%c%c\n",
> @@ -1392,6 +1393,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
>  		mxc_jpeg_sizeimage(q_data_cap);
>  		notify_src_chg(ctx);
>  		ctx->source_change = 1;
> +		ctx->source_change_cnt++;
>  		if (vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)))
>  			mxc_jpeg_set_last_buffer(ctx);
>  	}
> @@ -1611,6 +1613,9 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
>  	for (i = 0; i < *nplanes; i++)
>  		sizes[i] = mxc_jpeg_get_plane_size(q_data, i);
>  
> +	if (V4L2_TYPE_IS_OUTPUT(q->type))
> +		ctx->source_change_cnt = 0;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index d80e94cc9d99..b7e94fa50e02 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -99,6 +99,7 @@ struct mxc_jpeg_ctx {
>  	enum mxc_jpeg_enc_state		enc_state;
>  	int				slot;
>  	unsigned int			source_change;
> +	unsigned int			source_change_cnt;

This is a confusing field. It is not a counter at all, it is just a
bool to indicate if the initial source change event was raised or not.

So something like:

	bool need_initial_source_change_evt;

(feel free to give it a better name!)

It is certainly not a counter.

Regards,

	Hans

>  	bool				header_parsed;
>  	struct v4l2_ctrl_handler	ctrl_handler;
>  	u8				jpeg_quality;