From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A399C4332F for ; Thu, 21 Apr 2022 11:22:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1388803AbiDULYs (ORCPT ); Thu, 21 Apr 2022 07:24:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56434 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1388780AbiDULYm (ORCPT ); Thu, 21 Apr 2022 07:24:42 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EBCDA2BB35; Thu, 21 Apr 2022 04:21:51 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id A5A27B82431; Thu, 21 Apr 2022 11:21:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 68112C385A7; Thu, 21 Apr 2022 11:21:47 +0000 (UTC) Message-ID: <733c4775-777d-b0cd-83b6-934fecb1475e@xs4all.nl> Date: Thu, 21 Apr 2022 13:21:46 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality Content-Language: en-US To: Ming Qian , "Mirela Rabulea (OSS)" , "mchehab@kernel.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" Cc: "kernel@pengutronix.de" , "festevam@gmail.com" , dl-linux-imx , "linux-media@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" References: <20220406094623.7887-1-ming.qian@nxp.com> <80bcabe7-5761-7244-c6ea-1b5893395170@oss.nxp.com> From: Hans Verkuil In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/04/2022 12:04, Ming Qian wrote: >> From: Mirela Rabulea (OSS) >> Sent: Thursday, April 14, 2022 5:42 PM >> To: Ming Qian ; mchehab@kernel.org; >> shawnguo@kernel.org; s.hauer@pengutronix.de >> Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com; >> dl-linux-imx ; 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 >> >> 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 >>> --- >>> .../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 >>> + >>> + 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 { From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1466FC433EF for ; Thu, 21 Apr 2022 11:23:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Z98kpNQzNOaC8/LXUhFOBqdXLzqbi+XTUFy5nYmf3Sw=; b=dd8jNvgmr9p/ng 3oYuUhSsMsHdEcyTlEU5VJEKs3o2tykjt175ft2p46jj4XU+nknlCk9lL+VKHMH0pClsqaNDjZa52 L9YFdJn0F9SUSYaVRWBGIlUyPHZrIYWQCUejnMicFzF3yGOm/HrL8ZbD6dkVx/ZNlvMZ+cb4aPKAl jKu4oGMQMbKxQcNCPdRljOJUnfbQzeeFxJEKAHFAkbW9xO+xTXRCq3C8yUq602mT973oZTmIhC18R nEf11jrWjzWUVlUEGmVNjAjQfCn8kOrzIj+IEt/5cMpA/6zgepXaMwffkcfiO4gEYpRK+t4/4h3Xm 9HD6bbNYACpRhN+UNGUw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nhUsm-00DAwN-7z; Thu, 21 Apr 2022 11:21:56 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nhUsi-00DAvA-FE for linux-arm-kernel@lists.infradead.org; Thu, 21 Apr 2022 11:21:54 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C0B61B82434; Thu, 21 Apr 2022 11:21:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 68112C385A7; Thu, 21 Apr 2022 11:21:47 +0000 (UTC) Message-ID: <733c4775-777d-b0cd-83b6-934fecb1475e@xs4all.nl> Date: Thu, 21 Apr 2022 13:21:46 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH] media: imx-jpeg: Encoder support to set jpeg quality Content-Language: en-US To: Ming Qian , "Mirela Rabulea (OSS)" , "mchehab@kernel.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" Cc: "kernel@pengutronix.de" , "festevam@gmail.com" , dl-linux-imx , "linux-media@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" References: <20220406094623.7887-1-ming.qian@nxp.com> <80bcabe7-5761-7244-c6ea-1b5893395170@oss.nxp.com> From: Hans Verkuil In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220421_042152_826668_963F5FC4 X-CRM114-Status: GOOD ( 31.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 14/04/2022 12:04, Ming Qian wrote: >> From: Mirela Rabulea (OSS) >> Sent: Thursday, April 14, 2022 5:42 PM >> To: Ming Qian ; mchehab@kernel.org; >> shawnguo@kernel.org; s.hauer@pengutronix.de >> Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com; >> dl-linux-imx ; 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 >> >> 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 >>> --- >>> .../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 >>> + >>> + 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