[libcamera-devel,v2,2/4] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

12757 diff mbox series

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>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 37 ++++++++++++++-----
 1 file changed, 27 insertions(+), 10 deletions(-)

Hi Naush,

Thank you for the patch.

On Thu, Jul 01, 2021 at 12:34:40PM +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>
> ---
>  .../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());
> -		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)));
>  	}

Strictly speaking, this could have stayed the same, but it doesn't hurt.

>  
> +	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);
> +	}
> +
> +	unicam_[Unicam::Image].dev()->setControls(&controls);

VBLANK will be set twice, could it be an issue ? The value will be the
same each time, but that can generate additional I2C writes, which isn't
great.

> +}
> +
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
>  	RPi::Stream *stream = nullptr;

Hi Laurent,

Thank you for your review feedback.

On Fri, 2 Jul 2021 at 01:04, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Jul 01, 2021 at 12:34:40PM +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>
> > ---
> >  .../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());
> > -             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)));
> >       }
>
> Strictly speaking, this could have stayed the same, but it doesn't hurt.
>

I thought it might be better to have a single function doing all
setControls calls to the device for
maintainability.


>
> >
> > +     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);
> > +     }
> > +
> Thank you for your
> > +     unicam_[Unicam::Image].dev()->setControls(&controls);
>
> VBLANK will be set twice, could it be an issue ? The value will be the
> same each time, but that can generate additional I2C writes, which isn't
> great.
>

Yes, I did have to stop and think about what to do here.  Ideally, I would
want to
remove VBLANK from controls, and call setControls(&controls) at the end.
However, no mechanism exists to remove elements from the ControlList that
I know, is that correct?

The alternative would be to construct a new ControlList that has all the
elements
from controls, except VBLANK and use that in the last call to setControls.
That
seems quite inefficient when run per frame.

The proposed way above does set VBLANK twice, but I was relying on the fact
that the V4L2 framework would realise that the control value is same as the
current value, and would not pass the control down to the driver, and avoid
the
subsequent second I2C write.  At least, that is what I thought would happen,
but admittedly I have not actually checked to confirm.

Regards,
Naush


>
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
>
> --
> Regards,
>
> Laurent Pinchart
>