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

Message ID 20210709095638.2801713-6-naush@raspberrypi.com
State Superseded
Headers show
Series
  • [libcamera-devel,v5,1/8] ipa: raspberrypi: Make device_status.h C++ only header, and update comments
Related show

Commit Message

Naushir Patuck July 9, 2021, 9:56 a.m. UTC
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(-)

Comments

Kieran Bingham July 9, 2021, 1:42 p.m. UTC | #1
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@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());

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?

With that done, or if it's not actually needed:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +	}
> +
> +	unicam_[Unicam::Image].dev()->setControls(&controls);
> +}
> +
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
>  	RPi::Stream *stream = nullptr;
>
Naushir Patuck July 9, 2021, 2:08 p.m. UTC | #2
Hi Kieran,

On Fri, 9 Jul 2021 at 14:42, Kieran Bingham <kieran.bingham@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@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());
>
> 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?

Regards,
Naush


>
> With that done, or if it's not actually needed:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> > +     }
> > +
> > +     unicam_[Unicam::Image].dev()->setControls(&controls);
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
> >
>
Kieran Bingham July 9, 2021, 2:16 p.m. UTC | #3
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@ideasonboard.com
> <mailto:kieran.bingham@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@raspberrypi.com
>     <mailto:naush@raspberrypi.com>>
>     > Reviewed-by: David Plowman <david.plowman@raspberrypi.com
>     <mailto: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());
> 
>     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@ideasonboard.com
>     <mailto:kieran.bingham@ideasonboard.com>>
> 
> 
>     > +     }
>     > +
>     > +     unicam_[Unicam::Image].dev()->setControls(&controls);
>     > +}
>     > +
>     >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>     >  {
>     >       RPi::Stream *stream = nullptr;
>     >
>
Naushir Patuck July 9, 2021, 2:23 p.m. UTC | #4
On Fri, 9 Jul 2021 at 15:16, Kieran Bingham <kieran.bingham@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@ideasonboard.com
> > <mailto:kieran.bingham@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@raspberrypi.com
> >     <mailto:naush@raspberrypi.com>>
> >     > Reviewed-by: David Plowman <david.plowman@raspberrypi.com
> >     <mailto: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());
> >
> >     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@ideasonboard.com
> >     <mailto:kieran.bingham@ideasonboard.com>>
> >
> >
> >     > +     }
> >     > +
> >     > +     unicam_[Unicam::Image].dev()->setControls(&controls);
> >     > +}
> >     > +
> >     >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >     >  {
> >     >       RPi::Stream *stream = nullptr;
> >     >
> >
>

Patch
diff mbox series

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)));
 	}
 
+	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);
+}
+
 void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
 {
 	RPi::Stream *stream = nullptr;