[{"id":13534,"web_url":"https://patchwork.libcamera.org/comment/13534/","msgid":"<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>","date":"2020-10-28T11:51:13","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta DelayedControls interface","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nThank you for the changes.  I will go through all the patches with review\ncomment in due course, but wanted to raise a critical point below:\n\nOn Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\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\n> 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>         ControlList getControls(const std::vector<uint32_t> &ids);\n>         int setControls(ControlList *ctrls);\n>\n> +       DelayedControls *delayedContols();\n> +\n>         const ControlList &properties() const { return properties_; }\n>         int sensorInfo(CameraSensorInfo *info) const;\n>\n> @@ -83,6 +86,8 @@ private:\n>         std::vector<Size> sizes_;\n>\n>         ControlList properties_;\n> +\n> +       std::unique_ptr<DelayedControls> delayedControls_;\n>  };\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_sensor.cpp\n> 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>         return 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\n> pipelines\n> + * keeping tack of controls that needs time to take effect. The interface\n> is\n> + * optional to use and does not interact with setControls() and\n> 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> +       if (!delayedControls_) {\n> +               /*\n> +                * \\todo Read dealy values from the sensor itself or from a\n> +                * a sensor database. For now use generic values taken from\n> +                * the Raspberry Pi and listed as generic values.\n> +                */\n> +               std::unordered_map<uint32_t, unsigned int> delays = {\n> +                       { V4L2_CID_ANALOGUE_GAIN, 1 },\n> +                       { V4L2_CID_EXPOSURE, 2 },\n> +               };\n>\n\nThis will break the OV5647 sensor usage with Raspberry Pi without\nreferring back to a sensor database .  OV5647 has a delay of 2 for gain and\nexposure, so will not work with the above hard-coded settings.\n\nIn a more general sense, I wonder if initialising the DelayedControl here\nis the right thing to do?  I have some changes waiting to be submitted for\nreview that add framerate control to the Raspberry Pi stack.  In this\nchange, I've added a VBLANK control to the staggered writer.  There are\nalso some changes to control very long exposures on the imx477 that go\nthrough the staggered writer.  In these cases, I would have to add values\nto the map above, but other pipeline handlers would never need to use\nthem.  This may be a bit wasteful, I don't know...  Would it make more\nsense for the pipeline handlers to initialise the DelayedControls as they\nneed to?\n\nRegards,\nNaush\n\n\n\n> +\n> +               delayedControls_ =\n> +                       std::make_unique<DelayedControls>(subdev_.get(),\n> delays);\n> +       }\n> +\n> +       return 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.1\n>\n>","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 0E546BDB1E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Oct 2020 11:51:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CD50625AC;\n\tWed, 28 Oct 2020 12:51:31 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B6616034C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Oct 2020 12:51:30 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id k25so1492984lji.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Oct 2020 04:51:29 -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=\"SlXGDf+3\"; 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=5+DKX9CTRR8XjYXLWAKq6GMS6goWYTJwe+Nvs/nePiw=;\n\tb=SlXGDf+3Dp3aPgHhVZjHAomob79nMZTDKQcRyzE21W/zBQYuRrNlA/fYj3oQQ9+iF0\n\tTT33voto/8e3R5mY8ZCPhWJRnpMdjsDzODc3pFgfTnGgDtVPsqeYQGH7hGFbsIW8WAOd\n\toieSwPe6qbvyvDb0jY9dgr7/6oLVi8CwI+VEfUgGmzLzs5OnRXAEJMdbdXymK+cZBiCN\n\tpcRUWeD/2MvhO1VsnCB8wSizicEoSP+4m71/wo3GIXAXFaor+1GbUkrFZOgY27zrvmig\n\t2kW1sC7Q+++0xVbYj9aiYDMckPkVasJo4fEa+GGjyT3OzXMNWIM0GLXOS8JVPVUYomtM\n\tY1ew==","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=5+DKX9CTRR8XjYXLWAKq6GMS6goWYTJwe+Nvs/nePiw=;\n\tb=KgpiHygKnRUMoYr9YTl8HkphhjcWmE44bp//FtuSfwfywFx8cY4tg5vtzns4EiUMQp\n\tL5k3Og/8WlX4K3ZZJjQ5zMuLrzt2yK87d+Rj//5UbY4BZiY8PgLbIjajGDgCBWXMu5ak\n\t3Cf7VrABUhYSqnDnoGUMGCvCAUPGcp9qgdMDVEZfNH8YdL73vQmXAHn/+k5osshIIj0G\n\thT7IKju2skq6bS5ny4iCdOb2L9rxFayZZHuzUudd0UcUAtL8sw4ZEBODSLTO9RtNjBOn\n\tuuwhfc0W7alLabzLd/3GcfqhNjt8MDwD89me1Zjm6lTX5r8e5sBtkGyihA2zs6V9umf2\n\tgvvw==","X-Gm-Message-State":"AOAM533xAC57w3exvNR9Qjz+bRaHH60hdfcy1I8IgPJxAe+rf4M5Cujl\n\t75DWsZFqZ3xQaufBgja75et/rpvfeEGvDkAoBcAYIQ==","X-Google-Smtp-Source":"ABdhPJw+Rtbuunh65sO8tPpHWs1h8sS59dZQESu7o4Wubi68/9nuDjdeusqZqcIP4zLsEt+MU1SfGEhRorGi7U3J0Vs=","X-Received":"by 2002:a2e:b1c5:: with SMTP id e5mr2319771lja.83.1603885889352; \n\tWed, 28 Oct 2020 04:51:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-7-niklas.soderlund@ragnatech.se>","In-Reply-To":"<20201028010051.3830668-7-niklas.soderlund@ragnatech.se>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 28 Oct 2020 11:51:13 +0000","Message-ID":"<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta 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":"multipart/mixed;\n\tboundary=\"===============3954942925945564362==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13536,"web_url":"https://patchwork.libcamera.org/comment/13536/","msgid":"<20201028132627.GB591661@oden.dyn.berto.se>","date":"2020-10-28T13:26:27","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta DelayedControls interface","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your feedback.\n\nOn 2020-10-28 11:51:13 +0000, Naushir Patuck wrote:\n> Hi Niklas,\n> \n> Thank you for the changes.  I will go through all the patches with review\n> comment in due course, but wanted to raise a critical point below:\n> \n> On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n> \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\n> > 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> >         ControlList getControls(const std::vector<uint32_t> &ids);\n> >         int setControls(ControlList *ctrls);\n> >\n> > +       DelayedControls *delayedContols();\n> > +\n> >         const ControlList &properties() const { return properties_; }\n> >         int sensorInfo(CameraSensorInfo *info) const;\n> >\n> > @@ -83,6 +86,8 @@ private:\n> >         std::vector<Size> sizes_;\n> >\n> >         ControlList properties_;\n> > +\n> > +       std::unique_ptr<DelayedControls> delayedControls_;\n> >  };\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_sensor.cpp\n> > 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> >         return 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\n> > pipelines\n> > + * keeping tack of controls that needs time to take effect. The interface\n> > is\n> > + * optional to use and does not interact with setControls() and\n> > 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> > +       if (!delayedControls_) {\n> > +               /*\n> > +                * \\todo Read dealy values from the sensor itself or from a\n> > +                * a sensor database. For now use generic values taken from\n> > +                * the Raspberry Pi and listed as generic values.\n> > +                */\n> > +               std::unordered_map<uint32_t, unsigned int> delays = {\n> > +                       { V4L2_CID_ANALOGUE_GAIN, 1 },\n> > +                       { V4L2_CID_EXPOSURE, 2 },\n> > +               };\n> >\n> \n> This will break the OV5647 sensor usage with Raspberry Pi without\n> referring back to a sensor database .  OV5647 has a delay of 2 for gain and\n> exposure, so will not work with the above hard-coded settings.\n\nI understand your point, and we really should get started with a \nsolution on how to get sensor specific data into CameraSensor database.\n\nBut with this series there is no problem for the Raspberry Pi pipeline \nas it does not deal with controls on the CameraSensor but the V4L2 video \ndevice. Therefor the RPi pipeline creates a private instance of \nDelayedControls witch delays still are initialized by the RPi IPA, just \nas things worked before using StaggerdCtrls.\n\nWhen developing this I ran DelayedControls and StaggerdCtrls in parallel \non the RPi and the output of both matched perfectly frame by frame.\n\n> \n> In a more general sense, I wonder if initialising the DelayedControl here\n> is the right thing to do?  I have some changes waiting to be submitted for\n> review that add framerate control to the Raspberry Pi stack.  In this\n> change, I've added a VBLANK control to the staggered writer.  There are\n> also some changes to control very long exposures on the imx477 that go\n> through the staggered writer.  In these cases, I would have to add values\n> to the map above, but other pipeline handlers would never need to use\n> them.  This may be a bit wasteful, I don't know...  Would it make more\n> sense for the pipeline handlers to initialise the DelayedControls as they\n> need to?\n\nFirst, we are still thinking about how VBLANK and other controls that \ncan modify the frame length are to be modelled in the CameraSensor. If \nyou have any ideas or tips in this area I would greatly appreciate them.\n\nI really think CameraSensor is the location to initialising the delays, \nthis separates the sensor from the pipeline. I think this is important \nas otherwise each pipeline will have have a list of sensors it knows \nabout and will not function popery with different ones. By \"forcing\" \npipelines to learn about the sensor used thru CameraSensor interface we \nensure that we can add more sensors in the future without having to \nupdate all pipelines and/or IPAs. This is even more important if we \nconsider closed-source IPAs as we can't just add a new sensor with \ntimings recompile.\n\nYet again as the Raspberry Pi pipeline is video device centric it can't \nreally use the CameraSensor to deal with controls and must instead \ncreate and configure its local version of DelayedControls. I still think \nin the future such pipelines can benefit from a sensor database in \nCameraSensor as it should be possible to query the datbase and use the \ndelays reported while still creating a private local DelayedControls.\n\nDoes this relive your worries bout this series?\n\n> \n> Regards,\n> Naush\n> \n> \n> \n> > +\n> > +               delayedControls_ =\n> > +                       std::make_unique<DelayedControls>(subdev_.get(),\n> > delays);\n> > +       }\n> > +\n> > +       return 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.1\n> >\n> >","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 E8DDBC3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Oct 2020 13:26:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5525D626B2;\n\tWed, 28 Oct 2020 14:26:32 +0100 (CET)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF1326052D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Oct 2020 14:26:31 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id t13so6148681ljk.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Oct 2020 06:26:31 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp8sm532122lfe.11.2020.10.28.06.26.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Oct 2020 06:26:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"HbbU7dRQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=NJWYAny0mwiArOk+eTm4sZWOV8tdE9SM2r7CYqiPwII=;\n\tb=HbbU7dRQK9TTTuRhLzVLKr2xbqKb8yrNsBa1NObVZSGa+8ycno0uL7SlSTNAMoxCLz\n\tcoCNcldwFE5rwZ9g4JzngE4ZXMmTKZVobsk308eW+phJC8ZLU5pO4UpadJfP+slUWNLt\n\tlHfEw+f4KYCVr1VFDGDgUTG7C2/zxFZ7tvBJvikkm0dfDTsws3LvYByWQSgSEzWAbxkG\n\tOoaernfPykZnjs8om8p7o2kvjCUNzIbkqQTczxt1MXFtANcTjh4SZNzANPz2N/+kBvDS\n\toF9/qvlH7Vf0N48eSK9JII5yqb6qHuvY3b5S2qWcT9oMTywqMBe/p2UqUuo14j6v6F/B\n\t79iA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=NJWYAny0mwiArOk+eTm4sZWOV8tdE9SM2r7CYqiPwII=;\n\tb=hUeEfUeF/I/Tshv6vfsL34w6HaGLmkacGwYO0Le52LK1k8wesrb8J4OzLC31mZ6jr1\n\t+Siuu+BLiEDZlClak0F8Vhj1bnIXdL+wCCl1nVDhdA1jgTHHwBOWXHXEztnvMqY3Iboo\n\tLNqzLjHlG2xTtePYnTC9JzVZWTx73S+t/HgmIsYZ5iqsLIc2kUObBcJ+/tAOebdG1Mbb\n\tAqffu4vtA+qfTE3YUZjD9gfNVtXPSOPJaXOBJFiskDknTvOoj4Hah8IzpwlCwS5ZbVAd\n\t60ChHqMlt7MOAURtPG0FH/wn1tgSV5AzwQ+9gdyeOjaqFn20Ur/cBZDccAgiaon7n6fr\n\t5DfQ==","X-Gm-Message-State":"AOAM530bi3nsY8I7i3lLzO16gmm/Z3KIxiG4cHTLQ95CVltc0t5njf9Z\n\tZHdPvPdXIeHfZo/A2e1JReslVw==","X-Google-Smtp-Source":"ABdhPJwp2OjqEgvW38t4vV7saQsBG+95S0KDX7Ar2QZbnAaaG3DKx9qsvdSjzIyyh6MtE0JYZA3A2g==","X-Received":"by 2002:a2e:80c2:: with SMTP id r2mr3152745ljg.402.1603891589519;\n\tWed, 28 Oct 2020 06:26:29 -0700 (PDT)","Date":"Wed, 28 Oct 2020 14:26:27 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20201028132627.GB591661@oden.dyn.berto.se>","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-7-niklas.soderlund@ragnatech.se>\n\t<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13552,"web_url":"https://patchwork.libcamera.org/comment/13552/","msgid":"<CAEmqJPq2Rh+2csp_K5v18MjxFqnkWoZ9Kav_2oOUstysRXnxsQ@mail.gmail.com>","date":"2020-10-29T10:58:19","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta DelayedControls interface","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nThank you for the clarification.\n\nOn Wed, 28 Oct 2020 at 13:26, Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Naushir,\n>\n> Thanks for your feedback.\n>\n> On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote:\n> > Hi Niklas,\n> >\n> > Thank you for the changes.  I will go through all the patches with review\n> > comment in due course, but wanted to raise a critical point below:\n> >\n> > On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\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\n> > > 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> > >         ControlList getControls(const std::vector<uint32_t> &ids);\n> > >         int setControls(ControlList *ctrls);\n> > >\n> > > +       DelayedControls *delayedContols();\n> > > +\n> > >         const ControlList &properties() const { return properties_; }\n> > >         int sensorInfo(CameraSensorInfo *info) const;\n> > >\n> > > @@ -83,6 +86,8 @@ private:\n> > >         std::vector<Size> sizes_;\n> > >\n> > >         ControlList properties_;\n> > > +\n> > > +       std::unique_ptr<DelayedControls> delayedControls_;\n> > >  };\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/camera_sensor.cpp\n> > > 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> > >         return 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\n> > > pipelines\n> > > + * keeping tack of controls that needs time to take effect. The\n> interface\n> > > is\n> > > + * optional to use and does not interact with setControls() and\n> > > 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> > > +       if (!delayedControls_) {\n> > > +               /*\n> > > +                * \\todo Read dealy values from the sensor itself or\n> from a\n> > > +                * a sensor database. For now use generic values taken\n> from\n> > > +                * the Raspberry Pi and listed as generic values.\n> > > +                */\n> > > +               std::unordered_map<uint32_t, unsigned int> delays = {\n> > > +                       { V4L2_CID_ANALOGUE_GAIN, 1 },\n> > > +                       { V4L2_CID_EXPOSURE, 2 },\n> > > +               };\n> > >\n> >\n> > This will break the OV5647 sensor usage with Raspberry Pi without\n> > referring back to a sensor database .  OV5647 has a delay of 2 for gain\n> and\n> > exposure, so will not work with the above hard-coded settings.\n>\n> I understand your point, and we really should get started with a\n> solution on how to get sensor specific data into CameraSensor database.\n>\n> But with this series there is no problem for the Raspberry Pi pipeline\n> as it does not deal with controls on the CameraSensor but the V4L2 video\n> device. Therefor the RPi pipeline creates a private instance of\n> DelayedControls witch delays still are initialized by the RPi IPA, just\n> as things worked before using StaggerdCtrls.\n>\n> When developing this I ran DelayedControls and StaggerdCtrls in parallel\n> on the RPi and the output of both matched perfectly frame by frame.\n>\n\nYes, I see that now.  So there should be no problems for pipeline handlers\nto allocate a DelayedControl for its own use?  In that case, the above\nshould not be a problem.  But you are correct, this does show the need to\nget a central sensor database available soon.\n\n\n>\n> >\n> > In a more general sense, I wonder if initialising the DelayedControl here\n> > is the right thing to do?  I have some changes waiting to be submitted\n> for\n> > review that add framerate control to the Raspberry Pi stack.  In this\n> > change, I've added a VBLANK control to the staggered writer.  There are\n> > also some changes to control very long exposures on the imx477 that go\n> > through the staggered writer.  In these cases, I would have to add values\n> > to the map above, but other pipeline handlers would never need to use\n> > them.  This may be a bit wasteful, I don't know...  Would it make more\n> > sense for the pipeline handlers to initialise the DelayedControls as they\n> > need to?\n>\n> First, we are still thinking about how VBLANK and other controls that\n> can modify the frame length are to be modelled in the CameraSensor. If\n> you have any ideas or tips in this area I would greatly appreciate them.\n>\n\nI could possibly share with you (privately for now, to avoid too much noise\non this mailing list) my changes to add VBLANK control, to discuss what I\nhave done?\n\n\n>\n> I really think CameraSensor is the location to initialising the delays,\n> this separates the sensor from the pipeline. I think this is important\n> as otherwise each pipeline will have have a list of sensors it knows\n> about and will not function popery with different ones. By \"forcing\"\n> pipelines to learn about the sensor used thru CameraSensor interface we\n> ensure that we can add more sensors in the future without having to\n> update all pipelines and/or IPAs. This is even more important if we\n> consider closed-source IPAs as we can't just add a new sensor with\n> timings recompile.\n>\n> Yet again as the Raspberry Pi pipeline is video device centric it can't\n> really use the CameraSensor to deal with controls and must instead\n> create and configure its local version of DelayedControls. I still think\n> in the future such pipelines can benefit from a sensor database in\n> CameraSensor as it should be possible to query the datbase and use the\n> delays reported while still creating a private local DelayedControls.\n>\n\nYes, I agree with this.  Just want to raise the point that some sensors\nwill have controls not available to others, e.g. imx477 long exposure\nsettings.  So it would be good to have as much flexibility in the sensor\ndatabase to allow for these variations.\n\n\n>\n> Does this relive your worries bout this series?\n>\n\nIndeed it does.  I will get to reviewing the full patchset shortly and send\nover my comments.\n\nBest regards,\nNaush\n\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > > +\n> > > +               delayedControls_ =\n> > > +\n>  std::make_unique<DelayedControls>(subdev_.get(),\n> > > delays);\n> > > +       }\n> > > +\n> > > +       return 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.1\n> > >\n> > >\n>\n> --\n> Regards,\n> Niklas Söderlund\n>","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 5B092BDB9B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 29 Oct 2020 10:58:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAEE5628DF;\n\tThu, 29 Oct 2020 11:58:37 +0100 (CET)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DB2062882\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Oct 2020 11:58:36 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id v6so2643978lfa.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Oct 2020 03:58:36 -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=\"cp/n6l7y\"; 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=kph4j5enkkEipIZ4dD0z3xLWrZuO7yLPOLlz0WCHFHk=;\n\tb=cp/n6l7yLdqcOcBP1c9Jn63Pr7rxAaiOLukd+Pw71IeFBty1SE2oZh1V47wzU2D8WV\n\t2UTvQGXmT6i0o+fKDj+vKQ6aytNaV7rL+Fz0iAcKUKx5/sDpanp4dtWyHCJQL0Jw+aom\n\tQuMG+Io/y/iZGFkTHsmTV7NRC4qZToCb+lqfBvmTR5afl7xhI7w95mKtGAQnhVahZpJ1\n\t+QygMYQpXMLfyRFLFdhpgavO8ZaUX0EEzlvUQ0vdC5pYTC6hGsrsTtkqrNf+0HTw78rE\n\tKe3FLnDN2MbdW6j29+DtVvZTz/OMCzvZ5VT6HPy+XsA4HwExg/ECOd4bjcC6eF6fqrHF\n\tOqYg==","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=kph4j5enkkEipIZ4dD0z3xLWrZuO7yLPOLlz0WCHFHk=;\n\tb=F7UWUNaMOXdG+SwIyF03ocaUe/nyquo3+BkI2h2ho6CidJ/nRHg5Ju8DX8GvIrDN+j\n\tMVSkV9PP+n3jwM7A1ByVBqqzD6IMGwOmw7AQ3QJBmeC8av9UXD7C+MZ+E4bH8scKC9Xi\n\tSCK2b97vod2niR7knOQdpoP9VklhZrprWPTLE+xh3RIIXbjcVU/613GMLTf5W+fga7sC\n\tAfwjX5qFApFXQbQuFSEpCfvtZQlzlcnO6IwKWTRjwUUH57Vzah6arTa2sSe0MI983Km2\n\tpDC8S/j3D2X0an8OqUQmen7+E7ZRYDSIspFXouY+mw2XMtQstxFR9cXUlXrq9Fxb5NpJ\n\tOHMA==","X-Gm-Message-State":"AOAM531PZKvXUbPrAO+XcVGCTt+UR0HTAZL0Z05UIoXOenAKi6x1R8yc\n\twKgEizL9DJFgppF1qRYRWiTzvBl1ErWOkuVkbL1EhQ==","X-Google-Smtp-Source":"ABdhPJyJLAlXsB7Oqtpb8eJeUwDT3NttpHaAlaZ0BXQ5dkyLU+Dk3NSjrF7FQ5MU0rdXErx+1QKM1h9xpOPOa6z17mc=","X-Received":"by 2002:a19:82c9:: with SMTP id\n\te192mr1239248lfd.215.1603969115775; \n\tThu, 29 Oct 2020 03:58:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-7-niklas.soderlund@ragnatech.se>\n\t<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>\n\t<20201028132627.GB591661@oden.dyn.berto.se>","In-Reply-To":"<20201028132627.GB591661@oden.dyn.berto.se>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Thu, 29 Oct 2020 10:58:19 +0000","Message-ID":"<CAEmqJPq2Rh+2csp_K5v18MjxFqnkWoZ9Kav_2oOUstysRXnxsQ@mail.gmail.com>","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta 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":"multipart/mixed;\n\tboundary=\"===============7032379182579989461==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13628,"web_url":"https://patchwork.libcamera.org/comment/13628/","msgid":"<20201106143716.GF3195686@oden.dyn.berto.se>","date":"2020-11-06T14:37:16","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta DelayedControls interface","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nOn 2020-10-29 10:58:19 +0000, Naushir Patuck wrote:\n> Hi Niklas,\n> \n> Thank you for the clarification.\n> \n> On Wed, 28 Oct 2020 at 13:26, Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n> \n> > Hi Naushir,\n> >\n> > Thanks for your feedback.\n> >\n> > On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote:\n> > > Hi Niklas,\n> > >\n> > > Thank you for the changes.  I will go through all the patches with review\n> > > comment in due course, but wanted to raise a critical point below:\n> > >\n> > > On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <\n> > > niklas.soderlund@ragnatech.se> wrote:\n> > >\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\n> > > > 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> > > >         ControlList getControls(const std::vector<uint32_t> &ids);\n> > > >         int setControls(ControlList *ctrls);\n> > > >\n> > > > +       DelayedControls *delayedContols();\n> > > > +\n> > > >         const ControlList &properties() const { return properties_; }\n> > > >         int sensorInfo(CameraSensorInfo *info) const;\n> > > >\n> > > > @@ -83,6 +86,8 @@ private:\n> > > >         std::vector<Size> sizes_;\n> > > >\n> > > >         ControlList properties_;\n> > > > +\n> > > > +       std::unique_ptr<DelayedControls> delayedControls_;\n> > > >  };\n> > > >\n> > > >  } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/camera_sensor.cpp\n> > > > 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> > > >         return 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\n> > > > pipelines\n> > > > + * keeping tack of controls that needs time to take effect. The\n> > interface\n> > > > is\n> > > > + * optional to use and does not interact with setControls() and\n> > > > 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> > > > +       if (!delayedControls_) {\n> > > > +               /*\n> > > > +                * \\todo Read dealy values from the sensor itself or\n> > from a\n> > > > +                * a sensor database. For now use generic values taken\n> > from\n> > > > +                * the Raspberry Pi and listed as generic values.\n> > > > +                */\n> > > > +               std::unordered_map<uint32_t, unsigned int> delays = {\n> > > > +                       { V4L2_CID_ANALOGUE_GAIN, 1 },\n> > > > +                       { V4L2_CID_EXPOSURE, 2 },\n> > > > +               };\n> > > >\n> > >\n> > > This will break the OV5647 sensor usage with Raspberry Pi without\n> > > referring back to a sensor database .  OV5647 has a delay of 2 for gain\n> > and\n> > > exposure, so will not work with the above hard-coded settings.\n> >\n> > I understand your point, and we really should get started with a\n> > solution on how to get sensor specific data into CameraSensor database.\n> >\n> > But with this series there is no problem for the Raspberry Pi pipeline\n> > as it does not deal with controls on the CameraSensor but the V4L2 video\n> > device. Therefor the RPi pipeline creates a private instance of\n> > DelayedControls witch delays still are initialized by the RPi IPA, just\n> > as things worked before using StaggerdCtrls.\n> >\n> > When developing this I ran DelayedControls and StaggerdCtrls in parallel\n> > on the RPi and the output of both matched perfectly frame by frame.\n> >\n> \n> Yes, I see that now.  So there should be no problems for pipeline handlers\n> to allocate a DelayedControl for its own use?  In that case, the above\n> should not be a problem.  But you are correct, this does show the need to\n> get a central sensor database available soon.\n\nI don't think there is going to be a problem for pipelines who are not \nusing CameraSensor to manage their sensor no. I do hope we given time \ncan move the Raspberry Pi pipeline to be media device centric so it \ndon't *have* to instantiate its own DelayedControl, but that is a \nseparate discussion ;-P\n\n> \n> \n> >\n> > >\n> > > In a more general sense, I wonder if initialising the DelayedControl here\n> > > is the right thing to do?  I have some changes waiting to be submitted\n> > for\n> > > review that add framerate control to the Raspberry Pi stack.  In this\n> > > change, I've added a VBLANK control to the staggered writer.  There are\n> > > also some changes to control very long exposures on the imx477 that go\n> > > through the staggered writer.  In these cases, I would have to add values\n> > > to the map above, but other pipeline handlers would never need to use\n> > > them.  This may be a bit wasteful, I don't know...  Would it make more\n> > > sense for the pipeline handlers to initialise the DelayedControls as they\n> > > need to?\n> >\n> > First, we are still thinking about how VBLANK and other controls that\n> > can modify the frame length are to be modelled in the CameraSensor. If\n> > you have any ideas or tips in this area I would greatly appreciate them.\n> >\n> \n> I could possibly share with you (privately for now, to avoid too much noise\n> on this mailing list) my changes to add VBLANK control, to discuss what I\n> have done?\n\nThat would be interesting if you have something you feel is ready, no \nrush. I'm just starting to think about this problem so any input and \nfeedback is good for me.\n\n> \n> \n> >\n> > I really think CameraSensor is the location to initialising the delays,\n> > this separates the sensor from the pipeline. I think this is important\n> > as otherwise each pipeline will have have a list of sensors it knows\n> > about and will not function popery with different ones. By \"forcing\"\n> > pipelines to learn about the sensor used thru CameraSensor interface we\n> > ensure that we can add more sensors in the future without having to\n> > update all pipelines and/or IPAs. This is even more important if we\n> > consider closed-source IPAs as we can't just add a new sensor with\n> > timings recompile.\n> >\n> > Yet again as the Raspberry Pi pipeline is video device centric it can't\n> > really use the CameraSensor to deal with controls and must instead\n> > create and configure its local version of DelayedControls. I still think\n> > in the future such pipelines can benefit from a sensor database in\n> > CameraSensor as it should be possible to query the datbase and use the\n> > delays reported while still creating a private local DelayedControls.\n> >\n> \n> Yes, I agree with this.  Just want to raise the point that some sensors\n> will have controls not available to others, e.g. imx477 long exposure\n> settings.  So it would be good to have as much flexibility in the sensor\n> database to allow for these variations.\n\nAgreed. One goal I hope we can achieve is that all such information can \nbe enumerated on the CameraSensor as controls/values/properties/flags. I \nwould if possible try to avoid pipelines and IPAs making decisions based \non the sensor model name if it can be described as a property of the \nsensor. But maybe that will not be possible in all cases.\n\n> \n> \n> >\n> > Does this relive your worries bout this series?\n> >\n> \n> Indeed it does.  I will get to reviewing the full patchset shortly and send\n> over my comments.\n\nThanks!\n\n> \n> Best regards,\n> Naush\n> \n> \n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > > +\n> > > > +               delayedControls_ =\n> > > > +\n> >  std::make_unique<DelayedControls>(subdev_.get(),\n> > > > delays);\n> > > > +       }\n> > > > +\n> > > > +       return 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.1\n> > > >\n> > > >\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> >","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 F3C30BE082\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Nov 2020 14:37:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BCBD62D2D;\n\tFri,  6 Nov 2020 15:37:20 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 084FA622AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Nov 2020 15:37:19 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id y16so1613628ljk.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Nov 2020 06:37:18 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id v3sm1335lfg.247.2020.11.06.06.37.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Nov 2020 06:37:17 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"WvMIeSjS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TP92ZzdNj7FFAbpXbYCb8n/s2Vx2dCWd0trag+ggJYE=;\n\tb=WvMIeSjSPmW8IW0UPEoJ5aS46jQ8ksz856wv0nKkpx4FdMgBLqxG5jdqGMuamPylCJ\n\t0dnSA5wyxDbIZ4UI9mvqPq/By9v9+ZVb6bT1/87WU+dBsLvDbA+dhL2QalFoDG9xygri\n\tBBrgPMHe6I4yJwQc4wH9gfQl/uzYLQZwWeiXfkGyosSTsqDCEMGW5T/Hexm//m8MTZoi\n\txJ86IFyd/kuE0Lqy0A/cXmLEW2lctIQ0amVoMHH2jBAB0rangrhmEz5wOEUKGNxtPpOw\n\t9AmpIuIK914S2PNMkh8pc7WMn1KKPGxXYE5ZMuPIcNfgUKrXKNfDIyJZaEBydigDc0PO\n\tP0WQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TP92ZzdNj7FFAbpXbYCb8n/s2Vx2dCWd0trag+ggJYE=;\n\tb=gV/VVi8FASBy4eHmEggqlDN+XZpLvJ7DAmoUB9shNUtqlO9W9Ho3HiwLRwrqUT3vGw\n\t389PRfro/rK8F1lb/qmhIngfMETdMPX4sZWUSB6MKSE/JFQDDaTLDzQLqoZTnUeKm63d\n\toCpZpnAtHWmq+pIYsevNIv9AIweqYlNZYX5QptalzOOm4zvUcFlNsD8ZjMNqwIFQirko\n\tlzFAu8YNEHvtb5V7PHSUyi0zsZ89UX1ZTsIr0TyEURZnf7W7DOwxZt8V1Hlg869c5q7H\n\taJIlBwhu5mydoJsGBldfz0B9OJI+sDYFc3jwZljH1mDbC7hd6qKM36piP/4KrMsJ9nbL\n\twJcg==","X-Gm-Message-State":"AOAM532Y3bKfLqi5lbu9cIPn4Up9IY1t18IumenmNylshH6+DRb4tmI0\n\t2b5IWO5iWpFF/MLO0HwOF1xlYA==","X-Google-Smtp-Source":"ABdhPJw5/Kvtg5fvjzsEt/aeVzBfka6r7vOnHJJxKhXJkGx5snBlOKBjQ7OrALNJ/4h9Annx8F9j0g==","X-Received":"by 2002:a2e:89d8:: with SMTP id c24mr894789ljk.435.1604673438247;\n\tFri, 06 Nov 2020 06:37:18 -0800 (PST)","Date":"Fri, 6 Nov 2020 15:37:16 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20201106143716.GF3195686@oden.dyn.berto.se>","References":"<20201028010051.3830668-1-niklas.soderlund@ragnatech.se>\n\t<20201028010051.3830668-7-niklas.soderlund@ragnatech.se>\n\t<CAEmqJPo7iKqV4tVke8g=8+eGy2n11r7ffK9-amzCT2fi7avAEw@mail.gmail.com>\n\t<20201028132627.GB591661@oden.dyn.berto.se>\n\t<CAEmqJPq2Rh+2csp_K5v18MjxFqnkWoZ9Kav_2oOUstysRXnxsQ@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPq2Rh+2csp_K5v18MjxFqnkWoZ9Kav_2oOUstysRXnxsQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose\n\ta 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]