Re: [PATCH v3 5/5] media: imx-jpeg: Support dynamic resolution change

From: Hans Verkuil
Date: Wed Apr 27 2022 - 03:03:23 EST

Next message: Juerg Haefliger: "[PATCH] crypto: inside-secure - Add MODULE_FIRMWARE macros"
Previous message: Ming Qian: "RE: [EXT] Re: [PATCH] media: amphion: ensure the buffer count is not less than min_buffer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 23/03/2022 10:05, Ming Qian wrote:
> To support dynamic resolution change,
> driver should meet the following conditions:
> 1. the previous pictures are all decoded before source change event.
> 2. prevent decoding new resolution pictures with incorrect capture
> buffer, until user handle source change event and setup capture.
> 3. report correct fmt and resolution during source change.
>
> Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> ---
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 69 ++++++++++++++-----
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.h | 2 +
> 2 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 30289f2be3dd..0c3a1efbeae7 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -933,13 +933,14 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
> {
> struct device *dev = ctx->mxc_jpeg->dev;
> struct mxc_jpeg_q_data *q_data_cap;
> - bool src_chg = false;
>
> if (!jpeg_src_buf->fmt)
> - return src_chg;
> + return false;
>
> q_data_cap = mxc_jpeg_get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> - if (q_data_cap->w != jpeg_src_buf->w || q_data_cap->h != jpeg_src_buf->h) {
> + if (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",
> q_data_cap->w, q_data_cap->h,
> jpeg_src_buf->w, jpeg_src_buf->h,
> @@ -976,9 +977,16 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
> mxc_jpeg_bytesperline(q_data_cap, jpeg_src_buf->fmt->precision);
> mxc_jpeg_sizeimage(q_data_cap);
> notify_src_chg(ctx);
> - src_chg = true;
> + ctx->source_change = 1;
> }
> - return src_chg;
> + return ctx->source_change ? true : false;
> +}
> +
> +static int mxc_jpeg_job_ready(void *priv)
> +{
> + struct mxc_jpeg_ctx *ctx = priv;
> +
> + return ctx->source_change ? 0 : 1;
> }
>
> static void mxc_jpeg_device_run(void *priv)
> @@ -1028,6 +1036,13 @@ static void mxc_jpeg_device_run(void *priv)
>
> return;
> }
> + if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE) {
> + if (ctx->source_change || mxc_jpeg_source_change(ctx, jpeg_src_buf)) {
> + spin_unlock_irqrestore(&ctx->mxc_jpeg->hw_lock, flags);
> + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> + return;
> + }
> + }
>
> mxc_jpeg_enable(reg);
> mxc_jpeg_set_l_endian(reg, 1);
> @@ -1074,6 +1089,7 @@ static void mxc_jpeg_set_last_buffer_dequeued(struct mxc_jpeg_ctx *ctx)
> q->last_buffer_dequeued = true;
> wake_up(&q->done_wq);
> ctx->stopped = 0;
> + ctx->header_parsed = false;
> }
>
> static int mxc_jpeg_decoder_cmd(struct file *file, void *priv,
> @@ -1167,6 +1183,8 @@ static int mxc_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
> struct mxc_jpeg_q_data *q_data = mxc_jpeg_get_q_data(ctx, q->type);
> int ret;
>
> + if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE && V4L2_TYPE_IS_CAPTURE(q->type))
> + ctx->source_change = 0;
> dev_dbg(ctx->mxc_jpeg->dev, "Start streaming ctx=%p", ctx);
> q_data->sequence = 0;
>
> @@ -1345,16 +1363,15 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
> dev_warn(dev, "Invalid user resolution 0x0");
> dev_warn(dev, "Keeping resolution from JPEG: %dx%d",
> header.frame.width, header.frame.height);
> - q_data_out->w = header.frame.width;
> - q_data_out->h = header.frame.height;
> } else if (header.frame.width != q_data_out->w ||
> header.frame.height != q_data_out->h) {
> dev_err(dev,
> "Resolution mismatch: %dx%d (JPEG) versus %dx%d(user)",
> header.frame.width, header.frame.height,
> q_data_out->w, q_data_out->h);
> - return -EINVAL;
> }
> + q_data_out->w = header.frame.width;
> + q_data_out->h = header.frame.height;
> if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> header.frame.width, header.frame.height);
> @@ -1390,8 +1407,10 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
> jpeg_src_buf->fmt = mxc_jpeg_find_format(ctx, fourcc);
> jpeg_src_buf->w = header.frame.width;
> jpeg_src_buf->h = header.frame.height;
> + ctx->header_parsed = true;
>
> - mxc_jpeg_source_change(ctx, jpeg_src_buf);
> + if (!v4l2_m2m_num_src_bufs_ready(ctx->fh.m2m_ctx))
> + mxc_jpeg_source_change(ctx, jpeg_src_buf);
>
> return 0;
> }
> @@ -1468,6 +1487,7 @@ static void mxc_jpeg_buf_finish(struct vb2_buffer *vb)
> if (list_empty(&q->done_list)) {
> vbuf->flags |= V4L2_BUF_FLAG_LAST;
> ctx->stopped = 0;
> + ctx->header_parsed = false;
> }
> }
>
> @@ -1613,26 +1633,42 @@ static int mxc_jpeg_enum_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_fmtdesc *f)
> {
> struct mxc_jpeg_ctx *ctx = mxc_jpeg_fh_to_ctx(priv);
> + struct mxc_jpeg_q_data *q_data = mxc_jpeg_get_q_data(ctx, f->type);
>
> - if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE)
> + if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE) {
> return enum_fmt(mxc_formats, MXC_JPEG_NUM_FORMATS, f,
> MXC_JPEG_FMT_TYPE_ENC);
> - else
> + } else if (!ctx->header_parsed) {
> return enum_fmt(mxc_formats, MXC_JPEG_NUM_FORMATS, f,
> MXC_JPEG_FMT_TYPE_RAW);
> + } else {
> + /* For the decoder CAPTURE queue, only enumerate the raw formats
> + * supported for the format currently active on OUTPUT
> + * (more precisely what was propagated on capture queue
> + * after jpeg parse on the output buffer)
> + */
> + if (f->index)
> + return -EINVAL;
> + f->pixelformat = q_data->fmt->fourcc;
> + strscpy(f->description, q_data->fmt->name, sizeof(f->description));

Don't fill the description field in struct v4l2_fmtdesc. The core will fill
this for you, ensuring consistent format description names.

I've seen this in enum_fmt() and in mxc_jpeg_enum_fmt_vid_cap(), so both
functions should be adapted. This can be done in a new patch since it is
unrelated to the changes of this patch series.

Just something I noticed :-)

