[libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls
Naushir Patuck
naush at raspberrypi.com
Fri Jul 9 16:23:13 CEST 2021
On Fri, 9 Jul 2021 at 15:16, Kieran Bingham <kieran.bingham at ideasonboard.com>
wrote:
> Hi Naush,
>
> On 09/07/2021 15:08, Naushir Patuck wrote:
> > Hi Kieran,
> >
> > On Fri, 9 Jul 2021 at 14:42, Kieran Bingham
> > <kieran.bingham at ideasonboard.com
> > <mailto:kieran.bingham at ideasonboard.com>> wrote:
> >
> > Hi Naush,
> >
> > On 09/07/2021 10:56, 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 at raspberrypi.com
> > <mailto:naush at raspberrypi.com>>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com
> > <mailto:david.plowman at raspberrypi.com>>
> > > ---
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 37
> > ++++++++++++++-----
> > > 1 file changed, 27 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 082eb1ee1c23..53a30cff1864 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());
> >
> > Hrm ... ctrls is initialised from the unicam dev, but I can't see
> where
> > controls gets initialised...
> >
> > Is that an issue?
> >
> > Hrm - it looks like it might be constructed with entityControls,
> which
> > is both unicam and ISP controls, so I guess it gets there from those.
> >
> >
> >
> > > - 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,24 @@ 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.
> > > + */
> > > + 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);
> >
> > Does controls need to have V4L2_CID_VBLANK removed from the control
> list
> > to stop it being set twice? (once above, and once below?
> >
> >
> > This is a bit of a tricky one. Ideally, I want to remove
> > V4L2_CID_VBLANK, but we don't have
> > a ControlList::Erase() type method. I could create a whole new
> > ControlList by iterating over
> > controls and adding all but V4L2_CID_VBLANK. This seems a bit
> inefficient.
> >
> > Instead, I am relying on the v4l2 framework to not pass on the control
> > to the driver if the value
> > has not changed from the last set value, which is what happens here. Do
> > you think that's a bit
> > ugly, and perhaps I should not rely on that?
>
> Oh I see ...
>
> Indeed, I don't think we should copy the lists just to erase a value...
>
> So I suspect we either need to implement an erase function on a control
> list control - or ... (perhaps the simpler short term option) put an
> explicit comment here stating that it relies on the behaviour you've
> described so it's not implicit ...
>
I'll add the comment and post an updated series shortly!
>
>
> > Regards,
> > Naush
> >
> >
> >
> > With that done, or if it's not actually needed:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com
> > <mailto:kieran.bingham at ideasonboard.com>>
> >
> >
> > > + }
> > > +
> > > + unicam_[Unicam::Image].dev()->setControls(&controls);
> > > +}
> > > +
> > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > > {
> > > RPi::Stream *stream = nullptr;
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210709/70497fc4/attachment-0001.htm>
More information about the libcamera-devel
mailing list