[{"id":17945,"web_url":"https://patchwork.libcamera.org/comment/17945/","msgid":"","date":"2021-07-02T00:03:54","subject":"Re: [libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Thu, Jul 01, 2021 at 12:34:40PM +0100, Naushir Patuck wrote:\n> When directly writing controls to the sensor device, ensure that VBLANK is\n> written ahead of and before the EXPOSURE control. This is the same priority\n> write mechanism used in DelayedControls.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 37 ++++++++++++++-----\n> 1 file changed, 27 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 082eb1ee1c23..53a30cff1864 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -154,6 +154,7 @@ public:\n> \tvoid embeddedComplete(uint32_t bufferId);\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls);\n> +\tvoid setSensorControls(ControlList &controls);\n> \n> \t/* bufferComplete signal handlers. */\n> \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \n> \t/* Apply any gain/exposure settings that the IPA may have passed back. */\n> \tif (!startConfig.controls.empty())\n> -\t\tdata->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> +\t\tdata->setSensorControls(startConfig.controls);\n> \n> \t/* Configure the number of dropped frames required on startup. */\n> \tdata->dropFrameCount_ = startConfig.dropFrameCount;\n> @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\treturn -EPIPE;\n> \t}\n> \n> -\tif (!controls.empty())\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&controls);\n> -\n> \t/*\n> \t * Configure the H/V flip controls based on the combination of\n> \t * the sensor and user transform.\n> \t */\n> \tif (supportsFlips_) {\n> -\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> -\t\tctrls.set(V4L2_CID_HFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> -\t\tctrls.set(V4L2_CID_VFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\t\tcontrols.set(V4L2_CID_HFLIP,\n> +\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> +\t\tcontrols.set(V4L2_CID_VFLIP,\n> +\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> \t}\n\nStrictly speaking, this could have stayed the same, but it doesn't hurt.\n\n> \n> +\tif (!controls.empty())\n> +\t\tsetSensorControls(controls);\n> +\n> \treturn 0;\n> }\n> \n> @@ -1379,6 +1378,24 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)\n> \thandleState();\n> }\n> \n> +void RPiCameraData::setSensorControls(ControlList &controls)\n> +{\n> +\t/*\n> +\t * We need to ensure that if both VBLANK and EXPOSURE are present, the\n> +\t * former must be written ahead of, and separately from EXPOSURE to avoid\n> +\t * V4L2 rejecting the latter. This is identical to what DelayedControls\n> +\t * does with the priority write flag.\n> +\t */\n> +\tif (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) {\n> +\t\tControlList vblank_ctrl;\n> +\n> +\t\tvblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> +\t\tunicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> +\t}\n> +\n> +\tunicam_[Unicam::Image].dev()->setControls(&controls);\n\nVBLANK will be set twice, could it be an issue ? The value will be the\nsame each time, but that can generate additional I2C writes, which isn't\ngreat.\n\n> +}\n> +\n> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> {\n> \tRPi::Stream *stream = nullptr;","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 24A57C3220\n\tfor ;\n\tFri, 2 Jul 2021 00:04:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BC7C684E3;\n\tFri, 2 Jul 2021 02:04:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EB9960288\n\tfor ;\n\tFri, 2 Jul 2021 02:04:33 +0200 (CEST)","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 EF3F64AB;\n\tFri, 2 Jul 2021 02:04:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SHTpOHnY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625184273;\n\tbh=FmYwDFt2DTQ/KMSfdOCUByOPByAHF4aCFkXzBaNvGjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SHTpOHnYku1/nH5p7QIoP/MKONibKt+Pzf1V1+rulQG7dKxDdhaUZUpvgsQRro6zH\n\te8J2DbtcJ2K8zH9v/yHkO0HSGN2BwiPNk8C8cwE7+m/dU3senBI/hng2hLeoYIdykV\n\tQ9YPqCbVpGbmxb8izk9xvQl1VF1n1pTwN/5oiaec=","Date":"Fri, 2 Jul 2021 03:03:54 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210701113442.111718-1-naush@raspberrypi.com>\n\t<20210701113442.111718-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210701113442.111718-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17947,"web_url":"https://patchwork.libcamera.org/comment/17947/","msgid":"","date":"2021-07-02T07:13:59","subject":"Re: [libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your review feedback.\n\nOn Fri, 2 Jul 2021 at 01:04, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Jul 01, 2021 at 12:34:40PM +0100, Naushir Patuck wrote:\n> > When directly writing controls to the sensor device, ensure that VBLANK\n> is\n> > written ahead of and before the EXPOSURE control. This is the same\n> priority\n> > write mechanism used in DelayedControls.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 37 ++++++++++++++-----\n> > 1 file changed, 27 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 082eb1ee1c23..53a30cff1864 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -154,6 +154,7 @@ public:\n> > void embeddedComplete(uint32_t bufferId);\n> > void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls);\n> > + void setSensorControls(ControlList &controls);\n> >\n> > /* bufferComplete signal handlers. */\n> > void unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const\n> ControlList *controls)\n> >\n> > /* Apply any gain/exposure settings that the IPA may have passed\n> back. */\n> > if (!startConfig.controls.empty())\n> > -\n> data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> > + data->setSensorControls(startConfig.controls);\n> >\n> > /* Configure the number of dropped frames required on startup. */\n> > data->dropFrameCount_ = startConfig.dropFrameCount;\n> > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > return -EPIPE;\n> > }\n> >\n> > - if (!controls.empty())\n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > -\n> > /*\n> > * Configure the H/V flip controls based on the combination of\n> > * the sensor and user transform.\n> > */\n> > if (supportsFlips_) {\n> > - ControlList\n> ctrls(unicam_[Unicam::Image].dev()->controls());\n> > - ctrls.set(V4L2_CID_HFLIP,\n> > -\n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::HFlip)));\n> > - ctrls.set(V4L2_CID_VFLIP,\n> > -\n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::VFlip)));\n> > - unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > + controls.set(V4L2_CID_HFLIP,\n> > +\n> static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> > + controls.set(V4L2_CID_VFLIP,\n> > +\n> static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> > }\n>\n> Strictly speaking, this could have stayed the same, but it doesn't hurt.\n>\n\nI thought it might be better to have a single function doing all\nsetControls calls to the device for\nmaintainability.\n\n\n>\n> >\n> > + if (!controls.empty())\n> > + setSensorControls(controls);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1379,6 +1378,24 @@ void RPiCameraData::setDelayedControls(const\n> ControlList &controls)\n> > handleState();\n> > }\n> >\n> > +void RPiCameraData::setSensorControls(ControlList &controls)\n> > +{\n> > + /*\n> > + * We need to ensure that if both VBLANK and EXPOSURE are present,\n> the\n> > + * former must be written ahead of, and separately from EXPOSURE\n> to avoid\n> > + * V4L2 rejecting the latter. This is identical to what\n> DelayedControls\n> > + * does with the priority write flag.\n> > + */\n> > + if (controls.contains(V4L2_CID_EXPOSURE) &&\n> controls.contains(V4L2_CID_VBLANK)) {\n> > + ControlList vblank_ctrl;\n> > +\n> > + vblank_ctrl.set(V4L2_CID_VBLANK,\n> controls.get(V4L2_CID_VBLANK));\n> > + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> > + }\n> > +\n> Thank you for your\n> > + unicam_[Unicam::Image].dev()->setControls(&controls);\n>\n> VBLANK will be set twice, could it be an issue ? The value will be the\n> same each time, but that can generate additional I2C writes, which isn't\n> great.\n>\n\nYes, I did have to stop and think about what to do here. Ideally, I would\nwant to\nremove VBLANK from controls, and call setControls(&controls) at the end.\nHowever, no mechanism exists to remove elements from the ControlList that\nI know, is that correct?\n\nThe alternative would be to construct a new ControlList that has all the\nelements\nfrom controls, except VBLANK and use that in the last call to setControls.\nThat\nseems quite inefficient when run per frame.\n\nThe proposed way above does set VBLANK twice, but I was relying on the fact\nthat the V4L2 framework would realise that the control value is same as the\ncurrent value, and would not pass the control down to the driver, and avoid\nthe\nsubsequent second I2C write. At least, that is what I thought would happen,\nbut admittedly I have not actually checked to confirm.\n\nRegards,\nNaush\n\n\n>\n> > +}\n> > +\n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > {\n> > RPi::Stream *stream = nullptr;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 72E7CC3220\n\tfor ;\n\tFri, 2 Jul 2021 07:14:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B58E3684F4;\n\tFri, 2 Jul 2021 09:14:17 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C4CC6050A\n\tfor ;\n\tFri, 2 Jul 2021 09:14:16 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id k21so12032881ljh.2\n\tfor ;\n\tFri, 02 Jul 2021 00:14:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"eZqcDim/\"; 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=+WQAm3M5vqeYibD1tvHz+RbNaWUwQOCjSk72n37bPOA=;\n\tb=eZqcDim/uKcgb8T4yj9uPkCa8JnMvsf+a6lTDrdoQvmeu8Dr+YrkRhU3MzBhnfyw9g\n\tRJNhrzqxS1p/Lu8yRLJRtolboK9Epmw3+ca8sg0yq43KXv/Dr2ambF5TatKmF9E6b5X8\n\t5Mn+mL+H18C5bOqgsq8IwT+Xk1y6LoLtgQyjXQ/5kBbOljMolmTybXDIIldzO5H9LznS\n\t0s/P+sLuOPb/0SjHdsyPmYyF44BzTjeHuV0G+RsKFBIqRCkeR3r7AvFWQ5g/9YwU+4QM\n\tKlvQwIH6y/KeyPANOSgwJ7tjK2zPdJvHGWlKVsLdNbuFe7OA2/GX2YB0l25rd83FSopF\n\t/JpQ==","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=+WQAm3M5vqeYibD1tvHz+RbNaWUwQOCjSk72n37bPOA=;\n\tb=K/QfJVsXy4OtamwvQGY2CycUeOU3t/f2PApjPme4z6XAKAoZhmbsyaUTV/06XTFiP8\n\tIPHRMtzDYdpxllQFKWaduzKMpJ7pPp8Dl4QckIwEpC7oxvI7KfoLRhUlB7VzEq12gGm+\n\tjaXGeOGA6fPkmVOFmAqnuRCtREQN0hIDQNmTZpveFqzGPjpKgB3Z2ii98fXtazhisItt\n\tkbNRQIRQ5DRpFK7A1maHUxmCGWdBQKHEDv9zZ7tqqtwsIG0jDi33/lwEmwMkHG4Y+Jp4\n\tpGzvVP0/6cdE8moRHLAvoaIxStBiHa/5ayNRz64cPkjpm+LgZj9frZ++BLy6ljeTU32S\n\trv/Q==","X-Gm-Message-State":"AOAM530dQv5b/JqVhGgYdKKYHLYWAYj19fEZXqwsNeqazTdWCWwKFjSo\n\tx//+Ohh1RtZKx7ZxPCiCHTtOkr5xaHtnUkX+BJcHZQ==","X-Google-Smtp-Source":"ABdhPJwALfzRLDe8AAkVjt1k5ECH1/uTmGVCLbJZmWFSRhF2413FHf7rw6D2e1UTflvQQh3OFOKdSIZGUy567pDyDCg=","X-Received":"by 2002:a2e:9c02:: with SMTP id s2mr2668953lji.299.1625210055191;\n\tFri, 02 Jul 2021 00:14:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210701113442.111718-1-naush@raspberrypi.com>\n\t<20210701113442.111718-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 2 Jul 2021 08:13:59 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000000680a205c61eb459\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]