Message ID | 20230131192016.3476937-8-dave.stevenson@raspberrypi.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pMwC1-00DDPa-PZ; Tue, 31 Jan 2023 19:21:22 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231454AbjAaTVU (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 31 Jan 2023 14:21:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37580 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230519AbjAaTVK (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 31 Jan 2023 14:21:10 -0500 Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4E4D459E59 for <linux-media@vger.kernel.org>; Tue, 31 Jan 2023 11:21:01 -0800 (PST) Received: by mail-wm1-x32f.google.com with SMTP id j29-20020a05600c1c1d00b003dc52fed235so5888021wms.1 for <linux-media@vger.kernel.org>; Tue, 31 Jan 2023 11:21:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=WH2m6BhNR1uDXlk7bsxUU9DHWcuEsawDHjnKOH5ul0Q=; b=pzyU8Gc+Cr1YndkNKnbwJBp7PInVg9cNURZvI+svHboxz6aOAUih7YT1xIQlCnWIYJ 9HJg5347CqK787gl8UjVrjh4hTFgMXYw95nE1wO36eF05pYoxCHKPaTIxN8bfJiqKJm5 9F/Ejbg4Q5GbcCrx73vqXr2hDY/K9ELH0j9cveb8F19qfOAkXbwNv7UNmVosLuhMmo6U 3fjjssQv2JwaoL0FJevzQ8WW2IrXy1+YUU6pIdV9K6p5TXwpEtaObogqTh23AF53kqEc 94hILru1e+RaZDPxmWMo1clo0XWnXlWPQ5ib/L5li8AkTsBHhvM6UiePeK3Vtw4p6Xlx yx6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WH2m6BhNR1uDXlk7bsxUU9DHWcuEsawDHjnKOH5ul0Q=; b=YfHjzdj84Gzlfyx3tnm12NNYiaDoKEIRue66OBs84K59ccrxhwevDiaVqzaxWON1CF Xv0sl8yJSaASKtsx032U+oG0//3+WP9Nr8AVfKcVl0bd4vVbskwExZBKgsWnch3Nn0OT l6lXUxvtFkSNcXpmoHgA9wR1J1/ygM6yT+Gp/4TJ3Xx39qOcaLJjntz9Dxf8pNUSROVE gvnXuEixyx2vAI2cFF7IEhgRbS3rcpQERcSwXhkTB3f2R3XuOY2gdakRn7y7jqP/NJxO DFjy3ZadqETsQzm6rOwwIGsKoJqP6soGE1mH4u5ZAEdR+beDt8g93TV6q/uUdvQy9fy0 Z7cA== X-Gm-Message-State: AO0yUKViDFBupH9DLS/PZrjMPqAJwjKrHeJk07gNpgX5rP0EqyloFaeN tLo2CQbB88PcT5X6oV2thdbogsZ6AZ8IhZJF X-Google-Smtp-Source: AK7set9q5zpQFp5aZC9veoh9czoaoB24cRbCPPmlXpJGCnH8ur6+I4XwGFTbHLY0TSmLDaHLnh7rXA== X-Received: by 2002:a05:600c:705:b0:3cf:85f7:bbc4 with SMTP id i5-20020a05600c070500b003cf85f7bbc4mr720561wmn.2.1675192859715; Tue, 31 Jan 2023 11:20:59 -0800 (PST) Received: from dave-Ubuntu2204.. (194.15.169.217.in-addr.arpa. [217.169.15.194]) by smtp.googlemail.com with ESMTPSA id x9-20020a05600c21c900b003dc434b39c7sm3527512wmj.0.2023.01.31.11.20.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Jan 2023 11:20:58 -0800 (PST) From: Dave Stevenson <dave.stevenson@raspberrypi.com> To: Manivannan Sadhasivam <mani@kernel.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, linux-media@vger.kernel.org Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> Subject: [PATCH 07/11] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Date: Tue, 31 Jan 2023 19:20:12 +0000 Message-Id: <20230131192016.3476937-8-dave.stevenson@raspberrypi.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230131192016.3476937-1-dave.stevenson@raspberrypi.com> References: <20230131192016.3476937-1-dave.stevenson@raspberrypi.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
imx290: Minor fixes, support for alternate INCK, and more ctrls
|
|
Commit Message
Dave Stevenson
Jan. 31, 2023, 7:20 p.m. UTC
The driver exposed V4L2_CID_HBLANK as a read only control to allow
for exposure calculations and determination of the frame rate.
Convert to a read/write control so that the frame rate can be
controlled.
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
Comments
Hi Dave, Thank you for the patch. On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote: > The driver exposed V4L2_CID_HBLANK as a read only control to allow > for exposure calculations and determination of the frame rate. > > Convert to a read/write control so that the frame rate can be > controlled. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > 1 file changed, 19 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index 9ddd6382b127..9006be6e5e7c 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -47,6 +47,7 @@ > #define IMX290_GAIN IMX290_REG_8BIT(0x3014) > #define IMX290_VMAX IMX290_REG_24BIT(0x3018) > #define IMX290_HMAX IMX290_REG_16BIT(0x301c) > +#define IMX290_HMAX_MAX 0xffff > #define IMX290_SHS1 IMX290_REG_24BIT(0x3020) > #define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) > #define IMX290_WINPV IMX290_REG_16BIT(0x303c) > @@ -167,7 +168,7 @@ struct imx290_regval { > struct imx290_mode { > u32 width; > u32 height; > - u32 hmax; > + u32 hmax_min; > u8 link_freq_index; > > const struct imx290_regval *data; > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > { > .width = 1920, > .height = 1080, > - .hmax = 2200, > + .hmax_min = 2200, I didn't find in the documentation a mention that these values are the minimum the sensor supports. Did you use them as such based on the fact that anything higher than the nominal hblank value reported by the datasheet is hopefully guaranteed to work, and lower values are uncharted territory, or are these the real minimums ? I'm fine using them as minimum for now even if we could possibly go lower, leaving that for future patches. I would however mention this in a comment or in the commit message. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > .link_freq_index = FREQ_INDEX_1080P, > .data = imx290_1080p_settings, > .data_size = ARRAY_SIZE(imx290_1080p_settings), > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > { > .width = 1280, > .height = 720, > - .hmax = 3300, > + .hmax_min = 3300, > .link_freq_index = FREQ_INDEX_720P, > .data = imx290_720p_settings, > .data_size = ARRAY_SIZE(imx290_720p_settings), > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > { > .width = 1920, > .height = 1080, > - .hmax = 2200, > + .hmax_min = 2200, > .link_freq_index = FREQ_INDEX_1080P, > .data = imx290_1080p_settings, > .data_size = ARRAY_SIZE(imx290_1080p_settings), > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > { > .width = 1280, > .height = 720, > - .hmax = 3300, > + .hmax_min = 3300, > .link_freq_index = FREQ_INDEX_720P, > .data = imx290_720p_settings, > .data_size = ARRAY_SIZE(imx290_720p_settings), > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) > } > break; > > + case V4L2_CID_HBLANK: > + ret = imx290_write(imx290, IMX290_HMAX, > + ctrl->val + imx290->current_mode->width, > + NULL); > + break; > + > default: > ret = -EINVAL; > break; > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, > const struct v4l2_mbus_framefmt *format, > const struct imx290_mode *mode) > { > - unsigned int hblank = mode->hmax - mode->width; > + unsigned int hblank_min = mode->hmax_min - mode->width; > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, > + hblank_min); > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); > } > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > 0, 0, imx290_test_pattern_menu); > > + /* > + * Actual range will be set from imx290_ctrl_update later in the probe. > + */ > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > V4L2_CID_HBLANK, 1, 1, 1, 1); > - if (imx290->hblank) > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > V4L2_CID_VBLANK, 1, 1, 1, 1); > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290, > return ret; > } > > - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, > - NULL); > - if (ret) > - return ret; > - > /* Apply customized values from user */ > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > if (ret) {
Hi Laurent On Thu, 2 Feb 2023 at 14:58, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > Thank you for the patch. > > On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote: > > The driver exposed V4L2_CID_HBLANK as a read only control to allow > > for exposure calculations and determination of the frame rate. > > > > Convert to a read/write control so that the frame rate can be > > controlled. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > --- > > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index 9ddd6382b127..9006be6e5e7c 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -47,6 +47,7 @@ > > #define IMX290_GAIN IMX290_REG_8BIT(0x3014) > > #define IMX290_VMAX IMX290_REG_24BIT(0x3018) > > #define IMX290_HMAX IMX290_REG_16BIT(0x301c) > > +#define IMX290_HMAX_MAX 0xffff > > #define IMX290_SHS1 IMX290_REG_24BIT(0x3020) > > #define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) > > #define IMX290_WINPV IMX290_REG_16BIT(0x303c) > > @@ -167,7 +168,7 @@ struct imx290_regval { > > struct imx290_mode { > > u32 width; > > u32 height; > > - u32 hmax; > > + u32 hmax_min; > > u8 link_freq_index; > > > > const struct imx290_regval *data; > > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > > { > > .width = 1920, > > .height = 1080, > > - .hmax = 2200, > > + .hmax_min = 2200, > > I didn't find in the documentation a mention that these values are the > minimum the sensor supports. Did you use them as such based on the fact > that anything higher than the nominal hblank value reported by the > datasheet is hopefully guaranteed to work, and lower values are > uncharted territory, or are these the real minimums ? I'm fine using > them as minimum for now even if we could possibly go lower, leaving that > for future patches. I would however mention this in a comment or in the > commit message. The datasheet shows varying HMAX to control frame rate. Table "List of Setting Register for CSI-2 serial output" for all-pixel mode gives HMAX values of : 0x14a0 for 25fps 0x1130 for 30fps 0x0a50 for 50fps 0x0898 for 60 fps 0x0528 for 100fps (needs FRSEL = 0 and higher link frequency) 0x044c for 120fps (ditto) VMAX is fixed at 0x465 (1125 decimal) for all frame rates in all-pixel (1080p) mode. The 2200 listed here is the 0x898 for 60fps - the nominal max with the configured lane / link frequency combinations. Doing the maths, 4 lanes at 445.5Mbit/s/lane = 1782Mbit/s. 2200 * 1125 * 12bpp * 60fps = 1782Mbit/s. Pixel clock is therefore the same as link bandwidth at this point. I have no information about blanking and clock lane behaviour for this sensor, but can believe that it needs that time for low level CSI2 transitions. AIUI you'll only be able to decrease this further if you add support for 891Mbit/s/lane (445.5MHz link freq) on 4 lanes, and drop to 10 bit readout. Programming HMAX also then becomes more entertaining as it appears to be half the expected value (0x44c = 1100 decimal, which is smaller than the width), so it'll need a fair amount of messing to get all the controls to behave as expected. That's all outside the scope of this patch set - 60fps was the only frame rate previously supported, and we've expanded on that with slower frame rate support. FWIW I have checked with Sony over the 120fps modes, and experimented a little. Sony will only support 120fps in 10bit. The flyer for IMX462 that implies it does 1920x1080@120fps in 12 bit mode is incorrect. Experimentation had 720p120 working in 12bit, but trying to work through limiting the various bit depth and resolution options is going to get really ugly in the driver. Dave > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > .link_freq_index = FREQ_INDEX_1080P, > > .data = imx290_1080p_settings, > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > > { > > .width = 1280, > > .height = 720, > > - .hmax = 3300, > > + .hmax_min = 3300, > > .link_freq_index = FREQ_INDEX_720P, > > .data = imx290_720p_settings, > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > > { > > .width = 1920, > > .height = 1080, > > - .hmax = 2200, > > + .hmax_min = 2200, > > .link_freq_index = FREQ_INDEX_1080P, > > .data = imx290_1080p_settings, > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > > { > > .width = 1280, > > .height = 720, > > - .hmax = 3300, > > + .hmax_min = 3300, > > .link_freq_index = FREQ_INDEX_720P, > > .data = imx290_720p_settings, > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) > > } > > break; > > > > + case V4L2_CID_HBLANK: > > + ret = imx290_write(imx290, IMX290_HMAX, > > + ctrl->val + imx290->current_mode->width, > > + NULL); > > + break; > > + > > default: > > ret = -EINVAL; > > break; > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, > > const struct v4l2_mbus_framefmt *format, > > const struct imx290_mode *mode) > > { > > - unsigned int hblank = mode->hmax - mode->width; > > + unsigned int hblank_min = mode->hmax_min - mode->width; > > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); > > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, > > + hblank_min); > > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); > > } > > > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > > 0, 0, imx290_test_pattern_menu); > > > > + /* > > + * Actual range will be set from imx290_ctrl_update later in the probe. > > + */ > > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > V4L2_CID_HBLANK, 1, 1, 1, 1); > > - if (imx290->hblank) > > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > V4L2_CID_VBLANK, 1, 1, 1, 1); > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290, > > return ret; > > } > > > > - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, > > - NULL); > > - if (ret) > > - return ret; > > - > > /* Apply customized values from user */ > > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > > if (ret) { > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Thu, Feb 02, 2023 at 03:48:19PM +0000, Dave Stevenson wrote: > On Thu, 2 Feb 2023 at 14:58, Laurent Pinchart wrote: > > On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote: > > > The driver exposed V4L2_CID_HBLANK as a read only control to allow > > > for exposure calculations and determination of the frame rate. > > > > > > Convert to a read/write control so that the frame rate can be > > > controlled. > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > index 9ddd6382b127..9006be6e5e7c 100644 > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -47,6 +47,7 @@ > > > #define IMX290_GAIN IMX290_REG_8BIT(0x3014) > > > #define IMX290_VMAX IMX290_REG_24BIT(0x3018) > > > #define IMX290_HMAX IMX290_REG_16BIT(0x301c) > > > +#define IMX290_HMAX_MAX 0xffff > > > #define IMX290_SHS1 IMX290_REG_24BIT(0x3020) > > > #define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) > > > #define IMX290_WINPV IMX290_REG_16BIT(0x303c) > > > @@ -167,7 +168,7 @@ struct imx290_regval { > > > struct imx290_mode { > > > u32 width; > > > u32 height; > > > - u32 hmax; > > > + u32 hmax_min; > > > u8 link_freq_index; > > > > > > const struct imx290_regval *data; > > > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > > > { > > > .width = 1920, > > > .height = 1080, > > > - .hmax = 2200, > > > + .hmax_min = 2200, > > > > I didn't find in the documentation a mention that these values are the > > minimum the sensor supports. Did you use them as such based on the fact > > that anything higher than the nominal hblank value reported by the > > datasheet is hopefully guaranteed to work, and lower values are > > uncharted territory, or are these the real minimums ? I'm fine using > > them as minimum for now even if we could possibly go lower, leaving that > > for future patches. I would however mention this in a comment or in the > > commit message. > > The datasheet shows varying HMAX to control frame rate. > Table "List of Setting Register for CSI-2 serial output" for all-pixel > mode gives HMAX values of : > 0x14a0 for 25fps > 0x1130 for 30fps > 0x0a50 for 50fps > 0x0898 for 60 fps > 0x0528 for 100fps (needs FRSEL = 0 and higher link frequency) > 0x044c for 120fps (ditto) > > VMAX is fixed at 0x465 (1125 decimal) for all frame rates in all-pixel > (1080p) mode. > > The 2200 listed here is the 0x898 for 60fps - the nominal max with the > configured lane / link frequency combinations. > > Doing the maths, 4 lanes at 445.5Mbit/s/lane = 1782Mbit/s. > 2200 * 1125 * 12bpp * 60fps = 1782Mbit/s. Pixel clock is therefore the > same as link bandwidth at this point. I have no information about > blanking and clock lane behaviour for this sensor, but can believe > that it needs that time for low level CSI2 transitions. > > AIUI you'll only be able to decrease this further if you add support > for 891Mbit/s/lane (445.5MHz link freq) on 4 lanes, and drop to 10 bit > readout. Programming HMAX also then becomes more entertaining as it > appears to be half the expected value (0x44c = 1100 decimal, which is > smaller than the width), so it'll need a fair amount of messing to get > all the controls to behave as expected. > That's all outside the scope of this patch set - 60fps was the only > frame rate previously supported, and we've expanded on that with > slower frame rate support. I'm totally fine with that. Thanks for the explanation. > FWIW I have checked with Sony over the 120fps modes, and experimented a little. > Sony will only support 120fps in 10bit. The flyer for IMX462 that > implies it does 1920x1080@120fps in 12 bit mode is incorrect. > Experimentation had 720p120 working in 12bit, but trying to work > through limiting the various bit depth and resolution options is going > to get really ugly in the driver. Let's enjoy the beauty of your patch set today and leave the ugliness for tomorrow :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > .link_freq_index = FREQ_INDEX_1080P, > > > .data = imx290_1080p_settings, > > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { > > > { > > > .width = 1280, > > > .height = 720, > > > - .hmax = 3300, > > > + .hmax_min = 3300, > > > .link_freq_index = FREQ_INDEX_720P, > > > .data = imx290_720p_settings, > > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > > > { > > > .width = 1920, > > > .height = 1080, > > > - .hmax = 2200, > > > + .hmax_min = 2200, > > > .link_freq_index = FREQ_INDEX_1080P, > > > .data = imx290_1080p_settings, > > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > > > { > > > .width = 1280, > > > .height = 720, > > > - .hmax = 3300, > > > + .hmax_min = 3300, > > > .link_freq_index = FREQ_INDEX_720P, > > > .data = imx290_720p_settings, > > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) > > > } > > > break; > > > > > > + case V4L2_CID_HBLANK: > > > + ret = imx290_write(imx290, IMX290_HMAX, > > > + ctrl->val + imx290->current_mode->width, > > > + NULL); > > > + break; > > > + > > > default: > > > ret = -EINVAL; > > > break; > > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, > > > const struct v4l2_mbus_framefmt *format, > > > const struct imx290_mode *mode) > > > { > > > - unsigned int hblank = mode->hmax - mode->width; > > > + unsigned int hblank_min = mode->hmax_min - mode->width; > > > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > > > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > > > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > > > > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); > > > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, > > > + hblank_min); > > > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); > > > } > > > > > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > > > 0, 0, imx290_test_pattern_menu); > > > > > > + /* > > > + * Actual range will be set from imx290_ctrl_update later in the probe. > > > + */ > > > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > > V4L2_CID_HBLANK, 1, 1, 1, 1); > > > - if (imx290->hblank) > > > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > > V4L2_CID_VBLANK, 1, 1, 1, 1); > > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290, > > > return ret; > > > } > > > > > > - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, > > > - NULL); > > > - if (ret) > > > - return ret; > > > - > > > /* Apply customized values from user */ > > > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > > > if (ret) {
Hi Dave, thanks for the patch. Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson: > The driver exposed V4L2_CID_HBLANK as a read only control to allow > for exposure calculations and determination of the frame rate. > > Convert to a read/write control so that the frame rate can be > controlled. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > 1 file changed, 19 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index 9ddd6382b127..9006be6e5e7c 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -47,6 +47,7 @@ > #define IMX290_GAIN IMX290_REG_8BIT(0x3014) > #define IMX290_VMAX IMX290_REG_24BIT(0x3018) > #define IMX290_HMAX IMX290_REG_16BIT(0x301c) > +#define IMX290_HMAX_MAX 0xffff > #define IMX290_SHS1 IMX290_REG_24BIT(0x3020) > #define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) > #define IMX290_WINPV IMX290_REG_16BIT(0x303c) > @@ -167,7 +168,7 @@ struct imx290_regval { > struct imx290_mode { > u32 width; > u32 height; > - u32 hmax; > + u32 hmax_min; > u8 link_freq_index; > > const struct imx290_regval *data; > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = > { { > .width = 1920, > .height = 1080, > - .hmax = 2200, > + .hmax_min = 2200, > .link_freq_index = FREQ_INDEX_1080P, > .data = imx290_1080p_settings, > .data_size = ARRAY_SIZE(imx290_1080p_settings), > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = > { { > .width = 1280, > .height = 720, > - .hmax = 3300, > + .hmax_min = 3300, > .link_freq_index = FREQ_INDEX_720P, > .data = imx290_720p_settings, > .data_size = ARRAY_SIZE(imx290_720p_settings), > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = > { { > .width = 1920, > .height = 1080, > - .hmax = 2200, > + .hmax_min = 2200, > .link_freq_index = FREQ_INDEX_1080P, > .data = imx290_1080p_settings, > .data_size = ARRAY_SIZE(imx290_1080p_settings), > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = > { { > .width = 1280, > .height = 720, > - .hmax = 3300, > + .hmax_min = 3300, > .link_freq_index = FREQ_INDEX_720P, > .data = imx290_720p_settings, > .data_size = ARRAY_SIZE(imx290_720p_settings), > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) You will need to remove V4L2_CID_HBLANK on the immediately return check at the beginning of the function. Otherwise this setting will never reach the device. Best regards Alexander > } > break; > > + case V4L2_CID_HBLANK: > + ret = imx290_write(imx290, IMX290_HMAX, > + ctrl->val + imx290->current_mode- >width, > + NULL); > + break; > + > default: > ret = -EINVAL; > break; > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, > const struct v4l2_mbus_framefmt *format, > const struct imx290_mode *mode) > { > - unsigned int hblank = mode->hmax - mode->width; > + unsigned int hblank_min = mode->hmax_min - mode->width; > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, > + hblank_min); > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); > } > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > 0, 0, imx290_test_pattern_menu); > > + /* > + * Actual range will be set from imx290_ctrl_update later in the probe. > + */ > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > V4L2_CID_HBLANK, 1, 1, 1, 1); > - if (imx290->hblank) > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > V4L2_CID_VBLANK, 1, 1, 1, 1); > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 > *imx290, return ret; > } > > - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, > - NULL); > - if (ret) > - return ret; > - > /* Apply customized values from user */ > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > if (ret) {
Hi Alexander On Fri, 3 Feb 2023 at 07:19, Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Hi Dave, > > thanks for the patch. > > Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson: > > The driver exposed V4L2_CID_HBLANK as a read only control to allow > > for exposure calculations and determination of the frame rate. > > > > Convert to a read/write control so that the frame rate can be > > controlled. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > --- > > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index 9ddd6382b127..9006be6e5e7c 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -47,6 +47,7 @@ > > #define IMX290_GAIN > IMX290_REG_8BIT(0x3014) > > #define IMX290_VMAX > IMX290_REG_24BIT(0x3018) > > #define IMX290_HMAX > IMX290_REG_16BIT(0x301c) > > +#define IMX290_HMAX_MAX 0xffff > > #define IMX290_SHS1 > IMX290_REG_24BIT(0x3020) > > #define IMX290_WINWV_OB > IMX290_REG_8BIT(0x303a) > > #define IMX290_WINPV > IMX290_REG_16BIT(0x303c) > > @@ -167,7 +168,7 @@ struct imx290_regval { > > struct imx290_mode { > > u32 width; > > u32 height; > > - u32 hmax; > > + u32 hmax_min; > > u8 link_freq_index; > > > > const struct imx290_regval *data; > > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = > > { { > > .width = 1920, > > .height = 1080, > > - .hmax = 2200, > > + .hmax_min = 2200, > > .link_freq_index = FREQ_INDEX_1080P, > > .data = imx290_1080p_settings, > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = > > { { > > .width = 1280, > > .height = 720, > > - .hmax = 3300, > > + .hmax_min = 3300, > > .link_freq_index = FREQ_INDEX_720P, > > .data = imx290_720p_settings, > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = > > { { > > .width = 1920, > > .height = 1080, > > - .hmax = 2200, > > + .hmax_min = 2200, > > .link_freq_index = FREQ_INDEX_1080P, > > .data = imx290_1080p_settings, > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = > > { { > > .width = 1280, > > .height = 720, > > - .hmax = 3300, > > + .hmax_min = 3300, > > .link_freq_index = FREQ_INDEX_720P, > > .data = imx290_720p_settings, > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) > > You will need to remove V4L2_CID_HBLANK on the immediately return check at the > beginning of the function. Otherwise this setting will never reach the device. What tree are you adding these patches to? I'm basing it on Sakari's tree at [1] - he's issued a pull for it, so that should be in 6.3. The only immediate return check at the start of imx290_set_ctrl is if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) return 0; The controls are no longer READ_ONLY, therefore they don't return early. There is no case for V4L2_CID_HBLANK. Does this also account for the difference you're reporting with V4L2_CID_VBLANK? Dave [1] https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290.c#n567 > Best regards > Alexander > > > } > > break; > > > > + case V4L2_CID_HBLANK: > > + ret = imx290_write(imx290, IMX290_HMAX, > > + ctrl->val + imx290->current_mode- > >width, > > + NULL); > > + break; > > + > > default: > > ret = -EINVAL; > > break; > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, > > const struct v4l2_mbus_framefmt *format, > > const struct imx290_mode *mode) > > { > > - unsigned int hblank = mode->hmax - mode->width; > > + unsigned int hblank_min = mode->hmax_min - mode->width; > > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); > > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, > > + hblank_min); > > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); > > } > > > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > > 0, 0, imx290_test_pattern_menu); > > > > + /* > > + * Actual range will be set from imx290_ctrl_update later in the > probe. > > + */ > > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > V4L2_CID_HBLANK, 1, 1, 1, > 1); > > - if (imx290->hblank) > > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > V4L2_CID_VBLANK, 1, 1, 1, > 1); > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 > > *imx290, return ret; > > } > > > > - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, > > - NULL); > > - if (ret) > > - return ret; > > - > > /* Apply customized values from user */ > > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > > if (ret) { > > > >
Hi Dave, Am Freitag, 3. Februar 2023, 09:05:44 CET schrieb Dave Stevenson: > Hi Alexander > > On Fri, 3 Feb 2023 at 07:19, Alexander Stein > > <alexander.stein@ew.tq-group.com> wrote: > > Hi Dave, > > > > thanks for the patch. > > > > Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson: > > > The driver exposed V4L2_CID_HBLANK as a read only control to allow > > > for exposure calculations and determination of the frame rate. > > > > > > Convert to a read/write control so that the frame rate can be > > > controlled. > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > > > > > drivers/media/i2c/imx290.c | 33 +++++++++++++++++++-------------- > > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > index 9ddd6382b127..9006be6e5e7c 100644 > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -47,6 +47,7 @@ > > > > > > #define IMX290_GAIN > > > > IMX290_REG_8BIT(0x3014) > > > > > #define IMX290_VMAX > > > > IMX290_REG_24BIT(0x3018) > > > > > #define IMX290_HMAX > > > > IMX290_REG_16BIT(0x301c) > > > > > +#define IMX290_HMAX_MAX 0xffff > > > > > > #define IMX290_SHS1 > > > > IMX290_REG_24BIT(0x3020) > > > > > #define IMX290_WINWV_OB > > > > IMX290_REG_8BIT(0x303a) > > > > > #define IMX290_WINPV > > > > IMX290_REG_16BIT(0x303c) > > > > > @@ -167,7 +168,7 @@ struct imx290_regval { > > > > > > struct imx290_mode { > > > > > > u32 width; > > > u32 height; > > > > > > - u32 hmax; > > > + u32 hmax_min; > > > > > > u8 link_freq_index; > > > > > > const struct imx290_regval *data; > > > > > > @@ -410,7 +411,7 @@ static const struct imx290_mode > > > imx290_modes_2lanes[] = > > > { { > > > > > > .width = 1920, > > > .height = 1080, > > > > > > - .hmax = 2200, > > > + .hmax_min = 2200, > > > > > > .link_freq_index = FREQ_INDEX_1080P, > > > .data = imx290_1080p_settings, > > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > > > > > @@ -418,7 +419,7 @@ static const struct imx290_mode > > > imx290_modes_2lanes[] = > > > { { > > > > > > .width = 1280, > > > .height = 720, > > > > > > - .hmax = 3300, > > > + .hmax_min = 3300, > > > > > > .link_freq_index = FREQ_INDEX_720P, > > > .data = imx290_720p_settings, > > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > > > > > @@ -429,7 +430,7 @@ static const struct imx290_mode > > > imx290_modes_4lanes[] = > > > { { > > > > > > .width = 1920, > > > .height = 1080, > > > > > > - .hmax = 2200, > > > + .hmax_min = 2200, > > > > > > .link_freq_index = FREQ_INDEX_1080P, > > > .data = imx290_1080p_settings, > > > .data_size = ARRAY_SIZE(imx290_1080p_settings), > > > > > > @@ -437,7 +438,7 @@ static const struct imx290_mode > > > imx290_modes_4lanes[] = > > > { { > > > > > > .width = 1280, > > > .height = 720, > > > > > > - .hmax = 3300, > > > + .hmax_min = 3300, > > > > > > .link_freq_index = FREQ_INDEX_720P, > > > .data = imx290_720p_settings, > > > .data_size = ARRAY_SIZE(imx290_720p_settings), > > > > > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) > > > > You will need to remove V4L2_CID_HBLANK on the immediately return check at > > the beginning of the function. Otherwise this setting will never reach > > the device. > What tree are you adding these patches to? I'm basing it on Sakari's > tree at [1] - he's issued a pull for it, so that should be in 6.3. Thanks for pointing this out. I'm based on linux-next as I need other patches for platform support. I've had Laurent's patches included, but only v2 :( my bad. With v3 included instead this seems to work. > The only immediate return check at the start of imx290_set_ctrl is > if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) > return 0; > > The controls are no longer READ_ONLY, therefore they don't return early. > There is no case for V4L2_CID_HBLANK. > > Does this also account for the difference you're reporting with > V4L2_CID_VBLANK? Yes, things look good so far. Thanks Best regards, Alexander > [1] > https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290 > .c#n567 > > Best regards > > Alexander > > > > > } > > > break; > > > > > > + case V4L2_CID_HBLANK: > > > + ret = imx290_write(imx290, IMX290_HMAX, > > > + ctrl->val + imx290->current_mode- > > > > > >width, > > > > > > + NULL); > > > + break; > > > + > > > > > > default: > > > ret = -EINVAL; > > > break; > > > > > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 > > > *imx290, > > > > > > const struct v4l2_mbus_framefmt *format, > > > const struct imx290_mode *mode) > > > > > > { > > > > > > - unsigned int hblank = mode->hmax - mode->width; > > > + unsigned int hblank_min = mode->hmax_min - mode->width; > > > + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > > > > > > unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > > > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > > > > > - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, > > > hblank); > > > + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, > > > 1, > > > + hblank_min); > > > > > > __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, > > > vblank); > > > > > > } > > > > > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > > > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > > > > > 0, 0, imx290_test_pattern_menu); > > > > > > + /* > > > + * Actual range will be set from imx290_ctrl_update later in the > > > > probe. > > > > > + */ > > > > > > imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, > > > &imx290_ctrl_ops, > > > > > > V4L2_CID_HBLANK, 1, 1, 1, > > > > 1); > > > > > - if (imx290->hblank) > > > - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, > > > &imx290_ctrl_ops, > > > > > > V4L2_CID_VBLANK, 1, 1, 1, > > > > 1); > > > > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 > > > *imx290, return ret; > > > > > > } > > > > > > - ret = imx290_write(imx290, IMX290_HMAX, > > > imx290->current_mode->hmax, > > > - NULL); > > > - if (ret) > > > - return ret; > > > - > > > > > > /* Apply customized values from user */ > > > ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); > > > if (ret) {
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c index 9ddd6382b127..9006be6e5e7c 100644 --- a/drivers/media/i2c/imx290.c +++ b/drivers/media/i2c/imx290.c @@ -47,6 +47,7 @@ #define IMX290_GAIN IMX290_REG_8BIT(0x3014) #define IMX290_VMAX IMX290_REG_24BIT(0x3018) #define IMX290_HMAX IMX290_REG_16BIT(0x301c) +#define IMX290_HMAX_MAX 0xffff #define IMX290_SHS1 IMX290_REG_24BIT(0x3020) #define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) #define IMX290_WINPV IMX290_REG_16BIT(0x303c) @@ -167,7 +168,7 @@ struct imx290_regval { struct imx290_mode { u32 width; u32 height; - u32 hmax; + u32 hmax_min; u8 link_freq_index; const struct imx290_regval *data; @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { { .width = 1920, .height = 1080, - .hmax = 2200, + .hmax_min = 2200, .link_freq_index = FREQ_INDEX_1080P, .data = imx290_1080p_settings, .data_size = ARRAY_SIZE(imx290_1080p_settings), @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = { { .width = 1280, .height = 720, - .hmax = 3300, + .hmax_min = 3300, .link_freq_index = FREQ_INDEX_720P, .data = imx290_720p_settings, .data_size = ARRAY_SIZE(imx290_720p_settings), @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { { .width = 1920, .height = 1080, - .hmax = 2200, + .hmax_min = 2200, .link_freq_index = FREQ_INDEX_1080P, .data = imx290_1080p_settings, .data_size = ARRAY_SIZE(imx290_1080p_settings), @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = { { .width = 1280, .height = 720, - .hmax = 3300, + .hmax_min = 3300, .link_freq_index = FREQ_INDEX_720P, .data = imx290_720p_settings, .data_size = ARRAY_SIZE(imx290_720p_settings), @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl) } break; + case V4L2_CID_HBLANK: + ret = imx290_write(imx290, IMX290_HMAX, + ctrl->val + imx290->current_mode->width, + NULL); + break; + default: ret = -EINVAL; break; @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290, const struct v4l2_mbus_framefmt *format, const struct imx290_mode *mode) { - unsigned int hblank = mode->hmax - mode->width; + unsigned int hblank_min = mode->hmax_min - mode->width; + unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); - __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank); + __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1, + hblank_min); __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank); } @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290) ARRAY_SIZE(imx290_test_pattern_menu) - 1, 0, 0, imx290_test_pattern_menu); + /* + * Actual range will be set from imx290_ctrl_update later in the probe. + */ imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, V4L2_CID_HBLANK, 1, 1, 1, 1); - if (imx290->hblank) - imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, V4L2_CID_VBLANK, 1, 1, 1, 1); @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290, return ret; } - ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax, - NULL); - if (ret) - return ret; - /* Apply customized values from user */ ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler); if (ret) {