| Message ID | 20210709145825.2943443-7-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Fri, Jul 09, 2021 at 03:58:23PM +0100, Naushir Patuck wrote: > When directly writing controls to the sensor device, ensure that VBLANK is > written ahead of and before the EXPOSURE control. This is the same priority > write mechanism used in DelayedControls. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++++++++++----- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 082eb1ee1c23..e09328ffa0bc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -154,6 +154,7 @@ public: > void embeddedComplete(uint32_t bufferId); > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > + void setSensorControls(ControlList &controls); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > /* Apply any gain/exposure settings that the IPA may have passed back. */ > if (!startConfig.controls.empty()) > - data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls); > + data->setSensorControls(startConfig.controls); > > /* Configure the number of dropped frames required on startup. */ > data->dropFrameCount_ = startConfig.dropFrameCount; > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > return -EPIPE; > } > > - if (!controls.empty()) > - unicam_[Unicam::Image].dev()->setControls(&controls); > - > /* > * Configure the H/V flip controls based on the combination of > * the sensor and user transform. > */ > if (supportsFlips_) { > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > - ctrls.set(V4L2_CID_HFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > - ctrls.set(V4L2_CID_VFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > + controls.set(V4L2_CID_HFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > + controls.set(V4L2_CID_VFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > } > > + if (!controls.empty()) > + setSensorControls(controls); > + > return 0; > } > > @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) > handleState(); > } > > +void RPiCameraData::setSensorControls(ControlList &controls) > +{ > + /* > + * We need to ensure that if both VBLANK and EXPOSURE are present, the > + * former must be written ahead of, and separately from EXPOSURE to avoid > + * V4L2 rejecting the latter. This is identical to what DelayedControls > + * does with the priority write flag. > + * > + * As a consequence of the below logic, VBLANK gets set twice, and we > + * rely on the v4l2 framework to not pass the second control set to the > + * driver as the actual control value has not changed. > + */ > + if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) { > + ControlList vblank_ctrl; > + > + vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK)); > + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl); > + } > + > + unicam_[Unicam::Image].dev()->setControls(&controls); > +} > + Uff, v4l2... I see another usage of setControls() at match time, but it should not involve VBLANK, so it could be kept the way it is Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr; > -- > 2.25.1 >
Hi Naush, Thank you for the patch. On Fri, Jul 09, 2021 at 03:58:23PM +0100, Naushir Patuck wrote: > When directly writing controls to the sensor device, ensure that VBLANK is > written ahead of and before the EXPOSURE control. This is the same priority > write mechanism used in DelayedControls. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++++++++++----- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 082eb1ee1c23..e09328ffa0bc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -154,6 +154,7 @@ public: > void embeddedComplete(uint32_t bufferId); > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > + void setSensorControls(ControlList &controls); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > /* Apply any gain/exposure settings that the IPA may have passed back. */ > if (!startConfig.controls.empty()) > - data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls); > + data->setSensorControls(startConfig.controls); > > /* Configure the number of dropped frames required on startup. */ > data->dropFrameCount_ = startConfig.dropFrameCount; > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > return -EPIPE; > } > > - if (!controls.empty()) > - unicam_[Unicam::Image].dev()->setControls(&controls); > - > /* > * Configure the H/V flip controls based on the combination of > * the sensor and user transform. > */ > if (supportsFlips_) { > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > - ctrls.set(V4L2_CID_HFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > - ctrls.set(V4L2_CID_VFLIP, > - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > + controls.set(V4L2_CID_HFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > + controls.set(V4L2_CID_VFLIP, > + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > } > > + if (!controls.empty()) > + setSensorControls(controls); > + > return 0; > } > > @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) > handleState(); > } > > +void RPiCameraData::setSensorControls(ControlList &controls) > +{ > + /* > + * We need to ensure that if both VBLANK and EXPOSURE are present, the > + * former must be written ahead of, and separately from EXPOSURE to avoid > + * V4L2 rejecting the latter. This is identical to what DelayedControls > + * does with the priority write flag. > + * > + * As a consequence of the below logic, VBLANK gets set twice, and we > + * rely on the v4l2 framework to not pass the second control set to the > + * driver as the actual control value has not changed. > + */ > + if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) { > + ControlList vblank_ctrl; > + > + vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK)); > + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl); > + } > + > + unicam_[Unicam::Image].dev()->setControls(&controls); > +} > + > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr;
Hi Jacopo, Thank you for your review feedback for all this series. On Fri, 9 Jul 2021 at 17:23, Jacopo Mondi <jacopo@jmondi.org> wrote: > On Fri, Jul 09, 2021 at 03:58:23PM +0100, Naushir Patuck wrote: > > When directly writing controls to the sensor device, ensure that VBLANK > is > > written ahead of and before the EXPOSURE control. This is the same > priority > > write mechanism used in DelayedControls. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++++++++++----- > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 082eb1ee1c23..e09328ffa0bc 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -154,6 +154,7 @@ public: > > void embeddedComplete(uint32_t bufferId); > > void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > + void setSensorControls(ControlList &controls); > > > > /* bufferComplete signal handlers. */ > > void unicamBufferDequeue(FrameBuffer *buffer); > > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const > ControlList *controls) > > > > /* Apply any gain/exposure settings that the IPA may have passed > back. */ > > if (!startConfig.controls.empty()) > > - > data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls); > > + data->setSensorControls(startConfig.controls); > > > > /* Configure the number of dropped frames required on startup. */ > > data->dropFrameCount_ = startConfig.dropFrameCount; > > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > return -EPIPE; > > } > > > > - if (!controls.empty()) > > - unicam_[Unicam::Image].dev()->setControls(&controls); > > - > > /* > > * Configure the H/V flip controls based on the combination of > > * the sensor and user transform. > > */ > > if (supportsFlips_) { > > - ControlList > ctrls(unicam_[Unicam::Image].dev()->controls()); > > - ctrls.set(V4L2_CID_HFLIP, > > - > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & > Transform::HFlip))); > > - ctrls.set(V4L2_CID_VFLIP, > > - > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & > Transform::VFlip))); > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + controls.set(V4L2_CID_HFLIP, > > + > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); > > + controls.set(V4L2_CID_VFLIP, > > + > static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); > > } > > > > + if (!controls.empty()) > > + setSensorControls(controls); > > + > > return 0; > > } > > > > @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const > ControlList &controls) > > handleState(); > > } > > > > +void RPiCameraData::setSensorControls(ControlList &controls) > > +{ > > + /* > > + * We need to ensure that if both VBLANK and EXPOSURE are present, > the > > + * former must be written ahead of, and separately from EXPOSURE > to avoid > > + * V4L2 rejecting the latter. This is identical to what > DelayedControls > > + * does with the priority write flag. > > + * > > + * As a consequence of the below logic, VBLANK gets set twice, and > we > > + * rely on the v4l2 framework to not pass the second control set > to the > > + * driver as the actual control value has not changed. > > + */ > > + if (controls.contains(V4L2_CID_EXPOSURE) && > controls.contains(V4L2_CID_VBLANK)) { > > + ControlList vblank_ctrl; > > + > > + vblank_ctrl.set(V4L2_CID_VBLANK, > controls.get(V4L2_CID_VBLANK)); > > + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl); > > + } > > + > > + unicam_[Unicam::Image].dev()->setControls(&controls); > > +} > > + > > Uff, v4l2... > > I see another usage of setControls() at match time, but it should not > involve VBLANK, so it could be kept the way it is > Good spot, i'll switch to using RPiCameraData::setSensorControls for that call in the next revision. Regards, Naush > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > { > > RPi::Stream *stream = nullptr; > > -- > > 2.25.1 > > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 082eb1ee1c23..e09328ffa0bc 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -154,6 +154,7 @@ public: void embeddedComplete(uint32_t bufferId); void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); + void setSensorControls(ControlList &controls); /* bufferComplete signal handlers. */ void unicamBufferDequeue(FrameBuffer *buffer); @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) /* Apply any gain/exposure settings that the IPA may have passed back. */ if (!startConfig.controls.empty()) - data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls); + data->setSensorControls(startConfig.controls); /* Configure the number of dropped frames required on startup. */ data->dropFrameCount_ = startConfig.dropFrameCount; @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) return -EPIPE; } - if (!controls.empty()) - unicam_[Unicam::Image].dev()->setControls(&controls); - /* * Configure the H/V flip controls based on the combination of * the sensor and user transform. */ if (supportsFlips_) { - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); - ctrls.set(V4L2_CID_HFLIP, - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); - ctrls.set(V4L2_CID_VFLIP, - static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); - unicam_[Unicam::Image].dev()->setControls(&ctrls); + controls.set(V4L2_CID_HFLIP, + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); + controls.set(V4L2_CID_VFLIP, + static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); } + if (!controls.empty()) + setSensorControls(controls); + return 0; } @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) handleState(); } +void RPiCameraData::setSensorControls(ControlList &controls) +{ + /* + * We need to ensure that if both VBLANK and EXPOSURE are present, the + * former must be written ahead of, and separately from EXPOSURE to avoid + * V4L2 rejecting the latter. This is identical to what DelayedControls + * does with the priority write flag. + * + * As a consequence of the below logic, VBLANK gets set twice, and we + * rely on the v4l2 framework to not pass the second control set to the + * driver as the actual control value has not changed. + */ + if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) { + ControlList vblank_ctrl; + + vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK)); + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl); + } + + unicam_[Unicam::Image].dev()->setControls(&controls); +} + void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) { RPi::Stream *stream = nullptr;