From: "mirela.rabulea@oss.nxp.com" <mirela.rabulea@oss.nxp.com> To: Ming Qian <ming.qian@nxp.com>, mchehab@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de Cc: hverkuil-cisco@xs4all.nl, kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality Date: Thu, 14 Apr 2022 12:42:06 +0300 [thread overview] Message-ID: <80bcabe7-5761-7244-c6ea-1b5893395170@oss.nxp.com> (raw) In-Reply-To: <20220406094623.7887-1-ming.qian@nxp.com> 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@nxp.com> > --- > .../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"); > 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. > + 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. > +} > + > +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. > + > + if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE) > + mxc_jpeg_encode_ctrls(ctx); > + > + 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 {
WARNING: multiple messages have this Message-ID (diff)
From: "mirela.rabulea@oss.nxp.com" <mirela.rabulea@oss.nxp.com> To: Ming Qian <ming.qian@nxp.com>, mchehab@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de Cc: hverkuil-cisco@xs4all.nl, kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality Date: Thu, 14 Apr 2022 12:42:06 +0300 [thread overview] Message-ID: <80bcabe7-5761-7244-c6ea-1b5893395170@oss.nxp.com> (raw) In-Reply-To: <20220406094623.7887-1-ming.qian@nxp.com> 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@nxp.com> > --- > .../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"); > 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. > + 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. > +} > + > +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. > + > + if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE) > + mxc_jpeg_encode_ctrls(ctx); > + > + 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 { _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-04-14 9:42 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-06 9:46 [PATCH] media: imx-jpeg: Encoder support to set jpeg quality Ming Qian 2022-04-06 9:46 ` Ming Qian 2022-04-14 9:42 ` mirela.rabulea [this message] 2022-04-14 9:42 ` mirela.rabulea 2022-04-14 10:04 ` Ming Qian 2022-04-14 10:04 ` Ming Qian 2022-04-21 11:21 ` Hans Verkuil 2022-04-21 11:21 ` Hans Verkuil 2022-04-21 11:42 ` [EXT] " Ming Qian 2022-04-21 11:42 ` Ming Qian
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=80bcabe7-5761-7244-c6ea-1b5893395170@oss.nxp.com \ --to=mirela.rabulea@oss.nxp.com \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=hverkuil-cisco@xs4all.nl \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=ming.qian@nxp.com \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.