> 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 {
