[{"id":10976,"web_url":"https://patchwork.libcamera.org/comment/10976/","msgid":"<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>","date":"2020-06-29T17:01:14","subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 23.06.20 18:55, Jacopo Mondi wrote:\n> From: Dave Stevenson \n> \n> Clients need to know the pixel rate in order to compute exposure\n> and frame rate values. Advertise it.\n> \n> Signed-off-by: Dave Stevenson \n> Signed-off-by: Jacopo Mondi \n> ---\n> drivers/media/i2c/ov5647.c | 40 +++++++++++++++++++++++++++++++-------\n> 1 file changed, 33 insertions(+), 7 deletions(-)\n> \n> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> index 35865e56de5f9..218576a05e66b 100644\n> --- a/drivers/media/i2c/ov5647.c\n> +++ b/drivers/media/i2c/ov5647.c\n> @@ -76,6 +76,7 @@ struct regval_list {\n> struct ov5647_mode {\n> \tstruct v4l2_mbus_framefmt\tformat;\n> \tstruct v4l2_rect\t\tcrop;\n> +\tu64\t\t\t\tpixel_rate;\n> \tstruct regval_list\t\t*reg_list;\n> \tunsigned int\t\t\tnum_regs;\n> };\n> @@ -97,6 +98,7 @@ struct ov5647 {\n> \tstruct v4l2_ctrl_handler\tctrls;\n> \tstruct ov5647_mode\t\t*mode;\n> \tstruct ov5647_mode\t\t*current_mode;\n> +\tstruct v4l2_ctrl\t\t*pixel_rate;\n> };\n> \n> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> @@ -583,6 +585,7 @@ static struct ov5647_mode ov5647_sbggr8_modes[] = {\n> \t\t\t.width\t\t= 1280,\n> \t\t\t.height\t\t= 960,\n> \t\t},\n> +\t\t.pixel_rate\t= 77291670,\n> \t\t.reg_list\t= ov5647_640x480_sbggr8,\n> \t\t.num_regs\t= ARRAY_SIZE(ov5647_640x480_sbggr8)\n> \t},\n> @@ -604,6 +607,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> \t\t\t.width\t\t= 2592,\n> \t\t\t.height\t\t= 1944\n> \t\t},\n> +\t\t.pixel_rate\t= 87500000,\n> \t\t.reg_list\t= ov5647_2592x1944_sbggr10,\n> \t\t.num_regs\t= ARRAY_SIZE(ov5647_2592x1944_sbggr10)\n> \t},\n> @@ -622,6 +626,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> \t\t\t.width\t\t= 1928,\n> \t\t\t.height\t\t= 1080,\n> \t\t},\n> +\t\t.pixel_rate\t= 81666700,\n> \t\t.reg_list\t= ov5647_1080p30_sbggr10,\n> \t\t.num_regs\t= ARRAY_SIZE(ov5647_1080p30_sbggr10)\n> \t},\n> @@ -640,6 +645,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> \t\t\t.width\t\t= 2592,\n> \t\t\t.height\t\t= 1944,\n> \t\t},\n> +\t\t.pixel_rate\t= 81666700,\n> \t\t.reg_list\t= ov5647_2x2binned_sbggr10,\n> \t\t.num_regs\t= ARRAY_SIZE(ov5647_2x2binned_sbggr10)\n> \t},\n> @@ -658,6 +664,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> \t\t\t.width\t\t= 2560,\n> \t\t\t.height\t\t= 1920,\n> \t\t},\n> +\t\t.pixel_rate\t= 55000000,\n> \t\t.reg_list\t= ov5647_640x480_sbggr10,\n> \t\t.num_regs\t= ARRAY_SIZE(ov5647_640x480_sbggr10)\n> \t},\n> @@ -1094,6 +1101,10 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,\n> \t/* Update the sensor mode and apply at it at streamon time. */\n> \tmutex_lock(&sensor->lock);\n> \tsensor->mode = mode;\n> +\n> +\t__v4l2_ctrl_modify_range(sensor->pixel_rate, mode->pixel_rate,\n> +\t\t\t\t mode->pixel_rate, 1, mode->pixel_rate);\n> +\n> \t*fmt = mode->format;\n> \tmutex_unlock(&sensor->lock);\n> \n> @@ -1295,6 +1306,9 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)\n> \t\treturn ov5647_s_analogue_gain(sd, ctrl->val);\n> \tcase V4L2_CID_EXPOSURE:\n> \t\treturn ov5647_s_exposure(sd, ctrl->val);\n> +\tcase V4L2_CID_PIXEL_RATE:\n> +\t\t/* Read-only, but we adjust it based on mode. */\n\nLooking at other drivers, I see they don't handle read only controls\nin s_ctrl cb. Also the docs (vidioc-queryctrl.rst) says that trying to set a read only control should\nreturn EINVAL\n\nThanks,\nDafna\n\n> +\t\treturn 0;\n> \tdefault:\n> \t\tdev_info(&client->dev,\n> \t\t\t \"Control (id:0x%x, val:0x%x) not supported\\n\",\n> @@ -1313,7 +1327,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> {\n> \tstruct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);\n> \n> -\tv4l2_ctrl_handler_init(&sensor->ctrls, 5);\n> +\tv4l2_ctrl_handler_init(&sensor->ctrls, 6);\n> \n> \tv4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> \t\t\t V4L2_CID_AUTOGAIN, 0, 1, 1, 0);\n> @@ -1333,17 +1347,29 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> \tv4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> \t\t\t V4L2_CID_ANALOGUE_GAIN, 16, 1023, 1, 32);\n> \n> -\tif (sensor->ctrls.error) {\n> -\t\tdev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> -\t\t\t__func__, sensor->ctrls.error);\n> -\t\tv4l2_ctrl_handler_free(&sensor->ctrls);\n> +\t/* By default, PIXEL_RATE is read only, but it does change per mode */\n> +\tsensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> +\t\t\t\t\t V4L2_CID_PIXEL_RATE,\n> +\t\t\t\t\t sensor->mode->pixel_rate,\n> +\t\t\t\t\t sensor->mode->pixel_rate, 1,\n> +\t\t\t\t\t sensor->mode->pixel_rate);\n> +\tif (!sensor->pixel_rate)\n> +\t\tgoto handler_free;\n> +\tsensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;\n> \n> -\t\treturn sensor->ctrls.error;\n> -\t}\n> +\tif (sensor->ctrls.error)\n> +\t\tgoto handler_free;\n> \n> \tsensor->sd.ctrl_handler = &sensor->ctrls;\n> \n> \treturn 0;\n> +\n> +handler_free:\n> +\tdev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> +\t\t__func__, sensor->ctrls.error);\n> +\tv4l2_ctrl_handler_free(&sensor->ctrls);\n> +\n> +\treturn sensor->ctrls.error;\n> }\n> \n> static int ov5647_parse_dt(struct ov5647 *sensor, struct device_node *np)\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D7A7FBFFE2\n\tfor ;\n\tMon, 29 Jun 2020 17:01:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66C88609DD;\n\tMon, 29 Jun 2020 19:01:19 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B813A603B2\n\tfor ;\n\tMon, 29 Jun 2020 19:01:17 +0200 (CEST)","from [IPv6:2003:cb:8737:cf00:84c7:ee07:61e9:a21f]\n\t(p200300cb8737cf0084c7ee0761e9a21f.dip0.t-ipconnect.de\n\t[IPv6:2003:cb:8737:cf00:84c7:ee07:61e9:a21f])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: dafna)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id DBF872A058A;\n\tMon, 29 Jun 2020 18:01:16 +0100 (BST)"],"To":"Jacopo Mondi , mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, hverkuil@xs4all.nl,\n\tlaurent.pinchart@ideasonboard.com, roman.kovalivskyi@globallogic.com, \n\tdave.stevenson@raspberrypi.org, naush@raspberrypi.com","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-3-jacopo@jmondi.org>","From":"Dafna Hirschfeld ","Message-ID":"<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>","Date":"Mon, 29 Jun 2020 19:01:14 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200623165550.45835-3-jacopo@jmondi.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew_gabbasov@mentor.com, mrodin@de.adit-jv.com, mripard@kernel.org,\n\tlibcamera-devel@lists.libcamera.org, sudipi@jp.adit-jv.com,\n\thugues.fruchet@st.com, erosca@de.adit-jv.com, aford173@gmail.com,\n\tlinux-media@vger.kernel.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10990,"web_url":"https://patchwork.libcamera.org/comment/10990/","msgid":"","date":"2020-06-29T21:25:12","subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Dafna\n\nOn Mon, 29 Jun 2020 at 18:01, Dafna Hirschfeld\n wrote:\n>\n>\n>\n> On 23.06.20 18:55, Jacopo Mondi wrote:\n> > From: Dave Stevenson \n> >\n> > Clients need to know the pixel rate in order to compute exposure\n> > and frame rate values. Advertise it.\n> >\n> > Signed-off-by: Dave Stevenson \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > drivers/media/i2c/ov5647.c | 40 +++++++++++++++++++++++++++++++-------\n> > 1 file changed, 33 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > index 35865e56de5f9..218576a05e66b 100644\n> > --- a/drivers/media/i2c/ov5647.c\n> > +++ b/drivers/media/i2c/ov5647.c\n> > @@ -76,6 +76,7 @@ struct regval_list {\n> > struct ov5647_mode {\n> > struct v4l2_mbus_framefmt format;\n> > struct v4l2_rect crop;\n> > + u64 pixel_rate;\n> > struct regval_list *reg_list;\n> > unsigned int num_regs;\n> > };\n> > @@ -97,6 +98,7 @@ struct ov5647 {\n> > struct v4l2_ctrl_handler ctrls;\n> > struct ov5647_mode *mode;\n> > struct ov5647_mode *current_mode;\n> > + struct v4l2_ctrl *pixel_rate;\n> > };\n> >\n> > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > @@ -583,6 +585,7 @@ static struct ov5647_mode ov5647_sbggr8_modes[] = {\n> > .width = 1280,\n> > .height = 960,\n> > },\n> > + .pixel_rate = 77291670,\n> > .reg_list = ov5647_640x480_sbggr8,\n> > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr8)\n> > },\n> > @@ -604,6 +607,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > .width = 2592,\n> > .height = 1944\n> > },\n> > + .pixel_rate = 87500000,\n> > .reg_list = ov5647_2592x1944_sbggr10,\n> > .num_regs = ARRAY_SIZE(ov5647_2592x1944_sbggr10)\n> > },\n> > @@ -622,6 +626,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > .width = 1928,\n> > .height = 1080,\n> > },\n> > + .pixel_rate = 81666700,\n> > .reg_list = ov5647_1080p30_sbggr10,\n> > .num_regs = ARRAY_SIZE(ov5647_1080p30_sbggr10)\n> > },\n> > @@ -640,6 +645,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > .width = 2592,\n> > .height = 1944,\n> > },\n> > + .pixel_rate = 81666700,\n> > .reg_list = ov5647_2x2binned_sbggr10,\n> > .num_regs = ARRAY_SIZE(ov5647_2x2binned_sbggr10)\n> > },\n> > @@ -658,6 +664,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > .width = 2560,\n> > .height = 1920,\n> > },\n> > + .pixel_rate = 55000000,\n> > .reg_list = ov5647_640x480_sbggr10,\n> > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr10)\n> > },\n> > @@ -1094,6 +1101,10 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,\n> > /* Update the sensor mode and apply at it at streamon time. */\n> > mutex_lock(&sensor->lock);\n> > sensor->mode = mode;\n> > +\n> > + __v4l2_ctrl_modify_range(sensor->pixel_rate, mode->pixel_rate,\n> > + mode->pixel_rate, 1, mode->pixel_rate);\n> > +\n> > *fmt = mode->format;\n> > mutex_unlock(&sensor->lock);\n> >\n> > @@ -1295,6 +1306,9 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)\n> > return ov5647_s_analogue_gain(sd, ctrl->val);\n> > case V4L2_CID_EXPOSURE:\n> > return ov5647_s_exposure(sd, ctrl->val);\n> > + case V4L2_CID_PIXEL_RATE:\n> > + /* Read-only, but we adjust it based on mode. */\n>\n> Looking at other drivers, I see they don't handle read only controls\n> in s_ctrl cb. Also the docs (vidioc-queryctrl.rst) says that trying to set a read only control should\n> return EINVAL\n\nIt was a while ago that I looked at this, but my memory says that\nwithout a handler here then the call to __v4l2_ctrl_modify_range to\nupdate the value failed. The ranges and default were changed, but the\ncurrent value wasn't.\n\nChecking now, yes, without returning 0 here, __v4l2_ctrl_modify_range\nreturns -22 (-EINVAL) and v4l2-ctl --list-ctrls afterwards shows\nImage Processing Controls\n\n pixel_rate 0x009f0902 (int64) : min=87500000\nmax=87500000 step=1 default=87500000 value=81666700 flags=read-only\n\nA failure of the framework then and needs fixing there, or is it\ncorrect for the driver to have to implement a set hander if it needs\nto update the ranges?\nA quick grep implies that ov5647 is the first driver that needs to\nupdate V4L2_CID_PIXEL_RATE based on mode. I'll check in the morning if\nthings like V4L2_CID_HBLANK behave the same - it may be a control type\nissue with 64 bit values.\n\n\nInteresting that the docs say EINVAL as at least one path in the\nframework appears to return -EACCES [1]\nChecking it:\npi@raspberrypi:~ $ v4l2-ctl --set-ctrl pixel_rate=42\nVIDIOC_S_EXT_CTRLS: failed: Permission denied\nError setting controls: Permission denied\nwhich would imply -EACCES\n\nI am testing on a 5.4 kernel rather than media_tree.git, but there\ndon't appear to be any significant differences in this area between\nthe two.\n\nCheers.\n Dave\n\n[1] https://git.linuxtv.org/media_tree.git/tree/drivers/media/v4l2-core/v4l2-ctrls.c#n4224\n\n> Thanks,\n> Dafna\n>\n> > + return 0;\n> > default:\n> > dev_info(&client->dev,\n> > \"Control (id:0x%x, val:0x%x) not supported\\n\",\n> > @@ -1313,7 +1327,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > {\n> > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);\n> >\n> > - v4l2_ctrl_handler_init(&sensor->ctrls, 5);\n> > + v4l2_ctrl_handler_init(&sensor->ctrls, 6);\n> >\n> > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);\n> > @@ -1333,17 +1347,29 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > V4L2_CID_ANALOGUE_GAIN, 16, 1023, 1, 32);\n> >\n> > - if (sensor->ctrls.error) {\n> > - dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > - __func__, sensor->ctrls.error);\n> > - v4l2_ctrl_handler_free(&sensor->ctrls);\n> > + /* By default, PIXEL_RATE is read only, but it does change per mode */\n> > + sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > + V4L2_CID_PIXEL_RATE,\n> > + sensor->mode->pixel_rate,\n> > + sensor->mode->pixel_rate, 1,\n> > + sensor->mode->pixel_rate);\n> > + if (!sensor->pixel_rate)\n> > + goto handler_free;\n> > + sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;\n> >\n> > - return sensor->ctrls.error;\n> > - }\n> > + if (sensor->ctrls.error)\n> > + goto handler_free;\n> >\n> > sensor->sd.ctrl_handler = &sensor->ctrls;\n> >\n> > return 0;\n> > +\n> > +handler_free:\n> > + dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > + __func__, sensor->ctrls.error);\n> > + v4l2_ctrl_handler_free(&sensor->ctrls);\n> > +\n> > + return sensor->ctrls.error;\n> > }\n> >\n> > static int ov5647_parse_dt(struct ov5647 *sensor, struct device_node *np)\n> >\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2FBCABFFE2\n\tfor ;\n\tMon, 29 Jun 2020 21:25:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2527609DB;\n\tMon, 29 Jun 2020 23:25:29 +0200 (CEST)","from mail-wr1-x441.google.com (mail-wr1-x441.google.com\n\t[IPv6:2a00:1450:4864:20::441])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8EC50603B2\n\tfor ;\n\tMon, 29 Jun 2020 23:25:28 +0200 (CEST)","by mail-wr1-x441.google.com with SMTP id r12so18001666wrj.13\n\tfor ;\n\tMon, 29 Jun 2020 14:25:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"P3b0Oc0J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=BM50nnBHnVeRvMZAbBCWlBD03TO+/80b6hWhqJH/GZY=;\n\tb=P3b0Oc0JgkqZgKoOV67SdE4VfSsU2JGvGtH/XKD5HTmzB9gzjqrOWmyeONm5DhPz+Q\n\t/MLyuMMBR04vWUBk1L571QgJ8PXXpmfOK5YD5RRSoAYmeD+QszCfRI3X68J9MKw20cwQ\n\tH3I3TRPYcEKB/x1T8wBYhS0OPd1c9hDarsz2D+p3fXG+hcujQz6CjK5hnrS5xxghMQbw\n\tc1K/f4aYDDIg0wJmyUrrDtFTxfPppuCMsgqGiI20Pel6lJxRBfNUufFRayMpM0sKgTVN\n\tB18/4knFdP3Uodk/LF0/6WTVTQPQOYoR2vy8fOPSfOtH74w6at3r6Tg2qG87XngppomV\n\tiiTA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=BM50nnBHnVeRvMZAbBCWlBD03TO+/80b6hWhqJH/GZY=;\n\tb=DV36HJbfLXjDc1wz4Fp7SITXBzeTmSUt6ISg/sml2LFaNROMmyEG4Nhp2me8LyWTcO\n\t7xlO1/4jJwN5ZCDEKDCu8+UX0ut+P8JXC/a1oOSmNYS2htnUClX8yFSnef1z10cFlql8\n\t31nlhdlnoNAQfb6/kLV+7gMn1cQDHFpxJ+hs4f9+8J3XWLkGO+0TaevptwxmsbHrRW0/\n\to9oqAg44iFTO3ccwb/4Z5K5CwkrYRZZcEOLVAOIM7h/54Ydt3cZ5l0ntAlfDAE5xi6Dn\n\tmnI52ICd3X+0b/47a8Q3IjKCWqfPcoJWagvRi41PCA5zsE4+sxgfu2NYM9zzUpqb4/vP\n\tB8nQ==","X-Gm-Message-State":"AOAM530WjyPH5/+5Dl3aAucA35bXq1i8lO2/ihdvcILxyvnALaGoHjYU\n\tktIuZLUi/1kqWE10icLaSVF5wFxx/ojvORRDQj69Dg==","X-Google-Smtp-Source":"ABdhPJybfFNMi3YyvOvpv8ztq38Sb1e0ZKFiOiMy63eHPDD2Rpb5GtPT4Szks0WP96EU0G6C26wzBtE2IUjnDice7MI=","X-Received":"by 2002:adf:fc90:: with SMTP id\n\tg16mr18655166wrr.42.1593465927946; \n\tMon, 29 Jun 2020 14:25:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-3-jacopo@jmondi.org>\n\t<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>","In-Reply-To":"<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>","From":"Dave Stevenson ","Date":"Mon, 29 Jun 2020 22:25:12 +0100","Message-ID":"","To":"Dafna Hirschfeld ","Subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew_gabbasov@mentor.com,\n\tDave Stevenson , \n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com,\n\tMaxime Ripard ,\n\tRoman Kovalivskyi ,\n\tlibcamera-devel@lists.libcamera.org,\n\tLinux Media Mailing List ,\n\tSakari Ailus , hugues.fruchet@st.com,\n\tMauro Carvalho Chehab , aford173@gmail.com,\n\tsudipi@jp.adit-jv.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11029,"web_url":"https://patchwork.libcamera.org/comment/11029/","msgid":"","date":"2020-06-30T15:13:29","subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"On Mon, 29 Jun 2020 at 22:25, Dave Stevenson\n wrote:\n>\n> Hi Dafna\n>\n> On Mon, 29 Jun 2020 at 18:01, Dafna Hirschfeld\n> wrote:\n> >\n> >\n> >\n> > On 23.06.20 18:55, Jacopo Mondi wrote:\n> > > From: Dave Stevenson \n> > >\n> > > Clients need to know the pixel rate in order to compute exposure\n> > > and frame rate values. Advertise it.\n> > >\n> > > Signed-off-by: Dave Stevenson \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > drivers/media/i2c/ov5647.c | 40 +++++++++++++++++++++++++++++++-------\n> > > 1 file changed, 33 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > > index 35865e56de5f9..218576a05e66b 100644\n> > > --- a/drivers/media/i2c/ov5647.c\n> > > +++ b/drivers/media/i2c/ov5647.c\n> > > @@ -76,6 +76,7 @@ struct regval_list {\n> > > struct ov5647_mode {\n> > > struct v4l2_mbus_framefmt format;\n> > > struct v4l2_rect crop;\n> > > + u64 pixel_rate;\n> > > struct regval_list *reg_list;\n> > > unsigned int num_regs;\n> > > };\n> > > @@ -97,6 +98,7 @@ struct ov5647 {\n> > > struct v4l2_ctrl_handler ctrls;\n> > > struct ov5647_mode *mode;\n> > > struct ov5647_mode *current_mode;\n> > > + struct v4l2_ctrl *pixel_rate;\n> > > };\n> > >\n> > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > > @@ -583,6 +585,7 @@ static struct ov5647_mode ov5647_sbggr8_modes[] = {\n> > > .width = 1280,\n> > > .height = 960,\n> > > },\n> > > + .pixel_rate = 77291670,\n> > > .reg_list = ov5647_640x480_sbggr8,\n> > > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr8)\n> > > },\n> > > @@ -604,6 +607,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > .width = 2592,\n> > > .height = 1944\n> > > },\n> > > + .pixel_rate = 87500000,\n> > > .reg_list = ov5647_2592x1944_sbggr10,\n> > > .num_regs = ARRAY_SIZE(ov5647_2592x1944_sbggr10)\n> > > },\n> > > @@ -622,6 +626,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > .width = 1928,\n> > > .height = 1080,\n> > > },\n> > > + .pixel_rate = 81666700,\n> > > .reg_list = ov5647_1080p30_sbggr10,\n> > > .num_regs = ARRAY_SIZE(ov5647_1080p30_sbggr10)\n> > > },\n> > > @@ -640,6 +645,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > .width = 2592,\n> > > .height = 1944,\n> > > },\n> > > + .pixel_rate = 81666700,\n> > > .reg_list = ov5647_2x2binned_sbggr10,\n> > > .num_regs = ARRAY_SIZE(ov5647_2x2binned_sbggr10)\n> > > },\n> > > @@ -658,6 +664,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > .width = 2560,\n> > > .height = 1920,\n> > > },\n> > > + .pixel_rate = 55000000,\n> > > .reg_list = ov5647_640x480_sbggr10,\n> > > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr10)\n> > > },\n> > > @@ -1094,6 +1101,10 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,\n> > > /* Update the sensor mode and apply at it at streamon time. */\n> > > mutex_lock(&sensor->lock);\n> > > sensor->mode = mode;\n> > > +\n> > > + __v4l2_ctrl_modify_range(sensor->pixel_rate, mode->pixel_rate,\n> > > + mode->pixel_rate, 1, mode->pixel_rate);\n> > > +\n> > > *fmt = mode->format;\n> > > mutex_unlock(&sensor->lock);\n> > >\n> > > @@ -1295,6 +1306,9 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)\n> > > return ov5647_s_analogue_gain(sd, ctrl->val);\n> > > case V4L2_CID_EXPOSURE:\n> > > return ov5647_s_exposure(sd, ctrl->val);\n> > > + case V4L2_CID_PIXEL_RATE:\n> > > + /* Read-only, but we adjust it based on mode. */\n> >\n> > Looking at other drivers, I see they don't handle read only controls\n> > in s_ctrl cb. Also the docs (vidioc-queryctrl.rst) says that trying to set a read only control should\n> > return EINVAL\n>\n> It was a while ago that I looked at this, but my memory says that\n> without a handler here then the call to __v4l2_ctrl_modify_range to\n> update the value failed. The ranges and default were changed, but the\n> current value wasn't.\n>\n> Checking now, yes, without returning 0 here, __v4l2_ctrl_modify_range\n> returns -22 (-EINVAL) and v4l2-ctl --list-ctrls afterwards shows\n> Image Processing Controls\n>\n> pixel_rate 0x009f0902 (int64) : min=87500000\n> max=87500000 step=1 default=87500000 value=81666700 flags=read-only\n>\n> A failure of the framework then and needs fixing there, or is it\n> correct for the driver to have to implement a set hander if it needs\n> to update the ranges?\n> A quick grep implies that ov5647 is the first driver that needs to\n> update V4L2_CID_PIXEL_RATE based on mode. I'll check in the morning if\n> things like V4L2_CID_HBLANK behave the same - it may be a control type\n> issue with 64 bit values.\n\nJust following this up, it's not a 64 bit control thing, it's any read\nonly control won't update the current value if you\nuse__v4l2_ctrl_modify_range. That's why the patch adding\nV4L2_CID_HBLANK has to do the same thing.\nI've just tried it on imx219 and it works fine.....\n\nThe answer is down to \"modern\" sensor drivers using pm_runtime having a line\n if (pm_runtime_get_if_in_use(&client->dev) == 0)\n return 0;\nin their s_ctrl function. So whenever the sensor isn't streaming,\nwhich has to be the case for setting modes, gets a 0 return value from\nthe s_ctrl call even though the control is invalid.\nThat does seem to be a failure within the ctrl core then. Effectively\nother drivers do have this handling in, but via a different route.\n\nHans or Sakari may have ideas on how they would like to see the core\nv4l2-ctrls code fixing to cover this. try_or_set_cluster is called\nfrom too many places for me to know whether it is safe to just skip\nthe s_ctrl call for read only controls.\n\n Dave\n\n> Interesting that the docs say EINVAL as at least one path in the\n> framework appears to return -EACCES [1]\n> Checking it:\n> pi@raspberrypi:~ $ v4l2-ctl --set-ctrl pixel_rate=42\n> VIDIOC_S_EXT_CTRLS: failed: Permission denied\n> Error setting controls: Permission denied\n> which would imply -EACCES\n>\n> I am testing on a 5.4 kernel rather than media_tree.git, but there\n> don't appear to be any significant differences in this area between\n> the two.\n>\n> Cheers.\n> Dave\n>\n> [1] https://git.linuxtv.org/media_tree.git/tree/drivers/media/v4l2-core/v4l2-ctrls.c#n4224\n>\n> > Thanks,\n> > Dafna\n> >\n> > > + return 0;\n> > > default:\n> > > dev_info(&client->dev,\n> > > \"Control (id:0x%x, val:0x%x) not supported\\n\",\n> > > @@ -1313,7 +1327,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > > {\n> > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);\n> > >\n> > > - v4l2_ctrl_handler_init(&sensor->ctrls, 5);\n> > > + v4l2_ctrl_handler_init(&sensor->ctrls, 6);\n> > >\n> > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);\n> > > @@ -1333,17 +1347,29 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > V4L2_CID_ANALOGUE_GAIN, 16, 1023, 1, 32);\n> > >\n> > > - if (sensor->ctrls.error) {\n> > > - dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > > - __func__, sensor->ctrls.error);\n> > > - v4l2_ctrl_handler_free(&sensor->ctrls);\n> > > + /* By default, PIXEL_RATE is read only, but it does change per mode */\n> > > + sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > + V4L2_CID_PIXEL_RATE,\n> > > + sensor->mode->pixel_rate,\n> > > + sensor->mode->pixel_rate, 1,\n> > > + sensor->mode->pixel_rate);\n> > > + if (!sensor->pixel_rate)\n> > > + goto handler_free;\n> > > + sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;\n> > >\n> > > - return sensor->ctrls.error;\n> > > - }\n> > > + if (sensor->ctrls.error)\n> > > + goto handler_free;\n> > >\n> > > sensor->sd.ctrl_handler = &sensor->ctrls;\n> > >\n> > > return 0;\n> > > +\n> > > +handler_free:\n> > > + dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > > + __func__, sensor->ctrls.error);\n> > > + v4l2_ctrl_handler_free(&sensor->ctrls);\n> > > +\n> > > + return sensor->ctrls.error;\n> > > }\n> > >\n> > > static int ov5647_parse_dt(struct ov5647 *sensor, struct device_node *np)\n> > >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 57917BFFE2\n\tfor ;\n\tTue, 30 Jun 2020 15:13:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D78EB60C58;\n\tTue, 30 Jun 2020 17:13:45 +0200 (CEST)","from mail-wr1-x444.google.com (mail-wr1-x444.google.com\n\t[IPv6:2a00:1450:4864:20::444])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2B21609C7\n\tfor ;\n\tTue, 30 Jun 2020 17:13:44 +0200 (CEST)","by mail-wr1-x444.google.com with SMTP id z15so9310024wrl.8\n\tfor ;\n\tTue, 30 Jun 2020 08:13:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"CYbRzVP/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=orAP0OW+vZ4Ea7vBgKJPGIWJn9cS+H33u/7ocA1puAo=;\n\tb=CYbRzVP/naRi/OODuFuhUtFr4s71ZHXdzCBshaw691LvrGdMrntD+pb/WliaAkaQzW\n\tYIuFaSzD0kfH9qAMDTxiEo3xoed3vnS71XCij0+CoAyfZHfil4vQbB4EioETOkRlhRwq\n\tsUK4Ch6rqjU21i5FVzA+BhzkGxtbsOvUFwgMhLx5bp6nJoqQ70sR0abXk9k4vHM7t7Jn\n\t6ILFBcbssmK36nFJqVvUC4cNouheFHCD2i/69rDdbW4rIc2Rcx0Fk38mEht9ubpVn0h2\n\t+gC4UYyQUJ9AOKyVFN/74t8tpz3Klj2otVdVbu3GPnVr7c9qddkm9zpXwHj+Vojv1r+c\n\tKmMQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=orAP0OW+vZ4Ea7vBgKJPGIWJn9cS+H33u/7ocA1puAo=;\n\tb=qEpAuUu6KKHzWQUlhtKo/f63U5ISrpZbuQb7mbkx1RbpB1jj/G7S6ATuca7cr+iqBJ\n\tu0Fw2So/xQSUXFGR6Q+kuvtn1qB0Eo4wdVEgiAygzSkU6bVRYC4rEAC7KN5AtVTqUj+f\n\tYMOsM92h+ZTcGFwDIqG7ZDz70CvxW6UiCTasRwCxTFm0Cpo1dcCfA+tDRe/Z8Sk4eSC2\n\tota9Loe3KuelrEXogMbHgFgNIPFvMBxzvsCOC0GEjZC+bXusMjpK9H3bgZAcLvQpkhPP\n\trkjMpIUiPHTqnSIjAZlM1VyH5lA65kE0OF7QQqpQUNAUdCGzs6pZkxZMZZoVwct3BG07\n\tj4yA==","X-Gm-Message-State":"AOAM532c5c/C+ti55fgwk6ISLbTycyNUKw871JQKI53mJy+iIscTvnFC\n\tWF/HssRBCFrl0uh75L7Ff7Ua3sFOFYhVDJ+JcA6wGQ==","X-Google-Smtp-Source":"ABdhPJzE21IDIacoRJkRPsiIHM2cEcYp79YOnlTzRLnG39CQH1+snJaZ9yLQM60UJCYKbmOmmxFWFvoMXdm4MyuzABU=","X-Received":"by 2002:adf:81c7:: with SMTP id 65mr21266143wra.47.1593530024240;\n\tTue, 30 Jun 2020 08:13:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-3-jacopo@jmondi.org>\n\t<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Tue, 30 Jun 2020 16:13:29 +0100","Message-ID":"","To":"Dafna Hirschfeld ","Subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew_gabbasov@mentor.com,\n\tDave Stevenson , \n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com,\n\tMaxime Ripard ,\n\tRoman Kovalivskyi ,\n\tlibcamera-devel@lists.libcamera.org,\n\tLinux Media Mailing List ,\n\tSakari Ailus , hugues.fruchet@st.com,\n\tMauro Carvalho Chehab , aford173@gmail.com,\n\tsudipi@jp.adit-jv.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11031,"web_url":"https://patchwork.libcamera.org/comment/11031/","msgid":"<20200701072132.GG5963@pendragon.ideasonboard.com>","date":"2020-07-01T07:21:32","subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dave,\n\n(Question for Hans below)\n\nOn Tue, Jun 30, 2020 at 04:13:29PM +0100, Dave Stevenson wrote:\n> On Mon, 29 Jun 2020 at 22:25, Dave Stevenson wrote:\n> > On Mon, 29 Jun 2020 at 18:01, Dafna Hirschfeld wrote:\n> > > On 23.06.20 18:55, Jacopo Mondi wrote:\n> > > > From: Dave Stevenson \n> > > >\n> > > > Clients need to know the pixel rate in order to compute exposure\n> > > > and frame rate values. Advertise it.\n> > > >\n> > > > Signed-off-by: Dave Stevenson \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > drivers/media/i2c/ov5647.c | 40 +++++++++++++++++++++++++++++++-------\n> > > > 1 file changed, 33 insertions(+), 7 deletions(-)\n> > > >\n> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > > > index 35865e56de5f9..218576a05e66b 100644\n> > > > --- a/drivers/media/i2c/ov5647.c\n> > > > +++ b/drivers/media/i2c/ov5647.c\n> > > > @@ -76,6 +76,7 @@ struct regval_list {\n> > > > struct ov5647_mode {\n> > > > struct v4l2_mbus_framefmt format;\n> > > > struct v4l2_rect crop;\n> > > > + u64 pixel_rate;\n> > > > struct regval_list *reg_list;\n> > > > unsigned int num_regs;\n> > > > };\n> > > > @@ -97,6 +98,7 @@ struct ov5647 {\n> > > > struct v4l2_ctrl_handler ctrls;\n> > > > struct ov5647_mode *mode;\n> > > > struct ov5647_mode *current_mode;\n> > > > + struct v4l2_ctrl *pixel_rate;\n> > > > };\n> > > >\n> > > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > > > @@ -583,6 +585,7 @@ static struct ov5647_mode ov5647_sbggr8_modes[] = {\n> > > > .width = 1280,\n> > > > .height = 960,\n> > > > },\n> > > > + .pixel_rate = 77291670,\n> > > > .reg_list = ov5647_640x480_sbggr8,\n> > > > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr8)\n> > > > },\n> > > > @@ -604,6 +607,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > > .width = 2592,\n> > > > .height = 1944\n> > > > },\n> > > > + .pixel_rate = 87500000,\n> > > > .reg_list = ov5647_2592x1944_sbggr10,\n> > > > .num_regs = ARRAY_SIZE(ov5647_2592x1944_sbggr10)\n> > > > },\n> > > > @@ -622,6 +626,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > > .width = 1928,\n> > > > .height = 1080,\n> > > > },\n> > > > + .pixel_rate = 81666700,\n> > > > .reg_list = ov5647_1080p30_sbggr10,\n> > > > .num_regs = ARRAY_SIZE(ov5647_1080p30_sbggr10)\n> > > > },\n> > > > @@ -640,6 +645,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > > .width = 2592,\n> > > > .height = 1944,\n> > > > },\n> > > > + .pixel_rate = 81666700,\n> > > > .reg_list = ov5647_2x2binned_sbggr10,\n> > > > .num_regs = ARRAY_SIZE(ov5647_2x2binned_sbggr10)\n> > > > },\n> > > > @@ -658,6 +664,7 @@ static struct ov5647_mode ov5647_sbggr10_modes[] = {\n> > > > .width = 2560,\n> > > > .height = 1920,\n> > > > },\n> > > > + .pixel_rate = 55000000,\n> > > > .reg_list = ov5647_640x480_sbggr10,\n> > > > .num_regs = ARRAY_SIZE(ov5647_640x480_sbggr10)\n> > > > },\n> > > > @@ -1094,6 +1101,10 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,\n> > > > /* Update the sensor mode and apply at it at streamon time. */\n> > > > mutex_lock(&sensor->lock);\n> > > > sensor->mode = mode;\n> > > > +\n> > > > + __v4l2_ctrl_modify_range(sensor->pixel_rate, mode->pixel_rate,\n> > > > + mode->pixel_rate, 1, mode->pixel_rate);\n> > > > +\n> > > > *fmt = mode->format;\n> > > > mutex_unlock(&sensor->lock);\n> > > >\n> > > > @@ -1295,6 +1306,9 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)\n> > > > return ov5647_s_analogue_gain(sd, ctrl->val);\n> > > > case V4L2_CID_EXPOSURE:\n> > > > return ov5647_s_exposure(sd, ctrl->val);\n> > > > + case V4L2_CID_PIXEL_RATE:\n> > > > + /* Read-only, but we adjust it based on mode. */\n> > >\n> > > Looking at other drivers, I see they don't handle read only controls\n> > > in s_ctrl cb. Also the docs (vidioc-queryctrl.rst) says that trying to set a read only control should\n> > > return EINVAL\n> >\n> > It was a while ago that I looked at this, but my memory says that\n> > without a handler here then the call to __v4l2_ctrl_modify_range to\n> > update the value failed. The ranges and default were changed, but the\n> > current value wasn't.\n> >\n> > Checking now, yes, without returning 0 here, __v4l2_ctrl_modify_range\n> > returns -22 (-EINVAL) and v4l2-ctl --list-ctrls afterwards shows\n> > Image Processing Controls\n> >\n> > pixel_rate 0x009f0902 (int64) : min=87500000\n> > max=87500000 step=1 default=87500000 value=81666700 flags=read-only\n> >\n> > A failure of the framework then and needs fixing there, or is it\n> > correct for the driver to have to implement a set hander if it needs\n> > to update the ranges?\n> > A quick grep implies that ov5647 is the first driver that needs to\n> > update V4L2_CID_PIXEL_RATE based on mode. I'll check in the morning if\n> > things like V4L2_CID_HBLANK behave the same - it may be a control type\n> > issue with 64 bit values.\n> \n> Just following this up, it's not a 64 bit control thing, it's any read\n> only control won't update the current value if you\n> use__v4l2_ctrl_modify_range. That's why the patch adding\n> V4L2_CID_HBLANK has to do the same thing.\n> I've just tried it on imx219 and it works fine.....\n> \n> The answer is down to \"modern\" sensor drivers using pm_runtime having a line\n> if (pm_runtime_get_if_in_use(&client->dev) == 0)\n> return 0;\n> in their s_ctrl function. So whenever the sensor isn't streaming,\n> which has to be the case for setting modes, gets a 0 return value from\n> the s_ctrl call even though the control is invalid.\n> That does seem to be a failure within the ctrl core then. Effectively\n> other drivers do have this handling in, but via a different route.\n> \n> Hans or Sakari may have ideas on how they would like to see the core\n> v4l2-ctrls code fixing to cover this. try_or_set_cluster is called\n> from too many places for me to know whether it is safe to just skip\n> the s_ctrl call for read only controls.\n\nI think the control framework shouldn't call .s_ctrl() for read-only\ncontrols. If it's read-only the __v4l2_ctrl_modify_range() is supposed\nto originate from the same driver, so any specific driver-side handling\ncan be handled before or after calling __v4l2_ctrl_modify_range().\n\nA better helper function to update the value of a read-only control may\nbe a good idea. I may also have bypassed __v4l2_ctrl_modify_range()\ncompletely and set the value manually in the corresponding v4l2_ctrl\ninstance, but I understand that this would be called a hack. I know we\nwould lose support for control events in that case, but for\nV4L2_CID_PIXEL_RATE I don't think this is a problem. Control events are\nactually annoying to use synchronously, if you want to know what other\ncontrols have been impacted by a VIDIOC_S_CTRL call, userspace code to\nhandle this through control events would be a bit cumbersome.\n\n> > Interesting that the docs say EINVAL as at least one path in the\n> > framework appears to return -EACCES [1]\n> > Checking it:\n> > pi@raspberrypi:~ $ v4l2-ctl --set-ctrl pixel_rate=42\n> > VIDIOC_S_EXT_CTRLS: failed: Permission denied\n> > Error setting controls: Permission denied\n> > which would imply -EACCES\n> >\n> > I am testing on a 5.4 kernel rather than media_tree.git, but there\n> > don't appear to be any significant differences in this area between\n> > the two.\n> >\n> > [1] https://git.linuxtv.org/media_tree.git/tree/drivers/media/v4l2-core/v4l2-ctrls.c#n4224\n> >\n> > > > + return 0;\n> > > > default:\n> > > > dev_info(&client->dev,\n> > > > \"Control (id:0x%x, val:0x%x) not supported\\n\",\n> > > > @@ -1313,7 +1327,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > > > {\n> > > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);\n> > > >\n> > > > - v4l2_ctrl_handler_init(&sensor->ctrls, 5);\n> > > > + v4l2_ctrl_handler_init(&sensor->ctrls, 6);\n> > > >\n> > > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);\n> > > > @@ -1333,17 +1347,29 @@ static int ov5647_init_controls(struct ov5647 *sensor)\n> > > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > > V4L2_CID_ANALOGUE_GAIN, 16, 1023, 1, 32);\n> > > >\n> > > > - if (sensor->ctrls.error) {\n> > > > - dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > > > - __func__, sensor->ctrls.error);\n> > > > - v4l2_ctrl_handler_free(&sensor->ctrls);\n> > > > + /* By default, PIXEL_RATE is read only, but it does change per mode */\n> > > > + sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,\n> > > > + V4L2_CID_PIXEL_RATE,\n> > > > + sensor->mode->pixel_rate,\n> > > > + sensor->mode->pixel_rate, 1,\n> > > > + sensor->mode->pixel_rate);\n> > > > + if (!sensor->pixel_rate)\n> > > > + goto handler_free;\n> > > > + sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;\n> > > >\n> > > > - return sensor->ctrls.error;\n> > > > - }\n> > > > + if (sensor->ctrls.error)\n> > > > + goto handler_free;\n> > > >\n> > > > sensor->sd.ctrl_handler = &sensor->ctrls;\n> > > >\n> > > > return 0;\n> > > > +\n> > > > +handler_free:\n> > > > + dev_err(&client->dev, \"%s Controls initialization failed (%d)\\n\",\n> > > > + __func__, sensor->ctrls.error);\n> > > > + v4l2_ctrl_handler_free(&sensor->ctrls);\n> > > > +\n> > > > + return sensor->ctrls.error;\n> > > > }\n> > > >\n> > > > static int ov5647_parse_dt(struct ov5647 *sensor, struct device_node *np)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 04FA8BFFE2\n\tfor ;\n\tWed, 1 Jul 2020 07:21:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7ACD2603B3;\n\tWed, 1 Jul 2020 09:21:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 732A3603B3\n\tfor ;\n\tWed, 1 Jul 2020 09:21:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5F00556;\n\tWed, 1 Jul 2020 09:21:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uL8Iv4UH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593588095;\n\tbh=KnYOupadj5TRJi+1RB//JTyvDISLiTZHVsqacPc314M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uL8Iv4UHXbBSfOKYpAXjgXsUsXx2DdTRd2aIdkDnVNyYujp4hBXNTsIHD8VvILGO6\n\t9tnSSB9ixSFqIV2JX8Awkzn8pT8KyNHriBz+QlhVCARinfNmornrtn7u7yAeq61vUf\n\t4v45ue0pwjXscnYQ/ZKL+9v38kAHOh4tzwZaayHw=","Date":"Wed, 1 Jul 2020 10:21:32 +0300","From":"Laurent Pinchart ","To":"Dave Stevenson ","Message-ID":"<20200701072132.GG5963@pendragon.ideasonboard.com>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-3-jacopo@jmondi.org>\n\t<3aab9b3d-0156-e83c-9a63-026ded395af6@collabora.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 22/25] media: ov5647: Support\n\tV4L2_CID_PIXEL_RATE","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew_gabbasov@mentor.com,\n\tDave Stevenson , \n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com,\n\tMaxime Ripard ,\n\tRoman Kovalivskyi ,\n\tlibcamera-devel@lists.libcamera.org, sudipi@jp.adit-jv.com,\n\tSakari Ailus , hugues.fruchet@st.com,\n\tMauro Carvalho Chehab , aford173@gmail.com,\n\tLinux Media Mailing List ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]