[{"id":10979,"web_url":"https://patchwork.libcamera.org/comment/10979/","msgid":"<80139e40-914f-c547-922f-91fe3f611202@collabora.com>","date":"2020-06-29T17:48:16","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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> Store in the driver structure a pointer to the currently applied mode\n> and program a new one at s_stream(1) time only if it has changed.\n\nHi,\nI think this can be more readably implemented with a 'is_streaming' boolean\nfield.\n\nThanks,\nDafna\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> 1 file changed, 15 insertions(+), 1 deletion(-)\n> \n> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> index 39e320f321bd8..ac114269e1c73 100644\n> --- a/drivers/media/i2c/ov5647.c\n> +++ b/drivers/media/i2c/ov5647.c\n> @@ -96,6 +96,7 @@ struct ov5647 {\n> \tbool\t\t\t\tclock_ncont;\n> \tstruct v4l2_ctrl_handler\tctrls;\n> \tstruct ov5647_mode\t\t*mode;\n> +\tstruct ov5647_mode\t\t*current_mode;\n> };\n> \n> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> static int ov5647_set_mode(struct v4l2_subdev *sd)\n> {\n> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> +\tstruct ov5647 *sensor = to_sensor(sd);\n> \tu8 resetval, rdval;\n> \tint ret;\n> \n> +\tif (sensor->mode == sensor->current_mode)\n> +\t\treturn 0;\n> +\n> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> \tif (ret < 0)\n> \t\treturn ret;\n> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> \t\t\treturn ret;\n> \t}\n> \n> +\tsensor->current_mode = sensor->mode;\n> +\n> \treturn 0;\n> }\n> \n> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> \n> static int ov5647_stream_off(struct v4l2_subdev *sd)\n> {\n> +\tstruct ov5647 *sensor = to_sensor(sd);\n> \tint ret;\n> \n> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tsensor->current_mode = NULL;\n> +\n> +\treturn 0;\n> }\n> \n> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 59E7CBF415\n\tfor ;\n\tMon, 29 Jun 2020 17:48:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBA47609DD;\n\tMon, 29 Jun 2020 19:48:22 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0788609C9\n\tfor ;\n\tMon, 29 Jun 2020 19:48:20 +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 95EBD2A212B;\n\tMon, 29 Jun 2020 18:48:19 +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-1-jacopo@jmondi.org>","From":"Dafna Hirschfeld ","Message-ID":"<80139e40-914f-c547-922f-91fe3f611202@collabora.com>","Date":"Mon, 29 Jun 2020 19:48:16 +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-1-jacopo@jmondi.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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":10998,"web_url":"https://patchwork.libcamera.org/comment/10998/","msgid":"<20200630074305.soctqoaqirfdnvv2@uno.localdomain>","date":"2020-06-30T07:43:05","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Dafna,\n\nOn Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n>\n>\n> On 23.06.20 18:55, Jacopo Mondi wrote:\n> > Store in the driver structure a pointer to the currently applied mode\n> > and program a new one at s_stream(1) time only if it has changed.\n>\n> Hi,\n> I think this can be more readably implemented with a 'is_streaming' boolean\n> field.\n\nHow would you like to use an 'is_streaming' flag to decide if the\nsensor mode has to be updated ?\n\nThanks\n j\n\n\n>\n> Thanks,\n> Dafna\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> > 1 file changed, 15 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > index 39e320f321bd8..ac114269e1c73 100644\n> > --- a/drivers/media/i2c/ov5647.c\n> > +++ b/drivers/media/i2c/ov5647.c\n> > @@ -96,6 +96,7 @@ struct ov5647 {\n> > \tbool\t\t\t\tclock_ncont;\n> > \tstruct v4l2_ctrl_handler\tctrls;\n> > \tstruct ov5647_mode\t\t*mode;\n> > +\tstruct ov5647_mode\t\t*current_mode;\n> > };\n> > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> > static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > {\n> > \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > \tu8 resetval, rdval;\n> > \tint ret;\n> > +\tif (sensor->mode == sensor->current_mode)\n> > +\t\treturn 0;\n> > +\n> > \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > \t\t\treturn ret;\n> > \t}\n> > +\tsensor->current_mode = sensor->mode;\n> > +\n> > \treturn 0;\n> > }\n> > @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> > static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > {\n> > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > \tint ret;\n> > \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> > @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tsensor->current_mode = NULL;\n> > +\n> > +\treturn 0;\n> > }\n> > static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 161CDBFFE2\n\tfor ;\n\tTue, 30 Jun 2020 07:39:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A88E260C56;\n\tTue, 30 Jun 2020 09:39:40 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6213B609A9\n\tfor ;\n\tTue, 30 Jun 2020 09:39:39 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id DD2D7240015;\n\tTue, 30 Jun 2020 07:39:32 +0000 (UTC)"],"Date":"Tue, 30 Jun 2020 09:43:05 +0200","From":"Jacopo Mondi ","To":"Dafna Hirschfeld ","Message-ID":"<20200630074305.soctqoaqirfdnvv2@uno.localdomain>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<80139e40-914f-c547-922f-91fe3f611202@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, 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":11001,"web_url":"https://patchwork.libcamera.org/comment/11001/","msgid":"","date":"2020-06-30T09:32:12","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 30.06.20 09:43, Jacopo Mondi wrote:\n> Hi Dafna,\n> \n> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n>>\n>>\n>> On 23.06.20 18:55, Jacopo Mondi wrote:\n>>> Store in the driver structure a pointer to the currently applied mode\n>>> and program a new one at s_stream(1) time only if it has changed.\n>>\n>> Hi,\n>> I think this can be more readably implemented with a 'is_streaming' boolean\n>> field.\n> \n> How would you like to use an 'is_streaming' flag to decide if the\n> sensor mode has to be updated ?\n\nsince 'current_mode' is set to NULL upon `ov5647_stream_off`\nand you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\nit seem very similar to returning immediately from 'ov5647_set_stream' if the\ndevice is currently streaming.\n\nBut actually in this patch it seems to be possible to change the mode\nwhile streaming, if the callbacks are executed:\n\ns_stream(1)\ns_fmt\ns_stream(1)\n\nwhich is maybe a bug?\n\nThanks,\nDafna\n\n> \n> Thanks\n> j\n> \n> \n>>\n>> Thanks,\n>> Dafna\n>>\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n>>> index 39e320f321bd8..ac114269e1c73 100644\n>>> --- a/drivers/media/i2c/ov5647.c\n>>> +++ b/drivers/media/i2c/ov5647.c\n>>> @@ -96,6 +96,7 @@ struct ov5647 {\n>>> \tbool\t\t\t\tclock_ncont;\n>>> \tstruct v4l2_ctrl_handler\tctrls;\n>>> \tstruct ov5647_mode\t\t*mode;\n>>> +\tstruct ov5647_mode\t\t*current_mode;\n>>> };\n>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>> {\n>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>> \tu8 resetval, rdval;\n>>> \tint ret;\n>>> +\tif (sensor->mode == sensor->current_mode)\n>>> +\t\treturn 0;\n>>> +\n>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n>>> \tif (ret < 0)\n>>> \t\treturn ret;\n>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>> \t\t\treturn ret;\n>>> \t}\n>>> +\tsensor->current_mode = sensor->mode;\n>>> +\n>>> \treturn 0;\n>>> }\n>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>> {\n>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>> \tint ret;\n>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>> \tif (ret < 0)\n>>> \t\treturn ret;\n>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>> +\tif (ret < 0)\n>>> +\t\treturn ret;\n>>> +\n>>> +\tsensor->current_mode = NULL;\n>>> +\n>>> +\treturn 0;\n>>> }\n>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 842B1BF415\n\tfor ;\n\tTue, 30 Jun 2020 09:32:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17B4D60C56;\n\tTue, 30 Jun 2020 11:32: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 56D5B609A9\n\tfor ;\n\tTue, 30 Jun 2020 11:32:18 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 181F92A41FF"],"To":"Jacopo Mondi ","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>","From":"Dafna Hirschfeld ","Message-ID":"","Date":"Tue, 30 Jun 2020 11:32:12 +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":"<20200630074305.soctqoaqirfdnvv2@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, aford173@gmail.com,\n\tsudipi@jp.adit-jv.com","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":11002,"web_url":"https://patchwork.libcamera.org/comment/11002/","msgid":"<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>","date":"2020-06-30T10:06:51","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Dafna,\n\nOn Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n>\n>\n> On 30.06.20 09:43, Jacopo Mondi wrote:\n> > Hi Dafna,\n> >\n> > On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> > >\n> > >\n> > > On 23.06.20 18:55, Jacopo Mondi wrote:\n> > > > Store in the driver structure a pointer to the currently applied mode\n> > > > and program a new one at s_stream(1) time only if it has changed.\n> > >\n> > > Hi,\n> > > I think this can be more readably implemented with a 'is_streaming' boolean\n> > > field.\n> >\n> > How would you like to use an 'is_streaming' flag to decide if the\n> > sensor mode has to be updated ?\n>\n> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> device is currently streaming.\n\nNo, the code returns immediately from ov5647_set_mode() if mode ==\ncurrent_mode. The modes comparison makes sure the sensor is not\nreprogrammed if the mode hasn't changed. The remaning part of\ns_stream() is executed regardless of the mode configuration. Am I\nmissing some part of the picture ?\n\n>\n> But actually in this patch it seems to be possible to change the mode\n> while streaming, if the callbacks are executed:\n>\n> s_stream(1)\n> s_fmt\n> s_stream(1)\n>\n> which is maybe a bug?\n\nThe new format is stored in sensor->mode, and applied at the next\ns_stream(1) operation if it differs from the already programmed one,\nat least, this is how it is intended to work, have you found any\nfailing s_stream/set_fmt/s_stream which could be caused by a bug ?\n\nThanks\n j\n>\n> Thanks,\n> Dafna\n>\n> >\n> > Thanks\n> > j\n> >\n> >\n> > >\n> > > Thanks,\n> > > Dafna\n> > >\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> > > > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > > > index 39e320f321bd8..ac114269e1c73 100644\n> > > > --- a/drivers/media/i2c/ov5647.c\n> > > > +++ b/drivers/media/i2c/ov5647.c\n> > > > @@ -96,6 +96,7 @@ struct ov5647 {\n> > > > \tbool\t\t\t\tclock_ncont;\n> > > > \tstruct v4l2_ctrl_handler\tctrls;\n> > > > \tstruct ov5647_mode\t\t*mode;\n> > > > +\tstruct ov5647_mode\t\t*current_mode;\n> > > > };\n> > > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > > > @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> > > > static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > > > {\n> > > > \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> > > > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > > > \tu8 resetval, rdval;\n> > > > \tint ret;\n> > > > +\tif (sensor->mode == sensor->current_mode)\n> > > > +\t\treturn 0;\n> > > > +\n> > > > \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> > > > \tif (ret < 0)\n> > > > \t\treturn ret;\n> > > > @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > > > \t\t\treturn ret;\n> > > > \t}\n> > > > +\tsensor->current_mode = sensor->mode;\n> > > > +\n> > > > \treturn 0;\n> > > > }\n> > > > @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> > > > static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > > > {\n> > > > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > > > \tint ret;\n> > > > \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> > > > @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > > > \tif (ret < 0)\n> > > > \t\treturn ret;\n> > > > -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > > > +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tsensor->current_mode = NULL;\n> > > > +\n> > > > +\treturn 0;\n> > > > }\n> > > > static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 67178BFFE2\n\tfor ;\n\tTue, 30 Jun 2020 10:03:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC8FA60C56;\n\tTue, 30 Jun 2020 12:03:29 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 413B5609C5\n\tfor ;\n\tTue, 30 Jun 2020 12:03:28 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 06DE31BF205;\n\tTue, 30 Jun 2020 10:03:21 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 30 Jun 2020 12:06:51 +0200","From":"Jacopo Mondi ","To":"Dafna Hirschfeld ","Message-ID":"<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, 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":11012,"web_url":"https://patchwork.libcamera.org/comment/11012/","msgid":"","date":"2020-06-30T10:56:44","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 30.06.20 12:06, Jacopo Mondi wrote:\n> Hi Dafna,\n> \n> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n>>\n>>\n>> On 30.06.20 09:43, Jacopo Mondi wrote:\n>>> Hi Dafna,\n>>>\n>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n>>>>\n>>>>\n>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n>>>>> Store in the driver structure a pointer to the currently applied mode\n>>>>> and program a new one at s_stream(1) time only if it has changed.\n>>>>\n>>>> Hi,\n>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n>>>> field.\n>>>\n>>> How would you like to use an 'is_streaming' flag to decide if the\n>>> sensor mode has to be updated ?\n>>\n>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n>> device is currently streaming.\n> \n> No, the code returns immediately from ov5647_set_mode() if mode ==\n> current_mode. The modes comparison makes sure the sensor is not\n> reprogrammed if the mode hasn't changed. The remaning part of\n> s_stream() is executed regardless of the mode configuration. Am I\n> missing some part of the picture ?\n> \n>>\n>> But actually in this patch it seems to be possible to change the mode\n>> while streaming, if the callbacks are executed:\n>>\n>> s_stream(1)\n>> s_fmt\n>> s_stream(1)\n>>\n>> which is maybe a bug?\n> \n> The new format is stored in sensor->mode, and applied at the next\n> s_stream(1) operation if it differs from the already programmed one,\n> at least, this is how it is intended to work, have you found any\n> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n\nWhat I meant is that there could be valid sequence of calls\n\ns_stream(enable=1)\ns_fmt\ns_stream(enable=1)\n\nFor example if two video devices are connected to the sensor and they\nstream simultaneously. There was a discussion about adding a code to the core\nto follow the s_stream callback and call it only if the subdev is not streaming\nbut currently subdevs should support it themselves.\n\n\nThanks,\nDafna\n\n> \n> Thanks\n> j\n>>\n>> Thanks,\n>> Dafna\n>>\n>>>\n>>> Thanks\n>>> j\n>>>\n>>>\n>>>>\n>>>> Thanks,\n>>>> Dafna\n>>>>\n>>>>>\n>>>>> Signed-off-by: Jacopo Mondi \n>>>>> ---\n>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n>>>>> index 39e320f321bd8..ac114269e1c73 100644\n>>>>> --- a/drivers/media/i2c/ov5647.c\n>>>>> +++ b/drivers/media/i2c/ov5647.c\n>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n>>>>> \tbool\t\t\t\tclock_ncont;\n>>>>> \tstruct v4l2_ctrl_handler\tctrls;\n>>>>> \tstruct ov5647_mode\t\t*mode;\n>>>>> +\tstruct ov5647_mode\t\t*current_mode;\n>>>>> };\n>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>> {\n>>>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>>>> \tu8 resetval, rdval;\n>>>>> \tint ret;\n>>>>> +\tif (sensor->mode == sensor->current_mode)\n>>>>> +\t\treturn 0;\n>>>>> +\n>>>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n>>>>> \tif (ret < 0)\n>>>>> \t\treturn ret;\n>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>> \t\t\treturn ret;\n>>>>> \t}\n>>>>> +\tsensor->current_mode = sensor->mode;\n>>>>> +\n>>>>> \treturn 0;\n>>>>> }\n>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>> {\n>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>>>> \tint ret;\n>>>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>> \tif (ret < 0)\n>>>>> \t\treturn ret;\n>>>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>> +\tif (ret < 0)\n>>>>> +\t\treturn ret;\n>>>>> +\n>>>>> +\tsensor->current_mode = NULL;\n>>>>> +\n>>>>> +\treturn 0;\n>>>>> }\n>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 E6E84BF415\n\tfor ;\n\tTue, 30 Jun 2020 10:56:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AB0460C59;\n\tTue, 30 Jun 2020 12:56:50 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB1EB609C7\n\tfor ;\n\tTue, 30 Jun 2020 12:56:48 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 864782A0538"],"To":"Jacopo Mondi ","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>","From":"Dafna Hirschfeld ","Message-ID":"","Date":"Tue, 30 Jun 2020 12:56:44 +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":"<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, aford173@gmail.com,\n\tsudipi@jp.adit-jv.com","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":11014,"web_url":"https://patchwork.libcamera.org/comment/11014/","msgid":"<20200630120528.xffvivfriblc6a2y@uno.localdomain>","date":"2020-06-30T12:05:28","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Dafna,\n\nOn Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n>\n>\n> On 30.06.20 12:06, Jacopo Mondi wrote:\n> > Hi Dafna,\n> >\n> > On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n> > >\n> > >\n> > > On 30.06.20 09:43, Jacopo Mondi wrote:\n> > > > Hi Dafna,\n> > > >\n> > > > On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> > > > >\n> > > > >\n> > > > > On 23.06.20 18:55, Jacopo Mondi wrote:\n> > > > > > Store in the driver structure a pointer to the currently applied mode\n> > > > > > and program a new one at s_stream(1) time only if it has changed.\n> > > > >\n> > > > > Hi,\n> > > > > I think this can be more readably implemented with a 'is_streaming' boolean\n> > > > > field.\n> > > >\n> > > > How would you like to use an 'is_streaming' flag to decide if the\n> > > > sensor mode has to be updated ?\n> > >\n> > > since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> > > and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> > > it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> > > device is currently streaming.\n> >\n> > No, the code returns immediately from ov5647_set_mode() if mode ==\n> > current_mode. The modes comparison makes sure the sensor is not\n> > reprogrammed if the mode hasn't changed. The remaning part of\n> > s_stream() is executed regardless of the mode configuration. Am I\n> > missing some part of the picture ?\n> >\n> > >\n> > > But actually in this patch it seems to be possible to change the mode\n> > > while streaming, if the callbacks are executed:\n> > >\n> > > s_stream(1)\n> > > s_fmt\n> > > s_stream(1)\n> > >\n> > > which is maybe a bug?\n> >\n> > The new format is stored in sensor->mode, and applied at the next\n> > s_stream(1) operation if it differs from the already programmed one,\n> > at least, this is how it is intended to work, have you found any\n> > failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n>\n> What I meant is that there could be valid sequence of calls\n>\n> s_stream(enable=1)\n> s_fmt\n> s_stream(enable=1)\n>\n> For example if two video devices are connected to the sensor and they\n> stream simultaneously. There was a discussion about adding a code to the core\n\nI'm not sure it is possible, given that the subdev has a single source\npad\n\n> to follow the s_stream callback and call it only if the subdev is not streaming\n> but currently subdevs should support it themselves.\n>\n\nOh, so you're concerned about the fact userspace can call s_stream(1)\ntwice consecutively! it's indipendent from s_ftm if I got your point.\n\nIn this case a flag to control if the device is streaming already should\nhelp, yes, I overlooked that indeed.\n\nThanks\n j\n\n>\n> Thanks,\n> Dafna\n>\n> >\n> > Thanks\n> > j\n> > >\n> > > Thanks,\n> > > Dafna\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks,\n> > > > > Dafna\n> > > > >\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> > > > > > 1 file changed, 15 insertions(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > > > > > index 39e320f321bd8..ac114269e1c73 100644\n> > > > > > --- a/drivers/media/i2c/ov5647.c\n> > > > > > +++ b/drivers/media/i2c/ov5647.c\n> > > > > > @@ -96,6 +96,7 @@ struct ov5647 {\n> > > > > > \tbool\t\t\t\tclock_ncont;\n> > > > > > \tstruct v4l2_ctrl_handler\tctrls;\n> > > > > > \tstruct ov5647_mode\t\t*mode;\n> > > > > > +\tstruct ov5647_mode\t\t*current_mode;\n> > > > > > };\n> > > > > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > > > > > @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> > > > > > static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > > > > > {\n> > > > > > \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> > > > > > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > > > > > \tu8 resetval, rdval;\n> > > > > > \tint ret;\n> > > > > > +\tif (sensor->mode == sensor->current_mode)\n> > > > > > +\t\treturn 0;\n> > > > > > +\n> > > > > > \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> > > > > > \tif (ret < 0)\n> > > > > > \t\treturn ret;\n> > > > > > @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > > > > > \t\t\treturn ret;\n> > > > > > \t}\n> > > > > > +\tsensor->current_mode = sensor->mode;\n> > > > > > +\n> > > > > > \treturn 0;\n> > > > > > }\n> > > > > > @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> > > > > > static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > > > > > {\n> > > > > > +\tstruct ov5647 *sensor = to_sensor(sd);\n> > > > > > \tint ret;\n> > > > > > \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> > > > > > @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > > > > > \tif (ret < 0)\n> > > > > > \t\treturn ret;\n> > > > > > -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > > > > > +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > > > > > +\tif (ret < 0)\n> > > > > > +\t\treturn ret;\n> > > > > > +\n> > > > > > +\tsensor->current_mode = NULL;\n> > > > > > +\n> > > > > > +\treturn 0;\n> > > > > > }\n> > > > > > static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 CC391BF415\n\tfor ;\n\tTue, 30 Jun 2020 12:02:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 320A260C56;\n\tTue, 30 Jun 2020 14:02:07 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 813CF603B4\n\tfor ;\n\tTue, 30 Jun 2020 14:02:06 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id B89EB200008;\n\tTue, 30 Jun 2020 12:01:59 +0000 (UTC)"],"Date":"Tue, 30 Jun 2020 14:05:28 +0200","From":"Jacopo Mondi ","To":"Dafna Hirschfeld ","Message-ID":"<20200630120528.xffvivfriblc6a2y@uno.localdomain>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, 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":11017,"web_url":"https://patchwork.libcamera.org/comment/11017/","msgid":"","date":"2020-06-30T13:01:21","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"Hi,\n\n\nOn 30.06.20 14:05, Jacopo Mondi wrote:\n> Hi Dafna,\n> \n> On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n>>\n>>\n>> On 30.06.20 12:06, Jacopo Mondi wrote:\n>>> Hi Dafna,\n>>>\n>>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n>>>>\n>>>>\n>>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n>>>>> Hi Dafna,\n>>>>>\n>>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n>>>>>>\n>>>>>>\n>>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n>>>>>>> Store in the driver structure a pointer to the currently applied mode\n>>>>>>> and program a new one at s_stream(1) time only if it has changed.\n>>>>>>\n>>>>>> Hi,\n>>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n>>>>>> field.\n>>>>>\n>>>>> How would you like to use an 'is_streaming' flag to decide if the\n>>>>> sensor mode has to be updated ?\n>>>>\n>>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n>>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n>>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n>>>> device is currently streaming.\n>>>\n>>> No, the code returns immediately from ov5647_set_mode() if mode ==\n>>> current_mode. The modes comparison makes sure the sensor is not\n>>> reprogrammed if the mode hasn't changed. The remaning part of\n>>> s_stream() is executed regardless of the mode configuration. Am I\n>>> missing some part of the picture ?\n>>>\n>>>>\n>>>> But actually in this patch it seems to be possible to change the mode\n>>>> while streaming, if the callbacks are executed:\n>>>>\n>>>> s_stream(1)\n>>>> s_fmt\n>>>> s_stream(1)\n>>>>\n>>>> which is maybe a bug?\n>>>\n>>> The new format is stored in sensor->mode, and applied at the next\n>>> s_stream(1) operation if it differs from the already programmed one,\n>>> at least, this is how it is intended to work, have you found any\n>>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n>>\n>> What I meant is that there could be valid sequence of calls\n>>\n>> s_stream(enable=1)\n>> s_fmt\n>> s_stream(enable=1)\n>>\n>> For example if two video devices are connected to the sensor and they\n>> stream simultaneously. There was a discussion about adding a code to the core\n> \n> I'm not sure it is possible, given that the subdev has a single source\n> pad\n\nVideo devices should not be connected directly to the sensor, they can also\nbe connected to the sensor through an isp entity that is connected to the sensor\nfrom one side and to two video devices from the other side.\n\nThanks,\nD\n\n> \n>> to follow the s_stream callback and call it only if the subdev is not streaming\n>> but currently subdevs should support it themselves.\n>>\n> \n> Oh, so you're concerned about the fact userspace can call s_stream(1)\n> twice consecutively! it's indipendent from s_ftm if I got your point.\n> \n> In this case a flag to control if the device is streaming already should\n> help, yes, I overlooked that indeed.\n> \n> Thanks\n> j\n> \n>>\n>> Thanks,\n>> Dafna\n>>\n>>>\n>>> Thanks\n>>> j\n>>>>\n>>>> Thanks,\n>>>> Dafna\n>>>>\n>>>>>\n>>>>> Thanks\n>>>>> j\n>>>>>\n>>>>>\n>>>>>>\n>>>>>> Thanks,\n>>>>>> Dafna\n>>>>>>\n>>>>>>>\n>>>>>>> Signed-off-by: Jacopo Mondi \n>>>>>>> ---\n>>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n>>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>>>>>\n>>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n>>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n>>>>>>> --- a/drivers/media/i2c/ov5647.c\n>>>>>>> +++ b/drivers/media/i2c/ov5647.c\n>>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n>>>>>>> \tbool\t\t\t\tclock_ncont;\n>>>>>>> \tstruct v4l2_ctrl_handler\tctrls;\n>>>>>>> \tstruct ov5647_mode\t\t*mode;\n>>>>>>> +\tstruct ov5647_mode\t\t*current_mode;\n>>>>>>> };\n>>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n>>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n>>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>>>> {\n>>>>>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n>>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>>>>>> \tu8 resetval, rdval;\n>>>>>>> \tint ret;\n>>>>>>> +\tif (sensor->mode == sensor->current_mode)\n>>>>>>> +\t\treturn 0;\n>>>>>>> +\n>>>>>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n>>>>>>> \tif (ret < 0)\n>>>>>>> \t\treturn ret;\n>>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>>>> \t\t\treturn ret;\n>>>>>>> \t}\n>>>>>>> +\tsensor->current_mode = sensor->mode;\n>>>>>>> +\n>>>>>>> \treturn 0;\n>>>>>>> }\n>>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n>>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>>>> {\n>>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n>>>>>>> \tint ret;\n>>>>>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n>>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>>>> \tif (ret < 0)\n>>>>>>> \t\treturn ret;\n>>>>>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>>>> +\tif (ret < 0)\n>>>>>>> +\t\treturn ret;\n>>>>>>> +\n>>>>>>> +\tsensor->current_mode = NULL;\n>>>>>>> +\n>>>>>>> +\treturn 0;\n>>>>>>> }\n>>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 D4BDBBF415\n\tfor ;\n\tTue, 30 Jun 2020 13:01:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE5AF60C57;\n\tTue, 30 Jun 2020 15:01:30 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8E58609C7\n\tfor ;\n\tTue, 30 Jun 2020 15:01:29 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 320C32756E6"],"To":"Jacopo Mondi ","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>","From":"Dafna Hirschfeld ","Message-ID":"","Date":"Tue, 30 Jun 2020 15:01:21 +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":"<20200630120528.xffvivfriblc6a2y@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tlinux-media@vger.kernel.org, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, mchehab@kernel.org, aford173@gmail.com,\n\tsudipi@jp.adit-jv.com","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":11023,"web_url":"https://patchwork.libcamera.org/comment/11023/","msgid":"","date":"2020-06-30T14:53:06","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Dafna\n\nOn Tue, 30 Jun 2020 at 14:01, Dafna Hirschfeld\n wrote:\n>\n> Hi,\n>\n>\n> On 30.06.20 14:05, Jacopo Mondi wrote:\n> > Hi Dafna,\n> >\n> > On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n> >>\n> >>\n> >> On 30.06.20 12:06, Jacopo Mondi wrote:\n> >>> Hi Dafna,\n> >>>\n> >>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n> >>>>\n> >>>>\n> >>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n> >>>>> Hi Dafna,\n> >>>>>\n> >>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> >>>>>>\n> >>>>>>\n> >>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n> >>>>>>> Store in the driver structure a pointer to the currently applied mode\n> >>>>>>> and program a new one at s_stream(1) time only if it has changed.\n> >>>>>>\n> >>>>>> Hi,\n> >>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n> >>>>>> field.\n> >>>>>\n> >>>>> How would you like to use an 'is_streaming' flag to decide if the\n> >>>>> sensor mode has to be updated ?\n> >>>>\n> >>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> >>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> >>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> >>>> device is currently streaming.\n> >>>\n> >>> No, the code returns immediately from ov5647_set_mode() if mode ==\n> >>> current_mode. The modes comparison makes sure the sensor is not\n> >>> reprogrammed if the mode hasn't changed. The remaning part of\n> >>> s_stream() is executed regardless of the mode configuration. Am I\n> >>> missing some part of the picture ?\n> >>>\n> >>>>\n> >>>> But actually in this patch it seems to be possible to change the mode\n> >>>> while streaming, if the callbacks are executed:\n> >>>>\n> >>>> s_stream(1)\n> >>>> s_fmt\n> >>>> s_stream(1)\n> >>>>\n> >>>> which is maybe a bug?\n> >>>\n> >>> The new format is stored in sensor->mode, and applied at the next\n> >>> s_stream(1) operation if it differs from the already programmed one,\n> >>> at least, this is how it is intended to work, have you found any\n> >>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n> >>\n> >> What I meant is that there could be valid sequence of calls\n> >>\n> >> s_stream(enable=1)\n> >> s_fmt\n> >> s_stream(enable=1)\n> >>\n> >> For example if two video devices are connected to the sensor and they\n> >> stream simultaneously. There was a discussion about adding a code to the core\n> >\n> > I'm not sure it is possible, given that the subdev has a single source\n> > pad\n>\n> Video devices should not be connected directly to the sensor,\n\nThat's an odd statement as it is exactly the situation we have on the\nPi. The CSI2 receiver writes data to RAM, therefore it is a video\ndevice.\nDid you intend to say that it isn't necessarily connected directly to\nthe sensor?\n\n> they can also\n> be connected to the sensor through an isp entity that is connected to the sensor\n> from one side and to two video devices from the other side.\n\nIt's true that some platforms will route the received CSI2 data\nstraight through the ISP, and only write the processed image(s) to\nRAM. If there are multiple output video devices from the ISP then yes\nthey will VIDIOC_STREAMON at different points.\nHowever is it really valid to call s_stream(1) on the sensor subdev\nfor each of them? Doesn't that mean you really need refcounting of the\nnumber of s_stream(1)'s (and s_stream(0)'s) within each sensor driver,\nso that it only actually stops streaming on the last s_stream(0), not\nthe first. A simple boolean isn't sufficient, otherwise\nVIDIOC_STREAMON(NODE_A);\nVIDIOC_STREAMON(NODE_B);\nVIDIOC_STREAMOFF(NODE_B);\nleaves you with no data from NODE_A even though it has never called\nVIDIOC_STREAMOFF.\n\nAnyway this patch was to remove excess writing of the mode registers if you did\ns_fmt\ns_stream(1)\ns_stream(0)\ns_stream(1)\n\nThe driver uses the s_power call rather than pm_runtime as that was an\nacceptable approach when it was written in 2017, so the power, and\nhence register settings, can be maintained between multiple s_stream\ncalls.\n\n Dave\n\n> Thanks,\n> D\n>\n> >\n> >> to follow the s_stream callback and call it only if the subdev is not streaming\n> >> but currently subdevs should support it themselves.\n> >>\n> >\n> > Oh, so you're concerned about the fact userspace can call s_stream(1)\n> > twice consecutively! it's indipendent from s_ftm if I got your point.\n> >\n> > In this case a flag to control if the device is streaming already should\n> > help, yes, I overlooked that indeed.\n> >\n> > Thanks\n> > j\n> >\n> >>\n> >> Thanks,\n> >> Dafna\n> >>\n> >>>\n> >>> Thanks\n> >>> j\n> >>>>\n> >>>> Thanks,\n> >>>> Dafna\n> >>>>\n> >>>>>\n> >>>>> Thanks\n> >>>>> j\n> >>>>>\n> >>>>>\n> >>>>>>\n> >>>>>> Thanks,\n> >>>>>> Dafna\n> >>>>>>\n> >>>>>>>\n> >>>>>>> Signed-off-by: Jacopo Mondi \n> >>>>>>> ---\n> >>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> >>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n> >>>>>>>\n> >>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> >>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n> >>>>>>> --- a/drivers/media/i2c/ov5647.c\n> >>>>>>> +++ b/drivers/media/i2c/ov5647.c\n> >>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n> >>>>>>> bool clock_ncont;\n> >>>>>>> struct v4l2_ctrl_handler ctrls;\n> >>>>>>> struct ov5647_mode *mode;\n> >>>>>>> + struct ov5647_mode *current_mode;\n> >>>>>>> };\n> >>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> >>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> >>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>> {\n> >>>>>>> struct i2c_client *client = v4l2_get_subdevdata(sd);\n> >>>>>>> + struct ov5647 *sensor = to_sensor(sd);\n> >>>>>>> u8 resetval, rdval;\n> >>>>>>> int ret;\n> >>>>>>> + if (sensor->mode == sensor->current_mode)\n> >>>>>>> + return 0;\n> >>>>>>> +\n> >>>>>>> ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> >>>>>>> if (ret < 0)\n> >>>>>>> return ret;\n> >>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>> return ret;\n> >>>>>>> }\n> >>>>>>> + sensor->current_mode = sensor->mode;\n> >>>>>>> +\n> >>>>>>> return 0;\n> >>>>>>> }\n> >>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> >>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>> {\n> >>>>>>> + struct ov5647 *sensor = to_sensor(sd);\n> >>>>>>> int ret;\n> >>>>>>> ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> >>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>> if (ret < 0)\n> >>>>>>> return ret;\n> >>>>>>> - return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>> + ret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>> + if (ret < 0)\n> >>>>>>> + return ret;\n> >>>>>>> +\n> >>>>>>> + sensor->current_mode = NULL;\n> >>>>>>> +\n> >>>>>>> + return 0;\n> >>>>>>> }\n> >>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 C45C5BFFE2\n\tfor ;\n\tTue, 30 Jun 2020 14:53:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4955560C56;\n\tTue, 30 Jun 2020 16:53:24 +0200 (CEST)","from mail-wm1-x343.google.com (mail-wm1-x343.google.com\n\t[IPv6:2a00:1450:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B1D1609C5\n\tfor ;\n\tTue, 30 Jun 2020 16:53:22 +0200 (CEST)","by mail-wm1-x343.google.com with SMTP id g75so19089899wme.5\n\tfor ;\n\tTue, 30 Jun 2020 07:53:22 -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=\"QFkotrxe\"; 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=vU8QJtiplqTmSqw8ypzasx0YbzBFm0BcQC4xxxr4HG8=;\n\tb=QFkotrxeRALEX9g7O3TGucCppkOo4DtRy3iZKCuQHnfKpsS3CKbq4QFvbl/swW/L1x\n\tjLBKMATVqXL7m5kGFn/958mWVAY/tzNagsfvKBgJKFxYmrO2wRyTe4qf8VWS2rlEDO12\n\tf808nRgr3jcdWVlWCISfY8vyxMrKsXd6bcHgN0pNu3qbF9ZhkvqIqw+uk4W6rzm3LZXw\n\txcTwwx8sjBpsi2evcSgk+jIWQnrUOAfxXjB81lqwsZfnr3Xg1XkD6yHkBBieyjRkA6Du\n\tc+SMsi2PcxAm95ENcETtphATMtazlo+jhXRpAuQ2DUFscjF4gsMPMYvETAR4fWV47U3H\n\tpEyg==","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=vU8QJtiplqTmSqw8ypzasx0YbzBFm0BcQC4xxxr4HG8=;\n\tb=rGQ6KHwrpMsfEkfOB5fMD129wB07eqYe/Jpa5Bx1zdO1lTJBlH1uRpXDmLDKl2fBAl\n\tx6u9aLRG9kJBlTsrAvckGQD/fMKVhLz/2evyps6d7odkrHOuMTPw6DNT9n7qM1jdG1Eb\n\tmS7cnXB1ya15FNexFaWDsRknAMQv0Tr0eM8cBWq0JN6j4cEbGyrwYbuvzJlfjWPJ3g8F\n\t7JFmP3pEUf4VLMf0bXSk0pk+TOhcGPUDyXh0DjxrhjFsaFdvj0k2qBCwCp+JX1OwZNQB\n\tApqefjgJOloML/y0z+I73DK6j/opwL1BMWdZh3tCg1LF8iP7I9AanzvIoo2OO/CaZNtf\n\tLKiA==","X-Gm-Message-State":"AOAM532g3m1QgxTqhcg9emR8LnuPoDF/bg1Huws9LtPeAzX4xUVdgfNg\n\t7IhslCXrjNtRyvQiBNToZtn+z4GG//K50csYiJpKOQ==","X-Google-Smtp-Source":"ABdhPJxVjn7hkQZM5GrsQu1KMVEr4+tZ7MRpq6N10mB2DFFWlDrXNyQ3iRUZh4jNmvfSDB2RAQYRmeXkrE01C85Gw0I=","X-Received":"by 2002:a05:600c:d7:: with SMTP id\n\tu23mr20682460wmm.183.1593528802015; \n\tTue, 30 Jun 2020 07:53:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Tue, 30 Jun 2020 15:53:06 +0100","Message-ID":"","To":"Dafna Hirschfeld ","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, Sakari Ailus , \n\tmrodin@de.adit-jv.com, Mauro Carvalho Chehab ,\n\tMaxime Ripard ,\n\tRoman Kovalivskyi ,\n\tlibcamera-devel@lists.libcamera.org, sudipi@jp.adit-jv.com,\n\tDave Stevenson , hugues.fruchet@st.com,\n\terosca@de.adit-jv.com, 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\" "}},{"id":11028,"web_url":"https://patchwork.libcamera.org/comment/11028/","msgid":"<887cd8b6-c82c-22b9-ce48-5881a8b51686@collabora.com>","date":"2020-06-30T15:11:28","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":46,"url":"https://patchwork.libcamera.org/api/people/46/","name":"Dafna Hirschfeld","email":"dafna.hirschfeld@collabora.com"},"content":"On 30.06.20 16:53, Dave Stevenson wrote:\n> Hi Dafna\n> \n> On Tue, 30 Jun 2020 at 14:01, Dafna Hirschfeld\n> wrote:\n>>\n>> Hi,\n>>\n>>\n>> On 30.06.20 14:05, Jacopo Mondi wrote:\n>>> Hi Dafna,\n>>>\n>>> On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n>>>>\n>>>>\n>>>> On 30.06.20 12:06, Jacopo Mondi wrote:\n>>>>> Hi Dafna,\n>>>>>\n>>>>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n>>>>>>\n>>>>>>\n>>>>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n>>>>>>> Hi Dafna,\n>>>>>>>\n>>>>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n>>>>>>>>\n>>>>>>>>\n>>>>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n>>>>>>>>> Store in the driver structure a pointer to the currently applied mode\n>>>>>>>>> and program a new one at s_stream(1) time only if it has changed.\n>>>>>>>>\n>>>>>>>> Hi,\n>>>>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n>>>>>>>> field.\n>>>>>>>\n>>>>>>> How would you like to use an 'is_streaming' flag to decide if the\n>>>>>>> sensor mode has to be updated ?\n>>>>>>\n>>>>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n>>>>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n>>>>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n>>>>>> device is currently streaming.\n>>>>>\n>>>>> No, the code returns immediately from ov5647_set_mode() if mode ==\n>>>>> current_mode. The modes comparison makes sure the sensor is not\n>>>>> reprogrammed if the mode hasn't changed. The remaning part of\n>>>>> s_stream() is executed regardless of the mode configuration. Am I\n>>>>> missing some part of the picture ?\n>>>>>\n>>>>>>\n>>>>>> But actually in this patch it seems to be possible to change the mode\n>>>>>> while streaming, if the callbacks are executed:\n>>>>>>\n>>>>>> s_stream(1)\n>>>>>> s_fmt\n>>>>>> s_stream(1)\n>>>>>>\n>>>>>> which is maybe a bug?\n>>>>>\n>>>>> The new format is stored in sensor->mode, and applied at the next\n>>>>> s_stream(1) operation if it differs from the already programmed one,\n>>>>> at least, this is how it is intended to work, have you found any\n>>>>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n>>>>\n>>>> What I meant is that there could be valid sequence of calls\n>>>>\n>>>> s_stream(enable=1)\n>>>> s_fmt\n>>>> s_stream(enable=1)\n>>>>\n>>>> For example if two video devices are connected to the sensor and they\n>>>> stream simultaneously. There was a discussion about adding a code to the core\n>>>\n>>> I'm not sure it is possible, given that the subdev has a single source\n>>> pad\n>>\n>> Video devices should not be connected directly to the sensor,\n> \n> That's an odd statement as it is exactly the situation we have on the\n> Pi. The CSI2 receiver writes data to RAM, therefore it is a video\n> device.\n> Did you intend to say that it isn't necessarily connected directly to\n> the sensor?\nYes, sorry, I meant, \"they don't have to\" (but they can).\n\n> \n>> they can also\n>> be connected to the sensor through an isp entity that is connected to the sensor\n>> from one side and to two video devices from the other side.\n> \n> It's true that some platforms will route the received CSI2 data\n> straight through the ISP, and only write the processed image(s) to\n> RAM. If there are multiple output video devices from the ISP then yes\n> they will VIDIOC_STREAMON at different points.\n> However is it really valid to call s_stream(1) on the sensor subdev\n> for each of them? Doesn't that mean you really need refcounting of the\n> number of s_stream(1)'s (and s_stream(0)'s) within each sensor driver,\n> so that it only actually stops streaming on the last s_stream(0), not\n> the first. A simple boolean isn't sufficient, otherwise\n> VIDIOC_STREAMON(NODE_A);\n> VIDIOC_STREAMON(NODE_B);\n> VIDIOC_STREAMOFF(NODE_B);\n> leaves you with no data from NODE_A even though it has never called\n> VIDIOC_STREAMOFF.\n\noh, right, there was a proposal to add functions that do it with refcounting\n\nhttps://patchwork.kernel.org/project/linux-media/list/?series=271153\n\nDafna\n\n\n> \n> Anyway this patch was to remove excess writing of the mode registers if you did\n> s_fmt\n> s_stream(1)\n> s_stream(0)\n> s_stream(1)\n> \n> The driver uses the s_power call rather than pm_runtime as that was an\n> acceptable approach when it was written in 2017, so the power, and\n> hence register settings, can be maintained between multiple s_stream\n> calls.\n> \n> Dave\n> \n>> Thanks,\n>> D\n>>\n>>>\n>>>> to follow the s_stream callback and call it only if the subdev is not streaming\n>>>> but currently subdevs should support it themselves.\n>>>>\n>>>\n>>> Oh, so you're concerned about the fact userspace can call s_stream(1)\n>>> twice consecutively! it's indipendent from s_ftm if I got your point.\n>>>\n>>> In this case a flag to control if the device is streaming already should\n>>> help, yes, I overlooked that indeed.\n>>>\n>>> Thanks\n>>> j\n>>>\n>>>>\n>>>> Thanks,\n>>>> Dafna\n>>>>\n>>>>>\n>>>>> Thanks\n>>>>> j\n>>>>>>\n>>>>>> Thanks,\n>>>>>> Dafna\n>>>>>>\n>>>>>>>\n>>>>>>> Thanks\n>>>>>>> j\n>>>>>>>\n>>>>>>>\n>>>>>>>>\n>>>>>>>> Thanks,\n>>>>>>>> Dafna\n>>>>>>>>\n>>>>>>>>>\n>>>>>>>>> Signed-off-by: Jacopo Mondi \n>>>>>>>>> ---\n>>>>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n>>>>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n>>>>>>>>>\n>>>>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n>>>>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n>>>>>>>>> --- a/drivers/media/i2c/ov5647.c\n>>>>>>>>> +++ b/drivers/media/i2c/ov5647.c\n>>>>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n>>>>>>>>> bool clock_ncont;\n>>>>>>>>> struct v4l2_ctrl_handler ctrls;\n>>>>>>>>> struct ov5647_mode *mode;\n>>>>>>>>> + struct ov5647_mode *current_mode;\n>>>>>>>>> };\n>>>>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n>>>>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n>>>>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>>>>>> {\n>>>>>>>>> struct i2c_client *client = v4l2_get_subdevdata(sd);\n>>>>>>>>> + struct ov5647 *sensor = to_sensor(sd);\n>>>>>>>>> u8 resetval, rdval;\n>>>>>>>>> int ret;\n>>>>>>>>> + if (sensor->mode == sensor->current_mode)\n>>>>>>>>> + return 0;\n>>>>>>>>> +\n>>>>>>>>> ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n>>>>>>>>> if (ret < 0)\n>>>>>>>>> return ret;\n>>>>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n>>>>>>>>> return ret;\n>>>>>>>>> }\n>>>>>>>>> + sensor->current_mode = sensor->mode;\n>>>>>>>>> +\n>>>>>>>>> return 0;\n>>>>>>>>> }\n>>>>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n>>>>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>>>>>> {\n>>>>>>>>> + struct ov5647 *sensor = to_sensor(sd);\n>>>>>>>>> int ret;\n>>>>>>>>> ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n>>>>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n>>>>>>>>> if (ret < 0)\n>>>>>>>>> return ret;\n>>>>>>>>> - return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>>>>>> + ret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n>>>>>>>>> + if (ret < 0)\n>>>>>>>>> + return ret;\n>>>>>>>>> +\n>>>>>>>>> + sensor->current_mode = NULL;\n>>>>>>>>> +\n>>>>>>>>> + return 0;\n>>>>>>>>> }\n>>>>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 EB02EBF415\n\tfor ;\n\tTue, 30 Jun 2020 15:11:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F60D60C56;\n\tTue, 30 Jun 2020 17:11:34 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08AAA609C5\n\tfor ;\n\tTue, 30 Jun 2020 17:11:33 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: dafna) with ESMTPSA id 165712A0938"],"To":"Dave Stevenson ","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>\n\t\n\t","From":"Dafna Hirschfeld ","Message-ID":"<887cd8b6-c82c-22b9-ce48-5881a8b51686@collabora.com>","Date":"Tue, 30 Jun 2020 17:11:28 +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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, Sakari Ailus , \n\tmrodin@de.adit-jv.com, Mauro Carvalho Chehab ,\n\tMaxime Ripard ,\n\tRoman Kovalivskyi ,\n\tlibcamera-devel@lists.libcamera.org, sudipi@jp.adit-jv.com,\n\tDave Stevenson , hugues.fruchet@st.com,\n\terosca@de.adit-jv.com, aford173@gmail.com,\n\tLinux Media Mailing List ","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":11032,"web_url":"https://patchwork.libcamera.org/comment/11032/","msgid":"<20200701072554.GH5963@pendragon.ideasonboard.com>","date":"2020-07-01T07:25:54","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nOn Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:\n> On 30.06.20 14:05, Jacopo Mondi wrote:\n> > On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n> >> On 30.06.20 12:06, Jacopo Mondi wrote:\n> >>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n> >>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n> >>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> >>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n> >>>>>>> Store in the driver structure a pointer to the currently applied mode\n> >>>>>>> and program a new one at s_stream(1) time only if it has changed.\n> >>>>>>\n> >>>>>> Hi,\n> >>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n> >>>>>> field.\n> >>>>>\n> >>>>> How would you like to use an 'is_streaming' flag to decide if the\n> >>>>> sensor mode has to be updated ?\n> >>>>\n> >>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> >>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> >>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> >>>> device is currently streaming.\n> >>>\n> >>> No, the code returns immediately from ov5647_set_mode() if mode ==\n> >>> current_mode. The modes comparison makes sure the sensor is not\n> >>> reprogrammed if the mode hasn't changed. The remaning part of\n> >>> s_stream() is executed regardless of the mode configuration. Am I\n> >>> missing some part of the picture ?\n> >>>\n> >>>> But actually in this patch it seems to be possible to change the mode\n> >>>> while streaming, if the callbacks are executed:\n> >>>>\n> >>>> s_stream(1)\n> >>>> s_fmt\n> >>>> s_stream(1)\n> >>>>\n> >>>> which is maybe a bug?\n> >>>\n> >>> The new format is stored in sensor->mode, and applied at the next\n> >>> s_stream(1) operation if it differs from the already programmed one,\n> >>> at least, this is how it is intended to work, have you found any\n> >>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n> >>\n> >> What I meant is that there could be valid sequence of calls\n> >>\n> >> s_stream(enable=1)\n> >> s_fmt\n> >> s_stream(enable=1)\n> >>\n> >> For example if two video devices are connected to the sensor and they\n> >> stream simultaneously. There was a discussion about adding a code to the core\n> > \n> > I'm not sure it is possible, given that the subdev has a single source\n> > pad\n> \n> Video devices should not be connected directly to the sensor, they can also\n> be connected to the sensor through an isp entity that is connected to the sensor\n> from one side and to two video devices from the other side.\n\nI don't think it should be the job of the sensor driver to handle this.\nA sensor can be streaming or not streaming, and a .s_stream(1) call\nwhile already streaming shouldn't happen. It's the job of the ISP driver\n(with help from core code) to ensure this won't happen. Otherwise we\nwould have to protect against that case in all sensor drivers,\nduplicating code in many places and opening the door to bugs. Subdev\ndrivers should be as simple as possible.\n\n> >> to follow the s_stream callback and call it only if the subdev is not streaming\n> >> but currently subdevs should support it themselves.\n> > \n> > Oh, so you're concerned about the fact userspace can call s_stream(1)\n> > twice consecutively! it's indipendent from s_ftm if I got your point.\n> > \n> > In this case a flag to control if the device is streaming already should\n> > help, yes, I overlooked that indeed.\n> > \n> >>>>>>> Signed-off-by: Jacopo Mondi \n> >>>>>>> ---\n> >>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> >>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n> >>>>>>>\n> >>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> >>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n> >>>>>>> --- a/drivers/media/i2c/ov5647.c\n> >>>>>>> +++ b/drivers/media/i2c/ov5647.c\n> >>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n> >>>>>>> \tbool\t\t\t\tclock_ncont;\n> >>>>>>> \tstruct v4l2_ctrl_handler\tctrls;\n> >>>>>>> \tstruct ov5647_mode\t\t*mode;\n> >>>>>>> +\tstruct ov5647_mode\t\t*current_mode;\n> >>>>>>> };\n> >>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> >>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> >>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>> {\n> >>>>>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> >>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> >>>>>>> \tu8 resetval, rdval;\n> >>>>>>> \tint ret;\n> >>>>>>> +\tif (sensor->mode == sensor->current_mode)\n> >>>>>>> +\t\treturn 0;\n> >>>>>>> +\n> >>>>>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> >>>>>>> \tif (ret < 0)\n> >>>>>>> \t\treturn ret;\n> >>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>> \t\t\treturn ret;\n> >>>>>>> \t}\n> >>>>>>> +\tsensor->current_mode = sensor->mode;\n> >>>>>>> +\n> >>>>>>> \treturn 0;\n> >>>>>>> }\n> >>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> >>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>> {\n> >>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> >>>>>>> \tint ret;\n> >>>>>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> >>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>> \tif (ret < 0)\n> >>>>>>> \t\treturn ret;\n> >>>>>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>> +\tif (ret < 0)\n> >>>>>>> +\t\treturn ret;\n> >>>>>>> +\n> >>>>>>> +\tsensor->current_mode = NULL;\n> >>>>>>> +\n> >>>>>>> +\treturn 0;\n> >>>>>>> }\n> >>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 A39AEBF415\n\tfor ;\n\tWed, 1 Jul 2020 07:26:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34F8D60C56;\n\tWed, 1 Jul 2020 09:26:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02660603B3\n\tfor ;\n\tWed, 1 Jul 2020 09:25:58 +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 2158659E;\n\tWed, 1 Jul 2020 09:25:58 +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=\"htALq97+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593588358;\n\tbh=Eqp5U2r/3PhlV+mZUmW603MzGVvLhq3URqVnjnFQP9M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=htALq97+gicZcDZCY4iflI9BHK0Q0spwnQ0PRlHdBF25HzeqymJYKMCKpcikZi4G9\n\t5fA/3xZgmLdIrdEgzIo7/x7AoRAIxTbx7UwyYeg3ZAkbiE4O6Mqob5+dzvPrBJPPqr\n\t2M9glkzoZ6eWYh2u6J+vg3yMQYI/wzgKOp/ck1F8=","Date":"Wed, 1 Jul 2020 10:25:54 +0300","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"<20200701072554.GH5963@pendragon.ideasonboard.com>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, dave.stevenson@raspberrypi.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tsudipi@jp.adit-jv.com, sakari.ailus@linux.intel.com,\n\thugues.fruchet@st.com, \n\tmchehab@kernel.org, aford173@gmail.com, linux-media@vger.kernel.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11155,"web_url":"https://patchwork.libcamera.org/comment/11155/","msgid":"<20200703122150.7mrsl3sw3hblcldv@uno.localdomain>","date":"2020-07-03T12:21:50","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Wed, Jul 01, 2020 at 10:25:54AM +0300, Laurent Pinchart wrote:\n> Hi Dafna,\n>\n> On Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:\n> > On 30.06.20 14:05, Jacopo Mondi wrote:\n> > > On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n> > >> On 30.06.20 12:06, Jacopo Mondi wrote:\n> > >>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n> > >>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n> > >>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> > >>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n> > >>>>>>> Store in the driver structure a pointer to the currently applied mode\n> > >>>>>>> and program a new one at s_stream(1) time only if it has changed.\n> > >>>>>>\n> > >>>>>> Hi,\n> > >>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n> > >>>>>> field.\n> > >>>>>\n> > >>>>> How would you like to use an 'is_streaming' flag to decide if the\n> > >>>>> sensor mode has to be updated ?\n> > >>>>\n> > >>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> > >>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> > >>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> > >>>> device is currently streaming.\n> > >>>\n> > >>> No, the code returns immediately from ov5647_set_mode() if mode ==\n> > >>> current_mode. The modes comparison makes sure the sensor is not\n> > >>> reprogrammed if the mode hasn't changed. The remaning part of\n> > >>> s_stream() is executed regardless of the mode configuration. Am I\n> > >>> missing some part of the picture ?\n> > >>>\n> > >>>> But actually in this patch it seems to be possible to change the mode\n> > >>>> while streaming, if the callbacks are executed:\n> > >>>>\n> > >>>> s_stream(1)\n> > >>>> s_fmt\n> > >>>> s_stream(1)\n> > >>>>\n> > >>>> which is maybe a bug?\n> > >>>\n> > >>> The new format is stored in sensor->mode, and applied at the next\n> > >>> s_stream(1) operation if it differs from the already programmed one,\n> > >>> at least, this is how it is intended to work, have you found any\n> > >>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n> > >>\n> > >> What I meant is that there could be valid sequence of calls\n> > >>\n> > >> s_stream(enable=1)\n> > >> s_fmt\n> > >> s_stream(enable=1)\n> > >>\n> > >> For example if two video devices are connected to the sensor and they\n> > >> stream simultaneously. There was a discussion about adding a code to the core\n> > >\n> > > I'm not sure it is possible, given that the subdev has a single source\n> > > pad\n> >\n> > Video devices should not be connected directly to the sensor, they can also\n> > be connected to the sensor through an isp entity that is connected to the sensor\n> > from one side and to two video devices from the other side.\n>\n> I don't think it should be the job of the sensor driver to handle this.\n> A sensor can be streaming or not streaming, and a .s_stream(1) call\n> while already streaming shouldn't happen. It's the job of the ISP driver\n> (with help from core code) to ensure this won't happen. Otherwise we\n> would have to protect against that case in all sensor drivers,\n> duplicating code in many places and opening the door to bugs. Subdev\n> drivers should be as simple as possible.\n>\n\nMost of the sensor driver I've briefly looked at implement a simple\ncheck to avoid double stream(1) but they do not implement any form of\nrefcounting. I think that does more harm than good to be honest, as it\nwould hide potential problematic start stream sequences, but would\nstop the sensor at the first stream(0), leaving one of the multiple\nreceivers stuck.\n\nI would prefer avoid doing this here.\n\nHowever the driver already refcounting on s_power(), which if I'm not\nmistaken could be avoided, as bridges should use\nv4l2_pipeline_pm_get(), which already does refcounting, if I'm not\nmistaken.\n\nTo me, grasping how s_stream() and s_start() work for real is still hard,\nas those are the only two operation propagated along the pipeline by\nbriges, even for MC platforms, and it seems looking at the existing\ndriver, the confusion is big, as all of them handle things slightly\ndifferently :/\n\n\n> > >> to follow the s_stream callback and call it only if the subdev is not streaming\n> > >> but currently subdevs should support it themselves.\n> > >\n> > > Oh, so you're concerned about the fact userspace can call s_stream(1)\n> > > twice consecutively! it's indipendent from s_ftm if I got your point.\n> > >\n> > > In this case a flag to control if the device is streaming already should\n> > > help, yes, I overlooked that indeed.\n> > >\n> > >>>>>>> Signed-off-by: Jacopo Mondi \n> > >>>>>>> ---\n> > >>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> > >>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n> > >>>>>>>\n> > >>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> > >>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n> > >>>>>>> --- a/drivers/media/i2c/ov5647.c\n> > >>>>>>> +++ b/drivers/media/i2c/ov5647.c\n> > >>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n> > >>>>>>> \tbool\t\t\t\tclock_ncont;\n> > >>>>>>> \tstruct v4l2_ctrl_handler\tctrls;\n> > >>>>>>> \tstruct ov5647_mode\t\t*mode;\n> > >>>>>>> +\tstruct ov5647_mode\t\t*current_mode;\n> > >>>>>>> };\n> > >>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> > >>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> > >>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > >>>>>>> {\n> > >>>>>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> > >>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> > >>>>>>> \tu8 resetval, rdval;\n> > >>>>>>> \tint ret;\n> > >>>>>>> +\tif (sensor->mode == sensor->current_mode)\n> > >>>>>>> +\t\treturn 0;\n> > >>>>>>> +\n> > >>>>>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> > >>>>>>> \tif (ret < 0)\n> > >>>>>>> \t\treturn ret;\n> > >>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> > >>>>>>> \t\t\treturn ret;\n> > >>>>>>> \t}\n> > >>>>>>> +\tsensor->current_mode = sensor->mode;\n> > >>>>>>> +\n> > >>>>>>> \treturn 0;\n> > >>>>>>> }\n> > >>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> > >>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > >>>>>>> {\n> > >>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> > >>>>>>> \tint ret;\n> > >>>>>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> > >>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> > >>>>>>> \tif (ret < 0)\n> > >>>>>>> \t\treturn ret;\n> > >>>>>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > >>>>>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> > >>>>>>> +\tif (ret < 0)\n> > >>>>>>> +\t\treturn ret;\n> > >>>>>>> +\n> > >>>>>>> +\tsensor->current_mode = NULL;\n> > >>>>>>> +\n> > >>>>>>> +\treturn 0;\n> > >>>>>>> }\n> > >>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\n> > >>>>>>>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 39267BE905\n\tfor ;\n\tFri, 3 Jul 2020 12:18:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A80DA60C5C;\n\tFri, 3 Jul 2020 14:18:28 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80848603AE\n\tfor ;\n\tFri, 3 Jul 2020 14:18:27 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id C3EF84000D;\n\tFri, 3 Jul 2020 12:18:21 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 3 Jul 2020 14:21:50 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200703122150.7mrsl3sw3hblcldv@uno.localdomain>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>\n\t\n\t<20200701072554.GH5963@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701072554.GH5963@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, sakari.ailus@linux.intel.com,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tsudipi@jp.adit-jv.com, dave.stevenson@raspberrypi.org,\n\thugues.fruchet@st.com, \n\tmchehab@kernel.org, aford173@gmail.com, linux-media@vger.kernel.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11163,"web_url":"https://patchwork.libcamera.org/comment/11163/","msgid":"<20200703163318.GF14255@pendragon.ideasonboard.com>","date":"2020-07-03T16:33:18","subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jul 03, 2020 at 02:21:50PM +0200, Jacopo Mondi wrote:\n> On Wed, Jul 01, 2020 at 10:25:54AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:\n> >> On 30.06.20 14:05, Jacopo Mondi wrote:\n> >>> On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:\n> >>>> On 30.06.20 12:06, Jacopo Mondi wrote:\n> >>>>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:\n> >>>>>> On 30.06.20 09:43, Jacopo Mondi wrote:\n> >>>>>>> On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:\n> >>>>>>>> On 23.06.20 18:55, Jacopo Mondi wrote:\n> >>>>>>>>> Store in the driver structure a pointer to the currently applied mode\n> >>>>>>>>> and program a new one at s_stream(1) time only if it has changed.\n> >>>>>>>>\n> >>>>>>>> Hi,\n> >>>>>>>> I think this can be more readably implemented with a 'is_streaming' boolean\n> >>>>>>>> field.\n> >>>>>>>\n> >>>>>>> How would you like to use an 'is_streaming' flag to decide if the\n> >>>>>>> sensor mode has to be updated ?\n> >>>>>>\n> >>>>>> since 'current_mode' is set to NULL upon `ov5647_stream_off`\n> >>>>>> and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'\n> >>>>>> it seem very similar to returning immediately from 'ov5647_set_stream' if the\n> >>>>>> device is currently streaming.\n> >>>>>\n> >>>>> No, the code returns immediately from ov5647_set_mode() if mode ==\n> >>>>> current_mode. The modes comparison makes sure the sensor is not\n> >>>>> reprogrammed if the mode hasn't changed. The remaning part of\n> >>>>> s_stream() is executed regardless of the mode configuration. Am I\n> >>>>> missing some part of the picture ?\n> >>>>>\n> >>>>>> But actually in this patch it seems to be possible to change the mode\n> >>>>>> while streaming, if the callbacks are executed:\n> >>>>>>\n> >>>>>> s_stream(1)\n> >>>>>> s_fmt\n> >>>>>> s_stream(1)\n> >>>>>>\n> >>>>>> which is maybe a bug?\n> >>>>>\n> >>>>> The new format is stored in sensor->mode, and applied at the next\n> >>>>> s_stream(1) operation if it differs from the already programmed one,\n> >>>>> at least, this is how it is intended to work, have you found any\n> >>>>> failing s_stream/set_fmt/s_stream which could be caused by a bug ?\n> >>>>\n> >>>> What I meant is that there could be valid sequence of calls\n> >>>>\n> >>>> s_stream(enable=1)\n> >>>> s_fmt\n> >>>> s_stream(enable=1)\n> >>>>\n> >>>> For example if two video devices are connected to the sensor and they\n> >>>> stream simultaneously. There was a discussion about adding a code to the core\n> >>>\n> >>> I'm not sure it is possible, given that the subdev has a single source\n> >>> pad\n> >>\n> >> Video devices should not be connected directly to the sensor, they can also\n> >> be connected to the sensor through an isp entity that is connected to the sensor\n> >> from one side and to two video devices from the other side.\n> >\n> > I don't think it should be the job of the sensor driver to handle this.\n> > A sensor can be streaming or not streaming, and a .s_stream(1) call\n> > while already streaming shouldn't happen. It's the job of the ISP driver\n> > (with help from core code) to ensure this won't happen. Otherwise we\n> > would have to protect against that case in all sensor drivers,\n> > duplicating code in many places and opening the door to bugs. Subdev\n> > drivers should be as simple as possible.\n> \n> Most of the sensor driver I've briefly looked at implement a simple\n> check to avoid double stream(1) but they do not implement any form of\n> refcounting. I think that does more harm than good to be honest, as it\n> would hide potential problematic start stream sequences, but would\n> stop the sensor at the first stream(0), leaving one of the multiple\n> receivers stuck.\n> \n> I would prefer avoid doing this here.\n> \n> However the driver already refcounting on s_power(), which if I'm not\n> mistaken could be avoided, as bridges should use\n> v4l2_pipeline_pm_get(), which already does refcounting, if I'm not\n> mistaken.\n\n.s_power() can also be called when opening the subdev device node from\nuserspace, through sd->internal_ops->open(). In new drivers, I'd\nrecommend implementing .s_power() based on runtime PM, with .s_power(1)\ncalling pm_runtime_get_sync() and .s_power(0) calling pm_runtime_put().\n.s_stream() should call the runtime PM functions too. That way all the\nrefcounting will be handled by runtime PM.\n\n.s_stream() should not be refcounted, the caller should ensure that a\nstarted sensor doesn't get started again and that a stopped sensor\ndoesn't receive a .s_stream(0) call.\n\n> To me, grasping how s_stream() and s_start() work for real is still hard,\n> as those are the only two operation propagated along the pipeline by\n> briges, even for MC platforms, and it seems looking at the existing\n> driver, the confusion is big, as all of them handle things slightly\n> differently :/\n\nNone of this has ever been really documented, and APIs have evolved over\ntime without fixing drivers, hence today's mess.\n\n> >>>> to follow the s_stream callback and call it only if the subdev is not streaming\n> >>>> but currently subdevs should support it themselves.\n> >>>\n> >>> Oh, so you're concerned about the fact userspace can call s_stream(1)\n> >>> twice consecutively! it's indipendent from s_ftm if I got your point.\n> >>>\n> >>> In this case a flag to control if the device is streaming already should\n> >>> help, yes, I overlooked that indeed.\n> >>>\n> >>>>>>>>> Signed-off-by: Jacopo Mondi \n> >>>>>>>>> ---\n> >>>>>>>>> drivers/media/i2c/ov5647.c | 16 +++++++++++++++-\n> >>>>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)\n> >>>>>>>>>\n> >>>>>>>>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> >>>>>>>>> index 39e320f321bd8..ac114269e1c73 100644\n> >>>>>>>>> --- a/drivers/media/i2c/ov5647.c\n> >>>>>>>>> +++ b/drivers/media/i2c/ov5647.c\n> >>>>>>>>> @@ -96,6 +96,7 @@ struct ov5647 {\n> >>>>>>>>> \tbool\t\t\t\tclock_ncont;\n> >>>>>>>>> \tstruct v4l2_ctrl_handler\tctrls;\n> >>>>>>>>> \tstruct ov5647_mode\t\t*mode;\n> >>>>>>>>> +\tstruct ov5647_mode\t\t*current_mode;\n> >>>>>>>>> };\n> >>>>>>>>> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> >>>>>>>>> @@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)\n> >>>>>>>>> static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>>>> {\n> >>>>>>>>> \tstruct i2c_client *client = v4l2_get_subdevdata(sd);\n> >>>>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> >>>>>>>>> \tu8 resetval, rdval;\n> >>>>>>>>> \tint ret;\n> >>>>>>>>> +\tif (sensor->mode == sensor->current_mode)\n> >>>>>>>>> +\t\treturn 0;\n> >>>>>>>>> +\n> >>>>>>>>> \tret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);\n> >>>>>>>>> \tif (ret < 0)\n> >>>>>>>>> \t\treturn ret;\n> >>>>>>>>> @@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> >>>>>>>>> \t\t\treturn ret;\n> >>>>>>>>> \t}\n> >>>>>>>>> +\tsensor->current_mode = sensor->mode;\n> >>>>>>>>> +\n> >>>>>>>>> \treturn 0;\n> >>>>>>>>> }\n> >>>>>>>>> @@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)\n> >>>>>>>>> static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>>>> {\n> >>>>>>>>> +\tstruct ov5647 *sensor = to_sensor(sd);\n> >>>>>>>>> \tint ret;\n> >>>>>>>>> \tret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |\n> >>>>>>>>> @@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)\n> >>>>>>>>> \tif (ret < 0)\n> >>>>>>>>> \t\treturn ret;\n> >>>>>>>>> -\treturn ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>>>> +\tret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);\n> >>>>>>>>> +\tif (ret < 0)\n> >>>>>>>>> +\t\treturn ret;\n> >>>>>>>>> +\n> >>>>>>>>> +\tsensor->current_mode = NULL;\n> >>>>>>>>> +\n> >>>>>>>>> +\treturn 0;\n> >>>>>>>>> }\n> >>>>>>>>> static int set_sw_standby(struct v4l2_subdev *sd, bool standby)\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 D084FBFFE2\n\tfor ;\n\tFri, 3 Jul 2020 16:33:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44A3360CAE;\n\tFri, 3 Jul 2020 18:33:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B2A3609A9\n\tfor ;\n\tFri, 3 Jul 2020 18:33:24 +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 02D8529E;\n\tFri, 3 Jul 2020 18:33:22 +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=\"rlpj94BX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593794003;\n\tbh=Nwh0YN0KeqC0InYjn2BqTnLR9TGdn4HVickM9LbHxRY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rlpj94BXVfWK3mFgx5+5HjqPxdUp01z3HFl0V9wyDfgH+Qc5CGkIlnr128MVJnjKT\n\tJp8A7iypgOSUL3ye1A/W0MVXg3d+BBVoPoU76HFmt9Ou6+OetRj/jfJk+X9dag7ZEE\n\t59yNJ6iPYF6nXCoBQf+5pQ+SauWjDj1cLaQKEsWs=","Date":"Fri, 3 Jul 2020 19:33:18 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200703163318.GF14255@pendragon.ideasonboard.com>","References":"<20200623165550.45835-1-jacopo@jmondi.org>\n\t<80139e40-914f-c547-922f-91fe3f611202@collabora.com>\n\t<20200630074305.soctqoaqirfdnvv2@uno.localdomain>\n\t\n\t<20200630100651.ikjcazgbvoq2hab4@uno.localdomain>\n\t\n\t<20200630120528.xffvivfriblc6a2y@uno.localdomain>\n\t\n\t<20200701072554.GH5963@pendragon.ideasonboard.com>\n\t<20200703122150.7mrsl3sw3hblcldv@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703122150.7mrsl3sw3hblcldv@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 20/25] media: ov5647: Program mode\n\tonly if it has changed","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, sakari.ailus@linux.intel.com,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tsudipi@jp.adit-jv.com, dave.stevenson@raspberrypi.org,\n\thugues.fruchet@st.com, \n\tmchehab@kernel.org, aford173@gmail.com, linux-media@vger.kernel.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]