RE: [EXT] Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Hans Verkuil [mailto:hverkuil-cisco@xxxxxxxxx]
> Sent: Thursday, April 21, 2022 7:22 PM
> To: Ming Qian <ming.qian@xxxxxxx>; Mirela Rabulea (OSS)
> <mirela.rabulea@xxxxxxxxxxx>; mchehab@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx
> Cc: kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality
> 
> Caution: EXT Email
> 
> On 14/04/2022 12:04, Ming Qian wrote:
> >> From: Mirela Rabulea (OSS)
> >> Sent: Thursday, April 14, 2022 5:42 PM
> >> To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
> >> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx
> >> Cc: hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> >> festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> >> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg
> >> quality
> >>
> >> Hi Ming,
> >>
> >> On 06.04.2022 12:46, Ming Qian wrote:
> >>> Implement V4L2_CID_JPEG_COMPRESSION_QUALITY to set jpeg quality
> >>>
> >>> Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> >>> ---
> >>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 11 ++--
> >>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> >>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 50
> >> +++++++++++++++++++
> >>>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  2 +
> >>>   4 files changed, 61 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> >>> index 29c604b1b179..c482228262a3 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> >>> @@ -100,9 +100,6 @@ void mxc_jpeg_enc_mode_conf(struct device
> *dev,
> >>> void __iomem *reg)
> >>>
> >>>     /* all markers and segments */
> >>>     writel(0x3ff, reg + CAST_CFG_MODE);
> >>> -
> >>> -   /* quality factor */
> >>> -   writel(0x4b, reg + CAST_QUALITY);
> >>>   }
> >>>
> >>>   void mxc_jpeg_enc_mode_go(struct device *dev, void __iomem *reg)
> >>> @@
> >>> -114,6 +111,14 @@ void mxc_jpeg_enc_mode_go(struct device *dev, void
> >> __iomem *reg)
> >>>     writel(0x140, reg + CAST_MODE);
> >>>   }
> >>>
> >>> +void mxc_jpeg_enc_set_quality(struct device *dev, void __iomem
> >>> +*reg,
> >>> +u8 quality) {
> >>> +   dev_dbg(dev, "CAST Encoder Quality %d...\n", quality);
> >>> +
> >>> +   /* quality factor */
> >>> +   writel(quality, reg + CAST_QUALITY); }
> >>> +
> >>>   void mxc_jpeg_dec_mode_go(struct device *dev, void __iomem *reg)
> >>>   {
> >>>     dev_dbg(dev, "CAST Decoder GO...\n"); 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 ae70d3a0dc24..356e40140987 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> >>> @@ -119,6 +119,7 @@ int mxc_jpeg_enable(void __iomem *reg);
> >>>   void wait_frmdone(struct device *dev, void __iomem *reg);
> >>>   void mxc_jpeg_enc_mode_conf(struct device *dev, void __iomem *reg);
> >>>   void mxc_jpeg_enc_mode_go(struct device *dev, void __iomem *reg);
> >>> +void mxc_jpeg_enc_set_quality(struct device *dev, void __iomem
> >>> +*reg,
> >>> +u8 quality);
> >>>   void mxc_jpeg_dec_mode_go(struct device *dev, void __iomem *reg);
> >>>   int mxc_jpeg_get_slot(void __iomem *reg);
> >>>   u32 mxc_jpeg_get_offset(void __iomem *reg, int slot); diff --git
> >>> a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> index 0c3a1efbeae7..ccc26372e178 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> @@ -624,6 +624,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq,
> >>> void
> >> *priv)
> >>>         ctx->enc_state == MXC_JPEG_ENC_CONF) {
> >>>             ctx->enc_state = MXC_JPEG_ENCODING;
> >>>             dev_dbg(dev, "Encoder config finished. Start
> >>> encoding...\n");
> >>> +           mxc_jpeg_enc_set_quality(dev, reg, ctx->jpeg_quality);
> >>
> >> I think the setting of the quality should be moved in device_run, to
> >> keep the interrupt handler lean, I checked it works fine after:
> >> dev_dbg(dev, "Encoder config finished. Start encoding...\n");
> >>
> >
> > Considering the multi-slot situation, the quality register is a global register
> for all slots.
> > So to avoid access it in the same time by different slots. It's safe to set after
> configure done but before encode.
> > And we only support yet, but I think we will support multi slots after we fix
> some issues.
> >
> >
> >>>             mxc_jpeg_enc_mode_go(dev, reg);
> >>>             goto job_unlock;
> >>>     }
> >>> @@ -1563,6 +1564,44 @@ static void
> >>> mxc_jpeg_set_default_params(struct
> >> mxc_jpeg_ctx *ctx)
> >>>     }
> >>>   }
> >>>
> >>> +static int mxc_jpeg_s_ctrl(struct v4l2_ctrl *ctrl) {
> >>> +   struct mxc_jpeg_ctx *ctx =
> >>> +           container_of(ctrl->handler, struct mxc_jpeg_ctx,
> >>> +ctrl_handler);
> >>> +
> >>> +   switch (ctrl->id) {
> >>> +   case V4L2_CID_JPEG_COMPRESSION_QUALITY:
> >>
> >> Looks like this is allowed for decoder, which is not ok, maybe return
> >> -EINVAL for decoder.
> >>
> >
> > This control is created for encoder only, so decoder has no chance to
> > execute here
> >
> >>> +           ctx->jpeg_quality = ctrl->val;
> >>> +           break;
> >>> +   default:
> >>> +           dev_err(ctx->mxc_jpeg->dev, "Invalid control, id = %d, val
> = %d\n",
> >>> +                   ctrl->id, ctrl->val);
> >>> +           return -EINVAL;
> >>> +   }
> >>> +
> >>> +   return 0;
> >>> +}
> >>> +
> >>> +static const struct v4l2_ctrl_ops mxc_jpeg_ctrl_ops = {
> >>> +   .s_ctrl = mxc_jpeg_s_ctrl,
> >>> +};
> >>> +
> >>> +static void mxc_jpeg_encode_ctrls(struct mxc_jpeg_ctx *ctx) {
> >>> +   v4l2_ctrl_new_std(&ctx->ctrl_handler, &mxc_jpeg_ctrl_ops,
> >>> +                     V4L2_CID_JPEG_COMPRESSION_QUALITY, 1, 100,
> 1,
> >>> +75);
> >>
> >> The v4l2_ctrl_new_std may return an error, which is not checked here
> >> (NULL is returned and @hdl->error is set...), please fix.
> >>
> >
> > Almost no driver check the return value of v4l2_ctrl_new_std. except some
> driver want to change some property of the created ctrl.
> > And if it return NULL, it won't bring some serious problems, just not
> > support this control
> 
> The typical behavior is to add all controls, then at the end check if
> hdl->error was set, and if so, v4l2_ctrl_handler_free is called and
> the error is returned.
> 
> >
> >>> +}
> >>> +
> >>> +static int mxc_jpeg_ctrls_setup(struct mxc_jpeg_ctx *ctx) {
> >>> +   v4l2_ctrl_handler_init(&ctx->ctrl_handler, 2);
> >>
> >> ctrl_handler has a lock member, which could be setup here.
> >>
> >
> > The lock will be set in v4l2_ctrl_handler_init:
> > mutex_init(&hdl->_lock);
> > hdl->lock = &hdl->_lock;
> >
> >>> +
> >>> +   if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE)
> >>> +           mxc_jpeg_encode_ctrls(ctx);
> 
> So:
> 
>         if (&ctx->ctrl_handler.error) {
>                 int err = ctx->ctrl_handler.error;
>                 v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>                 return err;
>         }
> 
> Regards,
> 
>         Hans
> 