Regards,

Hans

> + return 0;
> + }
> }
>
> static int mxc_jpeg_enum_fmt_vid_out(struct file *file, void *priv,
> struct v4l2_fmtdesc *f)
> {
> struct mxc_jpeg_ctx *ctx = mxc_jpeg_fh_to_ctx(priv);
> + u32 type = ctx->mxc_jpeg->mode == MXC_JPEG_DECODE ? MXC_JPEG_FMT_TYPE_ENC :
> + MXC_JPEG_FMT_TYPE_RAW;
> + int ret;
>
> + ret = enum_fmt(mxc_formats, MXC_JPEG_NUM_FORMATS, f, type);
> + if (ret)
> + return ret;
> if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE)
> - return enum_fmt(mxc_formats, MXC_JPEG_NUM_FORMATS, f,
> - MXC_JPEG_FMT_TYPE_ENC);
> - else
> - return enum_fmt(mxc_formats, MXC_JPEG_NUM_FORMATS, f,
> - MXC_JPEG_FMT_TYPE_RAW);
> + f->flags = V4L2_FMT_FLAG_DYN_RESOLUTION;
> + return 0;
> }
>
> static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fmt,
> @@ -2018,6 +2054,7 @@ static const struct v4l2_file_operations mxc_jpeg_fops = {
> };
>
> static const struct v4l2_m2m_ops mxc_jpeg_m2m_ops = {
> + .job_ready = mxc_jpeg_job_ready,
> .device_run = mxc_jpeg_device_run,
> };
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 82b38cc2dfab..9ae56e6e0fbe 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -94,6 +94,8 @@ struct mxc_jpeg_ctx {
> unsigned int stopping;
> unsigned int stopped;
> unsigned int slot;
> + unsigned int source_change;
> + bool header_parsed;
> };
>
> struct mxc_jpeg_slot_data {

Next message: Juerg Haefliger: "[PATCH] crypto: inside-secure - Add MODULE_FIRMWARE macros"
Previous message: Ming Qian: "RE: [EXT] Re: [PATCH] media: amphion: ensure the buffer count is not less than min_buffer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]