[{"id":13942,"web_url":"https://patchwork.libcamera.org/comment/13942/","msgid":"<20201127095608.dyjccfp26gmmzecn@uno.localdomain>","date":"2020-11-27T09:56:08","subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Nov 23, 2020 at 11:12:31PM +0100, Niklas Söderlund wrote:\n> Expose the optional DelayedControls interface to pipeline handlers. If\n> used by a pipeline generic delays are used. In the future the delay\n> values should be fetched to match the specific sensor module, either\n> from a new kernel interface or worst case a sensors database.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/internal/camera_sensor.h |  5 ++++\n>  src/libcamera/camera_sensor.cpp            | 31 ++++++++++++++++++++++\n>  2 files changed, 36 insertions(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index b9eba89f00fba882..6be1cfd49134c534 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -14,6 +14,7 @@\n>  #include <libcamera/controls.h>\n>  #include <libcamera/geometry.h>\n>\n> +#include \"libcamera/internal/delayed_controls.h\"\n>  #include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/log.h\"\n>  #include \"libcamera/internal/v4l2_subdevice.h\"\n> @@ -61,6 +62,8 @@ public:\n>  \tControlList getControls(const std::vector<uint32_t> &ids);\n>  \tint setControls(ControlList *ctrls);\n>\n> +\tDelayedControls *delayedContols();\n> +\n>  \tconst ControlList &properties() const { return properties_; }\n>  \tint sensorInfo(CameraSensorInfo *info) const;\n>\n> @@ -83,6 +86,8 @@ private:\n>  \tstd::vector<Size> sizes_;\n>\n>  \tControlList properties_;\n> +\n> +\tstd::unique_ptr<DelayedControls> delayedControls_;\n>  };\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 935de528c4963453..6c92c97f4cc2547a 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)\n>  \treturn subdev_->setControls(ctrls);\n>  }\n>\n> +/**\n> + * \\brief Get the sensors delayed controls interface\n> + *\n> + * Access the sensors delayed controls interface. This interface aids pipelines\n> + * keeping tack of controls that needs time to take effect. The interface is\n> + * optional to use and does not interact with setControls() and getControls()\n> + * that operates directly on the sensor device.\n> + *\n> + * \\sa DelayedControls\n> + * \\return The delayed controls interface\n> + */\n> +DelayedControls *CameraSensor::delayedContols()\n> +{\n> +\tif (!delayedControls_) {\n> +\t\t/*\n> +\t\t * \\todo Read dealy values from the sensor itself or from a\n> +\t\t * a sensor database. For now use generic values taken from\n> +\t\t * the Raspberry Pi and listed as generic values.\n> +\t\t */\n> +\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n> +\t\t\t{ V4L2_CID_ANALOGUE_GAIN, 1 },\n> +\t\t\t{ V4L2_CID_EXPOSURE, 2 },\n> +\t\t};\n\nI'm debated.\n\nAs I see this, the current interface will have to evolve, on top of\nthis series. I think pipeline handler will not have to get a\n'DelayedControls' object and interface with it, but rather the\nCameraSensor will use it internally for either:\n- all controls marked as delayed in the sensor database\n- controls initialized as delayed by the pipeline handler (which won't\n  be sensor-agnostic anymore)\n\nThe current implementation aims to be a drop-in replacement for\nStaggeredCtrl so I'm more than fine with the proposed enanchement\nbeing performed on top. At the same time as you know this specific\nbits will break ov5647 capture on RPi, and I don't think that's right.\n\nAs a possible plan I would propose:\n- replace staggered ctrls with this series and allow pipeline handler\n  to pass in delays to maintain 1-to-1 compatibility with the existing\n- grow the sensor database and the CameraSensor interface to remove\n  direct pipeline->DelayedControl interactions on top\n\nAm I missing a valid use case for pipeline handler having to deal with\nDelayedControls directly ?\n\n> +\n> +\t\tdelayedControls_ =\n> +\t\t\tstd::make_unique<DelayedControls>(subdev_.get(), delays);\n> +\t}\n> +\n> +\treturn delayedControls_.get();\n> +}\n> +\n>  /**\n>   * \\brief Assemble and return the camera sensor info\n>   * \\param[out] info The camera sensor info\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 604EDBE176\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 27 Nov 2020 09:56:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB1B06346E;\n\tFri, 27 Nov 2020 10:56:04 +0100 (CET)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7E8D6345C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 27 Nov 2020 10:56:03 +0100 (CET)","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 E2725200005;\n\tFri, 27 Nov 2020 09:56:02 +0000 (UTC)"],"Date":"Fri, 27 Nov 2020 10:56:08 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20201127095608.dyjccfp26gmmzecn@uno.localdomain>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123221234.485933-6-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14068,"web_url":"https://patchwork.libcamera.org/comment/14068/","msgid":"<20201205013912.GQ4109@pendragon.ideasonboard.com>","date":"2020-12-05T01:39:12","subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Niklas,\n\nOn Fri, Nov 27, 2020 at 10:56:08AM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 23, 2020 at 11:12:31PM +0100, Niklas Söderlund wrote:\n> > Expose the optional DelayedControls interface to pipeline handlers. If\n> > used by a pipeline generic delays are used. In the future the delay\n> > values should be fetched to match the specific sensor module, either\n> > from a new kernel interface or worst case a sensors database.\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/internal/camera_sensor.h |  5 ++++\n> >  src/libcamera/camera_sensor.cpp            | 31 ++++++++++++++++++++++\n> >  2 files changed, 36 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index b9eba89f00fba882..6be1cfd49134c534 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -14,6 +14,7 @@\n> >  #include <libcamera/controls.h>\n> >  #include <libcamera/geometry.h>\n> >\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> >  #include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/log.h\"\n> >  #include \"libcamera/internal/v4l2_subdevice.h\"\n> > @@ -61,6 +62,8 @@ public:\n> >  \tControlList getControls(const std::vector<uint32_t> &ids);\n> >  \tint setControls(ControlList *ctrls);\n> >\n> > +\tDelayedControls *delayedContols();\n\ns/delayedContols/delayedControls/\n\n> > +\n> >  \tconst ControlList &properties() const { return properties_; }\n> >  \tint sensorInfo(CameraSensorInfo *info) const;\n> >\n> > @@ -83,6 +86,8 @@ private:\n> >  \tstd::vector<Size> sizes_;\n> >\n> >  \tControlList properties_;\n> > +\n> > +\tstd::unique_ptr<DelayedControls> delayedControls_;\n> >  };\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 935de528c4963453..6c92c97f4cc2547a 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)\n> >  \treturn subdev_->setControls(ctrls);\n> >  }\n> >\n> > +/**\n> > + * \\brief Get the sensors delayed controls interface\n> > + *\n> > + * Access the sensors delayed controls interface. This interface aids pipelines\n> > + * keeping tack of controls that needs time to take effect. The interface is\n> > + * optional to use and does not interact with setControls() and getControls()\n> > + * that operates directly on the sensor device.\n> > + *\n> > + * \\sa DelayedControls\n> > + * \\return The delayed controls interface\n> > + */\n> > +DelayedControls *CameraSensor::delayedContols()\n> > +{\n> > +\tif (!delayedControls_) {\n> > +\t\t/*\n> > +\t\t * \\todo Read dealy values from the sensor itself or from a\n> > +\t\t * a sensor database. For now use generic values taken from\n> > +\t\t * the Raspberry Pi and listed as generic values.\n> > +\t\t */\n> > +\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n> > +\t\t\t{ V4L2_CID_ANALOGUE_GAIN, 1 },\n> > +\t\t\t{ V4L2_CID_EXPOSURE, 2 },\n> > +\t\t};\n> \n> I'm debated.\n> \n> As I see this, the current interface will have to evolve, on top of\n> this series. I think pipeline handler will not have to get a\n> 'DelayedControls' object and interface with it, but rather the\n> CameraSensor will use it internally for either:\n> - all controls marked as delayed in the sensor database\n> - controls initialized as delayed by the pipeline handler (which won't\n>   be sensor-agnostic anymore)\n> \n> The current implementation aims to be a drop-in replacement for\n> StaggeredCtrl so I'm more than fine with the proposed enanchement\n> being performed on top. At the same time as you know this specific\n> bits will break ov5647 capture on RPi, and I don't think that's right.\n> \n> As a possible plan I would propose:\n> - replace staggered ctrls with this series and allow pipeline handler\n>   to pass in delays to maintain 1-to-1 compatibility with the existing\n> - grow the sensor database and the CameraSensor interface to remove\n>   direct pipeline->DelayedControl interactions on top\n> \n> Am I missing a valid use case for pipeline handler having to deal with\n> DelayedControls directly ?\n\nI'm also wondering about the same. The main use case is indeed the\nCameraSensor class. I wouldn't rule out other potential users, which\nwould be nicely served by the fact that the DelayedControls class can be\ninstantiated separately from CameraSensor, but I'm not sure what those\nwould be.\n\nIn order to make the pipeline handler's life as easy as possible, I'd\nlike to completely hide the DelayedControl instance in the CameraSensor\nclass. This assumes that there would be no reason for a pipeline handler\nto deal with internals of delayed controls, ever, which may not always\nbe true (maybe we'll need hacks in some cases, or maybe there are some\nclever use cases I can't think about now, but I can't see how those\nwould be pipeline handler-specific, so implementing them in CameraSensor\nwould make most sense to me at this point).\n\nHow about leaving this patch out for now, and moving delayed controls\nhandling in CameraSensor in a second step ? Nobody uses this function in\nthis series :-)\n\n> > +\n> > +\t\tdelayedControls_ =\n> > +\t\t\tstd::make_unique<DelayedControls>(subdev_.get(), delays);\n> > +\t}\n> > +\n> > +\treturn delayedControls_.get();\n> > +}\n> > +\n> >  /**\n> >   * \\brief Assemble and return the camera sensor info\n> >   * \\param[out] info The camera sensor info","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1B5E8BE177\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  5 Dec 2020 01:39:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A70DB635EF;\n\tSat,  5 Dec 2020 02:39:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3F3960325\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  5 Dec 2020 02:39:14 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 643552A4;\n\tSat,  5 Dec 2020 02:39:14 +0100 (CET)"],"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=\"GGPBa5F9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607132354;\n\tbh=eoO+eq3t6LtqLR1jJSJ7AthA7WXJAH00lhGNvFgxC50=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GGPBa5F9UmDB9BbsR+oinjCO+ETL8FoSEBY9zJ8l4vu6a2IFd+frIcg3m2ufDIjyf\n\tX1SL07a9RsDyVAYUWVV6j0wCiyKoullg9EUf+COJDQPmcg/9VdK4vBT3ZWMSHcl1gP\n\tje4DQ6R92NMBXUz4REToSmpd5oZ32STpuEAC4ApA=","Date":"Sat, 5 Dec 2020 03:39:12 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20201205013912.GQ4109@pendragon.ideasonboard.com>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-6-niklas.soderlund@ragnatech.se>\n\t<20201127095608.dyjccfp26gmmzecn@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127095608.dyjccfp26gmmzecn@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14070,"web_url":"https://patchwork.libcamera.org/comment/14070/","msgid":"<X8rozwAu2gXZFhGF@pendragon.ideasonboard.com>","date":"2020-12-05T01:56:31","subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sat, Dec 05, 2020 at 03:39:12AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo and Niklas,\n> \n> On Fri, Nov 27, 2020 at 10:56:08AM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 23, 2020 at 11:12:31PM +0100, Niklas Söderlund wrote:\n> > > Expose the optional DelayedControls interface to pipeline handlers. If\n> > > used by a pipeline generic delays are used. In the future the delay\n> > > values should be fetched to match the specific sensor module, either\n> > > from a new kernel interface or worst case a sensors database.\n> > >\n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  include/libcamera/internal/camera_sensor.h |  5 ++++\n> > >  src/libcamera/camera_sensor.cpp            | 31 ++++++++++++++++++++++\n> > >  2 files changed, 36 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index b9eba89f00fba882..6be1cfd49134c534 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -14,6 +14,7 @@\n> > >  #include <libcamera/controls.h>\n> > >  #include <libcamera/geometry.h>\n> > >\n> > > +#include \"libcamera/internal/delayed_controls.h\"\n> > >  #include \"libcamera/internal/formats.h\"\n> > >  #include \"libcamera/internal/log.h\"\n> > >  #include \"libcamera/internal/v4l2_subdevice.h\"\n> > > @@ -61,6 +62,8 @@ public:\n> > >  \tControlList getControls(const std::vector<uint32_t> &ids);\n> > >  \tint setControls(ControlList *ctrls);\n> > >\n> > > +\tDelayedControls *delayedContols();\n> \n> s/delayedContols/delayedControls/\n> \n> > > +\n> > >  \tconst ControlList &properties() const { return properties_; }\n> > >  \tint sensorInfo(CameraSensorInfo *info) const;\n> > >\n> > > @@ -83,6 +86,8 @@ private:\n> > >  \tstd::vector<Size> sizes_;\n> > >\n> > >  \tControlList properties_;\n> > > +\n> > > +\tstd::unique_ptr<DelayedControls> delayedControls_;\n> > >  };\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 935de528c4963453..6c92c97f4cc2547a 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > >  \treturn subdev_->setControls(ctrls);\n> > >  }\n> > >\n> > > +/**\n> > > + * \\brief Get the sensors delayed controls interface\n> > > + *\n> > > + * Access the sensors delayed controls interface. This interface aids pipelines\n> > > + * keeping tack of controls that needs time to take effect. The interface is\n> > > + * optional to use and does not interact with setControls() and getControls()\n> > > + * that operates directly on the sensor device.\n> > > + *\n> > > + * \\sa DelayedControls\n> > > + * \\return The delayed controls interface\n> > > + */\n> > > +DelayedControls *CameraSensor::delayedContols()\n> > > +{\n> > > +\tif (!delayedControls_) {\n> > > +\t\t/*\n> > > +\t\t * \\todo Read dealy values from the sensor itself or from a\n> > > +\t\t * a sensor database. For now use generic values taken from\n> > > +\t\t * the Raspberry Pi and listed as generic values.\n> > > +\t\t */\n> > > +\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n> > > +\t\t\t{ V4L2_CID_ANALOGUE_GAIN, 1 },\n> > > +\t\t\t{ V4L2_CID_EXPOSURE, 2 },\n> > > +\t\t};\n> > \n> > I'm debated.\n> > \n> > As I see this, the current interface will have to evolve, on top of\n> > this series. I think pipeline handler will not have to get a\n> > 'DelayedControls' object and interface with it, but rather the\n> > CameraSensor will use it internally for either:\n> > - all controls marked as delayed in the sensor database\n> > - controls initialized as delayed by the pipeline handler (which won't\n> >   be sensor-agnostic anymore)\n> > \n> > The current implementation aims to be a drop-in replacement for\n> > StaggeredCtrl so I'm more than fine with the proposed enanchement\n> > being performed on top. At the same time as you know this specific\n> > bits will break ov5647 capture on RPi, and I don't think that's right.\n> > \n> > As a possible plan I would propose:\n> > - replace staggered ctrls with this series and allow pipeline handler\n> >   to pass in delays to maintain 1-to-1 compatibility with the existing\n> > - grow the sensor database and the CameraSensor interface to remove\n> >   direct pipeline->DelayedControl interactions on top\n> > \n> > Am I missing a valid use case for pipeline handler having to deal with\n> > DelayedControls directly ?\n> \n> I'm also wondering about the same. The main use case is indeed the\n> CameraSensor class. I wouldn't rule out other potential users, which\n> would be nicely served by the fact that the DelayedControls class can be\n> instantiated separately from CameraSensor, but I'm not sure what those\n> would be.\n> \n> In order to make the pipeline handler's life as easy as possible, I'd\n> like to completely hide the DelayedControl instance in the CameraSensor\n> class. This assumes that there would be no reason for a pipeline handler\n> to deal with internals of delayed controls, ever, which may not always\n> be true (maybe we'll need hacks in some cases, or maybe there are some\n> clever use cases I can't think about now, but I can't see how those\n> would be pipeline handler-specific, so implementing them in CameraSensor\n> would make most sense to me at this point).\n> \n> How about leaving this patch out for now, and moving delayed controls\n> handling in CameraSensor in a second step ? Nobody uses this function in\n> this series :-)\n\nI was wrong about that, it's used in the rkisp1 pipeline handler (and I\nassume by the IPU3 pipeline handler, on top of this series). Still, I\nthink we could duplicate this code in those two pipeline handlers, and\nthen move it to the CameraSensor class.\n\n> > > +\n> > > +\t\tdelayedControls_ =\n> > > +\t\t\tstd::make_unique<DelayedControls>(subdev_.get(), delays);\n> > > +\t}\n> > > +\n> > > +\treturn delayedControls_.get();\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\brief Assemble and return the camera sensor info\n> > >   * \\param[out] info The camera sensor info","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5BA0EBE176\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  5 Dec 2020 01:56:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0D87635DE;\n\tSat,  5 Dec 2020 02:56:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AB1A634C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  5 Dec 2020 02:56:33 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CDA72A4;\n\tSat,  5 Dec 2020 02:56:32 +0100 (CET)"],"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=\"EsPi5Uiq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607133392;\n\tbh=VhPFmGbqZNzf6cIXqchUn0y0zsEYc5Wa4h6StGQ7S9M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EsPi5UiqArZzw81dGcJlY5p90MFBVw9Q1CoCp015dnlMri0ntYVBdX7UhMpegL92Q\n\tics6HPRdvclA5jvOoASTY+FdpTZldtYiog0f04mN0Kst1fmPHj+iZrYnv+wiDJbHY3\n\t/aOqHgiDDZ1Gf7BJjD8u7m05PJ3haWUiM7H2Jjb4=","Date":"Sat, 5 Dec 2020 03:56:31 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<X8rozwAu2gXZFhGF@pendragon.ideasonboard.com>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-6-niklas.soderlund@ragnatech.se>\n\t<20201127095608.dyjccfp26gmmzecn@uno.localdomain>\n\t<20201205013912.GQ4109@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201205013912.GQ4109@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor:\n\tExpose a DelayedControls interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]