[libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

[Kieran Bingham]

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 ...


> 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;
>     >
>