[{"id":17569,"web_url":"https://patchwork.libcamera.org/comment/17569/","msgid":"","date":"2021-06-15T13:32:53","subject":"Re: [libcamera-devel] [PATCH 2/4] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch.\n\nCoincidentally, one for my forthcoming patches (related to\nV4L2_CID_NOTIFY_GAIN_RED/BLUE) added a setSensorControls method too,\nso this will in fact be helpful.\n\nOn Mon, 14 Jun 2021 at 11:00, Naushir Patuck wrote:\n>\n> When directly writing controls to the sensor device, ensure that VBLANK is\n> written ahead of and before the EXPSOURE control. This is the same priority\n\ns/EXPSOURE/EXPOSURE/\n\n> write mechanism used in DelayedControls.\n>\n> Signed-off-by: Naushir Patuck \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 887f8d0f7404..d180fc059613 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -153,6 +153,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> @@ -827,7 +828,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>\n> /* Apply any gain/exposure settings that the IPA may have passed back. */\n> if (!startConfig.controls.empty())\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> @@ -1293,22 +1294,20 @@ int RPiCameraData::configureIPA(const 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 ctrls(unicam_[Unicam::Image].dev()->controls());\n> - ctrls.set(V4L2_CID_HFLIP,\n> - static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> - ctrls.set(V4L2_CID_VFLIP,\n> - static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> - unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> + controls.set(V4L2_CID_HFLIP,\n> + static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> + controls.set(V4L2_CID_VFLIP,\n> + static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> }\n>\n> + if (!controls.empty())\n> + setSensorControls(controls);\n> +\n> return 0;\n> }\n>\n> @@ -1378,6 +1377,24 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)\n> handleState();\n> }\n>\n> +void RPiCameraData::setSensorControls(ControlList &controls)\n> +{\n> + /*\n> + * We need to ensure that if both VBLANK and EXPSURE are present, the\n\ns/EXPSURE/EXPOSURE/\n\n> + * former must be written ahead of, and separately from EXPOSURE to avoid\n> + * V4L2 rejecting the latter. This is identical to what DelayedControls\n> + * does with the priority write flag.\n> + */\n> + if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) {\n> + ControlList vblank_ctrl;\n> +\n> + vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> + }\n> +\n> + unicam_[Unicam::Image].dev()->setControls(&controls);\n\nThis might be setting the VBLANK for a second time, but presumably\nit's easier to let that happen than to do anything about it?\n\nWith the minor typos fixed:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> +}\n> +\n> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> {\n> RPi::Stream *stream = nullptr;\n> --\n> 2.25.1\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 17F80BD78E\n\tfor ;\n\tTue, 15 Jun 2021 13:33:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D31B6029D;\n\tTue, 15 Jun 2021 15:33:07 +0200 (CEST)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71C2F6029A\n\tfor ;\n\tTue, 15 Jun 2021 15:33:05 +0200 (CEST)","by mail-wr1-x42f.google.com with SMTP id n7so12191626wri.3\n\tfor ;\n\tTue, 15 Jun 2021 06:33:05 -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=\"j+19nNJi\"; 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=ecvzJONYXK26OqR/NeNew7SxTiK05A7itLsMHwEpqtk=;\n\tb=j+19nNJiGkgRCNLjBNztyw88wMslD7U9aSqlOL++ZZ4asyz6D+i2F06mAOL66QAa00\n\tl2FEB7dmb/IdMfIAunH3RhompZO2CvK49dW0xdicsG2jR3PZWKXPd1k3Ut+8dXSWyXF7\n\tSbB6XPSrxpKQiUmtrYwoVlvQomE1cyTfIQ9e96Tvn3O2D3qnw+S+7weX1AOi+GA/8QZO\n\tTtJlszCUF7m8AK6y0lmMXXNMuoCjph/WQI7NsSw4X2c1ND1QHu2lyvuoqJsL+2+hQixe\n\tvY+P3o6whRU91Xofircf//UKA9HZnG+F9doBV2cMl5+xnXZDWLFvAfO3yd1zEHigMeRT\n\tal3A==","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=ecvzJONYXK26OqR/NeNew7SxTiK05A7itLsMHwEpqtk=;\n\tb=ZZmcUpvmE2MBt2Cv59sgU076wyv1HaqWN2jNE9qexIi9ijUPCR/MFFI5IFCw7Jum/3\n\t+zrDEGLSMF5iinfZC4XTX5GrVLRl46AUpqZRaNQNYMbtWO2edkg6OPzXARpKnjrzI4Tf\n\tPi8sfs38/9X8izDBzldP/bfjONHLKFQCMNeVFnjdEEDVCJYPImRTFCRYUrqbXN6/vAqh\n\tU4J/JgmVZkIWX6m6MoeqT4pIWYhNO9otX7IZRRBt7BJv1e8l459Efu3ZcQtyzzHwD/rh\n\tXLtKHBsIagTEnGPT4LSbLRFJb8ZWJOhHJa1wISJXXasKUFY+bojhCdMa0MiUtwtGM8pw\n\tvt9A==","X-Gm-Message-State":"AOAM531zrVCTcsC8H6XKmjbPL3IyR+nfQrhWg2QFWdFP/8rHSQRg5wP0\n\te5X2mDvEBxMRhQuX4dFX40+K7Pjm5fWJyJEn5eUQtA==","X-Google-Smtp-Source":"ABdhPJygCHhpUFnGnUwIBf+vbP4vxub1t4Y3ms6fLdpsTGjG0IbPZgetjHWAEo4PJF4WsBojYG9cIIhiUSDECH30DgM=","X-Received":"by 2002:adf:f28b:: with SMTP id\n\tk11mr24391733wro.89.1623763985110; \n\tTue, 15 Jun 2021 06:33:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-3-naush@raspberrypi.com>","In-Reply-To":"<20210614100040.3054433-3-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 15 Jun 2021 14:32:53 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":17574,"web_url":"https://patchwork.libcamera.org/comment/17574/","msgid":"","date":"2021-06-15T13:41:21","subject":"Re: [libcamera-devel] [PATCH 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 David,\n\nThank you for your feedback.\n\nOn Tue, 15 Jun 2021 at 14:33, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch.\n>\n> Coincidentally, one for my forthcoming patches (related to\n> V4L2_CID_NOTIFY_GAIN_RED/BLUE) added a setSensorControls method too,\n> so this will in fact be helpful.\n>\n> On Mon, 14 Jun 2021 at 11:00, Naushir Patuck \n> wrote:\n> >\n> > When directly writing controls to the sensor device, ensure that VBLANK\n> is\n> > written ahead of and before the EXPSOURE control. This is the same\n> priority\n>\n> s/EXPSOURE/EXPOSURE/\n>\n> > write mechanism used in DelayedControls.\n> >\n> > Signed-off-by: Naushir Patuck \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 887f8d0f7404..d180fc059613 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -153,6 +153,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> > @@ -827,7 +828,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> > @@ -1293,22 +1294,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> > + if (!controls.empty())\n> > + setSensorControls(controls);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1378,6 +1377,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 EXPSURE are\n> present, the\n>\n> s/EXPSURE/EXPOSURE/\n>\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> > + unicam_[Unicam::Image].dev()->setControls(&controls);\n>\n> This might be setting the VBLANK for a second time, but presumably\n> it's easier to let that happen than to do anything about it?\n>\n\nYes, it will do the set twice. However, v4l2 will stop it from getting to\nthe sensor\nas the value will be identical. I though it easier to do that than to\nconstruct another\nControlList and not include VBLANK from that new list. Perhaps we could\nconsider\na ControlList::remove() method for cases like this...?\n\nRegards,\nNaush\n\n\n\n>\n> With the minor typos fixed:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\n> > +}\n> > +\n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > {\n> > RPi::Stream *stream = nullptr;\n> > --\n> > 2.25.1\n> >\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 584B8BD78E\n\tfor ;\n\tTue, 15 Jun 2021 13:41:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9169A68930;\n\tTue, 15 Jun 2021 15:41:39 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A77E6029A\n\tfor ;\n\tTue, 15 Jun 2021 15:41:38 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id x24so21509281lfr.10\n\tfor ;\n\tTue, 15 Jun 2021 06:41:38 -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=\"owVonFLi\"; 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=bOkf2tvaHWftUQ7KBSVA9/RdFEcsnxwZXzWPiG6lZXk=;\n\tb=owVonFLiiwWk/RM/WAFc22OHKP4+z0W3CfQQy8NBrsb6RTpV4qTl8GkcETu3/AVXBd\n\tkuNWgT756dRM7a1PxaXQdeDGXAcrCcUbAC1Tjh7vaq6B4SGIy3JzZL1aidgQSDqJQ6br\n\tisPXXbinmtl2y8D92xZSdzMySRNE2jWKlIYHDVG2e19d6ZOnrhRzama2NQX6lkANOxGO\n\t0JM/xyFbl9R+E7BwhA972PAlX0f6jE33N/MCDCLzrrEWGNZ4aIsUx33kv94N089xeWwT\n\td7I9TGFNEsC9NJCFQOzx7UnfHZtllR962A3hA8q/3bNDvdZDitmW3lQ2XvrNS/iKALEA\n\thWNQ==","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=bOkf2tvaHWftUQ7KBSVA9/RdFEcsnxwZXzWPiG6lZXk=;\n\tb=EOLQLarUXVw26eqlpK2dRcV+xfcTznBqO20t8WB98P9xL8dyg6EpZCMfsRvtBQyylz\n\tKHceTFKGR88tI+eaf4fpV3G1PMtK9Gd0tdDJm7ZTJ6IiwOkD+JZzLXfaQHryT6/1MhJ7\n\tes5ch1f3hfsOTBz+rqo/lBR/x9IzHg57K8Yq7HDZRf8MREgexHfaAt6m+05lpeXphj3H\n\tvSTXHph6KcclsHQ8MzI46EftMox4qtkpcbBY+9S/f6CI2X7BNUz41rgtoZf4l4Ib0sA2\n\txqu07sJ/XFuKJdFY7vLMYpCLirQU55MpXP0Om2BCMxy4XId4lpoPBA4vgn9GOpCEH0vz\n\twqkg==","X-Gm-Message-State":"AOAM531hSOWgo1Q9TIpF9HLXmo0vLPnhnvYPTgR2EfjJE9R0PEiy0fS4\n\tdqFnSLxWVKv0vO51uO2Lad2i/7JzqpT+jVec8exwADx+8lA=","X-Google-Smtp-Source":"ABdhPJzkQi/fHG+HoCgkb1y44qNl4UPb22gahBdKSwYlNJMN/BpyYTztZZXP6WLicWZu6jryCzQDxUHrgqQpxZu15lo=","X-Received":"by 2002:a05:6512:32a5:: with SMTP id\n\tq5mr15342935lfe.171.1623764497574; \n\tTue, 15 Jun 2021 06:41:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 14:41:21 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"00000000000013c17c05c4ce2238\"","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]