Got it, I'll fix it in V2

> >>> +
> >>> +   return v4l2_ctrl_handler_setup(&ctx->ctrl_handler);
> >>> +}
> >>> +
> >>>   static int mxc_jpeg_open(struct file *file)
> >>>   {
> >>>     struct mxc_jpeg_dev *mxc_jpeg = video_drvdata(file); @@ -1594,6
> >>> +1633,12 @@ static int mxc_jpeg_open(struct file *file)
> >>>             goto error;
> >>>     }
> >>>
> >>> +   ret = mxc_jpeg_ctrls_setup(ctx);
> >>> +   if (ret) {
> >>> +           dev_err(ctx->mxc_jpeg->dev, "failed to setup mxc jpeg
> controls\n");
> >>> +           goto err_ctrls_setup;
> >>> +   }
> >>> +   ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> >>>     mxc_jpeg_set_default_params(ctx);
> >>>     ctx->slot = MXC_MAX_SLOTS; /* slot not allocated yet */
> >>>
> >>> @@ -1605,6 +1650,8 @@ static int mxc_jpeg_open(struct file *file)
> >>>
> >>>     return 0;
> >>>
> >>> +err_ctrls_setup:
> >>> +   v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> >>>   error:
> >>>     v4l2_fh_del(&ctx->fh);
> >>>     v4l2_fh_exit(&ctx->fh);
> >>> @@ -1962,6 +2009,8 @@ static int mxc_jpeg_subscribe_event(struct
> >> v4l2_fh *fh,
> >>>             return v4l2_event_subscribe(fh, sub, 0, NULL);
> >>>     case V4L2_EVENT_SOURCE_CHANGE:
> >>>             return v4l2_src_change_event_subscribe(fh, sub);
> >>> +   case V4L2_EVENT_CTRL:
> >>> +           return v4l2_ctrl_subscribe_event(fh, sub);
> >>>     default:
> >>>             return -EINVAL;
> >>>     }
> >>> @@ -2035,6 +2084,7 @@ static int mxc_jpeg_release(struct file *file)
> >>>     else
> >>>             dev_dbg(dev, "Release JPEG encoder instance on slot %d.",
> >>>                     ctx->slot);
> >>> +   v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> >>>     v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> >>>     v4l2_fh_del(&ctx->fh);
> >>>     v4l2_fh_exit(&ctx->fh);
> >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> >>> index 9ae56e6e0fbe..9c9da32b2125 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> >>> @@ -96,6 +96,8 @@ struct mxc_jpeg_ctx {
> >>>     unsigned int                    slot;
> >>>     unsigned int                    source_change;
> >>>     bool                            header_parsed;
> >>> +   struct v4l2_ctrl_handler        ctrl_handler;
> >>> +   u8                              jpeg_quality;
> >>>   };
> >>>
> >>>   struct mxc_jpeg_slot_data {





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux