[libcamera-devel,1/1] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()
diff mbox series

Message ID 20210330165743.4924-2-david.plowman@raspberrypi.com
State Superseded
Delegated to: Laurent Pinchart
Headers show
Series
  • Fix CameraSensor::sensorInfo by updating VBLANK ControlInfo
Related show

Commit Message

David Plowman March 30, 2021, 4:57 p.m. UTC
The minimum and maximum vblanking can change when a new format is
applied to the sensor subdevice, so be sure to retrieve up-to-date
values.

The V4L2Device acquires the new refreshControls() method to perform
this function. Note that not all pipeline handlers invoke the
subdevice's setFormat method directly, so the new method must be made
publicly available.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/v4l2_device.h |  2 ++
 src/libcamera/camera_sensor.cpp          |  7 +++++++
 src/libcamera/v4l2_device.cpp            | 26 ++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

Comments

Jacopo Mondi March 31, 2021, 12:12 p.m. UTC | #1
Hi David,

I admit I have not followed closely the recent discussions on this
topic, but this seems an acceptable solution to me, however I wonder
if it would not be worth taking it a little step forward (I know...)

The problem I see here is fundamentally one: we're missing a method
to update a (V4L2)ControlInfo. The method itself could be quite
trivial, a ControlInfo::update(ControlValue min, ...) and one
V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).

The lack of a mechanism to update a control info has this
consequences:
1) V4L2 controls passed by the ph to the IPA could be stale after a
sensor format update
2) the Camera::controls_ might be stale after a Camera::configure()

If you introduce the above suggested update() (or whatever) methods in
(V4L2)ControlInfo, then they could be used as you're doing here below
to handle 1)

In order to handle 2) pipeline handlers that register controls would
have to get a freshened list of V4L2Controls from the CameraSensor (or
use a freshened CameraSensorInfo) to update the controls they register
as Camera::controls_, performing the opportune re-calculations (in
example the IPU3::initControls() function would need to be split in
calculateExposure(), calculateDurations() etc which will then be
called at configure() time to update Camera::controls_.

Do you think your proposed solution could work well with
ControlInfo::update() ?

A few minors on the patch

On Tue, Mar 30, 2021 at 05:57:43PM +0100, David Plowman wrote:
> The minimum and maximum vblanking can change when a new format is
> applied to the sensor subdevice, so be sure to retrieve up-to-date
> values.
>
> The V4L2Device acquires the new refreshControls() method to perform
> this function. Note that not all pipeline handlers invoke the
> subdevice's setFormat method directly, so the new method must be made
> publicly available.

I would however call refreshControls() in CameraSensor::setFormat()

>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  2 ++
>  src/libcamera/camera_sensor.cpp          |  7 +++++++
>  src/libcamera/v4l2_device.cpp            | 26 ++++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index d006bf68..6efcb0db 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -41,6 +41,8 @@ public:
>  	int setFrameStartEnabled(bool enable);
>  	Signal<uint32_t> frameStart;
>
> +	void refreshControls();
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device();
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f7ed91d9..9f99ce3e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	info->bitsPerPixel = format.bitsPerPixel();
>  	info->outputSize = format.size;
>
> +	/*
> +	 * Some controls, specifically the vblanking, may have different min
> +	 * or max values if the sensor format has been changed since it was
> +	 * opened, so be sure to update them first.
> +	 */
> +	subdev_->refreshControls();
> +

I feel a bit like it's up to the ph to be aware if controls need to be
refreshed or not. For 'regular' ph this would be transpared, calling
CameraSensor::setFormat() would refresh controls and it is guaranteed
that CameraSensorInfo is always up-to-date.

For ph like yours that use CameraSensor but do not set the format
directly on it, I think it's up to the ph to refresh controls after
you have successfully set a format on the Unicam::Image node (this
would require adding a refreshControls() method to CameraSensor
though, I'm not 100% sure it is worth it :/)

This would allow unnecessary refresh of controls, as keeping the state
in the V4L2Device class through a flag would require to tweak into the
subclasses' setFormat() methods, and it might get nasty, if even
doable in clean way.

All in all, I would be happy with this patch if it would provide a way
to update ControlInfo first, and then use it to update the
V4L2Controls during a refresh. Do you think it would be worth it ?

Thanks
   j

>  	/*
>  	 * Retrieve the pixel rate, line length and minimum/maximum frame
>  	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..6d396f89 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -514,6 +514,32 @@ void V4L2Device::listControls()
>  	controls_ = std::move(ctrls);
>  }
>
> +/*
> + * \brief Update the control information that was previously stored by
> + * listControls(). Some parts of this information, such as min and max
> + * values of some controls, are liable to change when a new format is set.
> + */
> +void V4L2Device::refreshControls()
> +{

And I would here add a flag to the class, something like needRefersh,
to be set to true in
> +	for (auto &controlId : controlIds_) {
> +		unsigned int id = controlId->id();
> +
> +		struct v4l2_query_ext_ctrl ctrl = {};
> +		ctrl.id = id;
> +
> +		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> +			LOG(V4L2, Debug)
> +				<< "Could not refresh control "
> +				<< utils::hex(ctrl.id);
> +			continue;
> +		}
> +
> +		/* Assume these are present - listControls() made them. */
> +		controlInfo_[id] = ctrl;
> +		controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);
> +	}
> +}
> +
>  /*
>   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
>   * values in \a v4l2Ctrls
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
David Plowman April 1, 2021, 9:52 a.m. UTC | #2
Hi Jacopo

Thanks for joining this discussion!

On Wed, 31 Mar 2021 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> I admit I have not followed closely the recent discussions on this
> topic, but this seems an acceptable solution to me, however I wonder
> if it would not be worth taking it a little step forward (I know...)
>
> The problem I see here is fundamentally one: we're missing a method
> to update a (V4L2)ControlInfo. The method itself could be quite
> trivial, a ControlInfo::update(ControlValue min, ...) and one
> V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).

So to help me understand, would

controlInfo.update(min, max, default)
and
v4l2ControlInfo.update(ctrl)

be the same as

controlInfo = ControlInfo(min, max, default)
and
v4l2ControlInfo = V4L2ControlInfo(ctrl)
?

>
> The lack of a mechanism to update a control info has this
> consequences:
> 1) V4L2 controls passed by the ph to the IPA could be stale after a
> sensor format update

Yes - though in my case the stale v4l2 controls are actually within
the CameraSensor class itself.

> 2) the Camera::controls_ might be stale after a Camera::configure()

True. Though in the Raspberry Pi pipeline handler I don't think we
really do anything about this. Should we? We do pass min and max
framerates back out to the application in the metadata.

>
> If you introduce the above suggested update() (or whatever) methods in
> (V4L2)ControlInfo, then they could be used as you're doing here below
> to handle 1)
>
> In order to handle 2) pipeline handlers that register controls would
> have to get a freshened list of V4L2Controls from the CameraSensor (or
> use a freshened CameraSensorInfo) to update the controls they register
> as Camera::controls_, performing the opportune re-calculations (in
> example the IPU3::initControls() function would need to be split in
> calculateExposure(), calculateDurations() etc which will then be
> called at configure() time to update Camera::controls_.
>
> Do you think your proposed solution could work well with
> ControlInfo::update() ?

I think so, though - as per the above - I'm not really clear on the
difference between an update method and using assignment. Do we think
an update method is a bit tidier?

>
> A few minors on the patch
>
> On Tue, Mar 30, 2021 at 05:57:43PM +0100, David Plowman wrote:
> > The minimum and maximum vblanking can change when a new format is
> > applied to the sensor subdevice, so be sure to retrieve up-to-date
> > values.
> >
> > The V4L2Device acquires the new refreshControls() method to perform
> > this function. Note that not all pipeline handlers invoke the
> > subdevice's setFormat method directly, so the new method must be made
> > publicly available.
>
> I would however call refreshControls() in CameraSensor::setFormat()

Yes, that seems a reasonable thing to do.

>
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  2 ++
> >  src/libcamera/camera_sensor.cpp          |  7 +++++++
> >  src/libcamera/v4l2_device.cpp            | 26 ++++++++++++++++++++++++
> >  3 files changed, 35 insertions(+)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index d006bf68..6efcb0db 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -41,6 +41,8 @@ public:
> >       int setFrameStartEnabled(bool enable);
> >       Signal<uint32_t> frameStart;
> >
> > +     void refreshControls();
> > +
> >  protected:
> >       V4L2Device(const std::string &deviceNode);
> >       ~V4L2Device();
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f7ed91d9..9f99ce3e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >       info->bitsPerPixel = format.bitsPerPixel();
> >       info->outputSize = format.size;
> >
> > +     /*
> > +      * Some controls, specifically the vblanking, may have different min
> > +      * or max values if the sensor format has been changed since it was
> > +      * opened, so be sure to update them first.
> > +      */
> > +     subdev_->refreshControls();
> > +
>
> I feel a bit like it's up to the ph to be aware if controls need to be
> refreshed or not. For 'regular' ph this would be transpared, calling
> CameraSensor::setFormat() would refresh controls and it is guaranteed
> that CameraSensorInfo is always up-to-date.

Agree.

>
> For ph like yours that use CameraSensor but do not set the format
> directly on it, I think it's up to the ph to refresh controls after
> you have successfully set a format on the Unicam::Image node (this
> would require adding a refreshControls() method to CameraSensor
> though, I'm not 100% sure it is worth it :/)

Yes, we could make it our pipeline handler's business to cause the
CameraSensor to refresh its V4L2 controls, and then not do it in the
sensorInfo() method. I think CameraSensor would need some kind of
public method for us to call, I don't see that there's another way for
us to get at the stuff within the CameraSensor class.

>
> This would allow unnecessary refresh of controls, as keeping the state
> in the V4L2Device class through a flag would require to tweak into the
> subclasses' setFormat() methods, and it might get nasty, if even
> doable in clean way.

Ah, are we suggesting a public CameraSensor::refresh() method which
merely sets a flag? CameraSensor::setFormat would call it, and indeed
so would our pipeline handler.

Then there's a private method which checks the flag, does the actual
work if it's set, and clears it. CameraSensor::sensorInfo would call
this, as indeed could other parts of the CameraSensor class if they
are using the ControlInfos and feeling a bit nervous. This approach
sounds like it would work too if we prefer it.

Anyway, please let me know what you think on these points and I can
produce a v2 to look at.

Thanks and best regards

David

>
> All in all, I would be happy with this patch if it would provide a way
> to update ControlInfo first, and then use it to update the
> V4L2Controls during a refresh. Do you think it would be worth it ?
>
> Thanks
>    j
>
> >       /*
> >        * Retrieve the pixel rate, line length and minimum/maximum frame
> >        * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index decd19ef..6d396f89 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -514,6 +514,32 @@ void V4L2Device::listControls()
> >       controls_ = std::move(ctrls);
> >  }
> >
> > +/*
> > + * \brief Update the control information that was previously stored by
> > + * listControls(). Some parts of this information, such as min and max
> > + * values of some controls, are liable to change when a new format is set.
> > + */
> > +void V4L2Device::refreshControls()
> > +{
>
> And I would here add a flag to the class, something like needRefersh,
> to be set to true in
> > +     for (auto &controlId : controlIds_) {
> > +             unsigned int id = controlId->id();
> > +
> > +             struct v4l2_query_ext_ctrl ctrl = {};
> > +             ctrl.id = id;
> > +
> > +             if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> > +                     LOG(V4L2, Debug)
> > +                             << "Could not refresh control "
> > +                             << utils::hex(ctrl.id);
> > +                     continue;
> > +             }
> > +
> > +             /* Assume these are present - listControls() made them. */
> > +             controlInfo_[id] = ctrl;
> > +             controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);
> > +     }
> > +}
> > +
> >  /*
> >   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
> >   * values in \a v4l2Ctrls
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi April 1, 2021, 3:42 p.m. UTC | #3
On Thu, Apr 01, 2021 at 10:52:27AM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for joining this discussion!
>
> On Wed, 31 Mar 2021 at 13:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > I admit I have not followed closely the recent discussions on this
> > topic, but this seems an acceptable solution to me, however I wonder
> > if it would not be worth taking it a little step forward (I know...)
> >
> > The problem I see here is fundamentally one: we're missing a method
> > to update a (V4L2)ControlInfo. The method itself could be quite
> > trivial, a ControlInfo::update(ControlValue min, ...) and one
> > V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).
>
> So to help me understand, would
>
> controlInfo.update(min, max, default)
> and
> v4l2ControlInfo.update(ctrl)
>
> be the same as
>
> controlInfo = ControlInfo(min, max, default)
> and
> v4l2ControlInfo = V4L2ControlInfo(ctrl)
> ?
>

It's just a syntactic sugar maybe, but I was wondering if it would be
safest in case application takes references to the Camera::controls_..


> >
> > The lack of a mechanism to update a control info has this
> > consequences:
> > 1) V4L2 controls passed by the ph to the IPA could be stale after a
> > sensor format update
>
> Yes - though in my case the stale v4l2 controls are actually within
> the CameraSensor class itself.
>

Yup, through the CameraSensorInfo I meant..

> > 2) the Camera::controls_ might be stale after a Camera::configure()
>
> True. Though in the Raspberry Pi pipeline handler I don't think we
> really do anything about this. Should we? We do pass min and max
> framerates back out to the application in the metadata.
>

Currently no pipeline does that afict, and I thought we had a todo for
"update this control when a mechanism to do so exists" but it's gone
through reviews of the frame durations series probably...

> >
> > If you introduce the above suggested update() (or whatever) methods in
> > (V4L2)ControlInfo, then they could be used as you're doing here below
> > to handle 1)
> >
> > In order to handle 2) pipeline handlers that register controls would
> > have to get a freshened list of V4L2Controls from the CameraSensor (or
> > use a freshened CameraSensorInfo) to update the controls they register
> > as Camera::controls_, performing the opportune re-calculations (in
> > example the IPU3::initControls() function would need to be split in
> > calculateExposure(), calculateDurations() etc which will then be
> > called at configure() time to update Camera::controls_.
> >
> > Do you think your proposed solution could work well with
> > ControlInfo::update() ?
>
> I think so, though - as per the above - I'm not really clear on the
> difference between an update method and using assignment. Do we think
> an update method is a bit tidier?
>

that and the concern about apps taking pointers to them. But it's a
minor indeed.

> >
> > A few minors on the patch
> >
> > On Tue, Mar 30, 2021 at 05:57:43PM +0100, David Plowman wrote:
> > > The minimum and maximum vblanking can change when a new format is
> > > applied to the sensor subdevice, so be sure to retrieve up-to-date
> > > values.
> > >
> > > The V4L2Device acquires the new refreshControls() method to perform
> > > this function. Note that not all pipeline handlers invoke the
> > > subdevice's setFormat method directly, so the new method must be made
> > > publicly available.
> >
> > I would however call refreshControls() in CameraSensor::setFormat()
>
> Yes, that seems a reasonable thing to do.
>
> >
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/v4l2_device.h |  2 ++
> > >  src/libcamera/camera_sensor.cpp          |  7 +++++++
> > >  src/libcamera/v4l2_device.cpp            | 26 ++++++++++++++++++++++++
> > >  3 files changed, 35 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > > index d006bf68..6efcb0db 100644
> > > --- a/include/libcamera/internal/v4l2_device.h
> > > +++ b/include/libcamera/internal/v4l2_device.h
> > > @@ -41,6 +41,8 @@ public:
> > >       int setFrameStartEnabled(bool enable);
> > >       Signal<uint32_t> frameStart;
> > >
> > > +     void refreshControls();
> > > +
> > >  protected:
> > >       V4L2Device(const std::string &deviceNode);
> > >       ~V4L2Device();
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index f7ed91d9..9f99ce3e 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > >       info->bitsPerPixel = format.bitsPerPixel();
> > >       info->outputSize = format.size;
> > >
> > > +     /*
> > > +      * Some controls, specifically the vblanking, may have different min
> > > +      * or max values if the sensor format has been changed since it was
> > > +      * opened, so be sure to update them first.
> > > +      */
> > > +     subdev_->refreshControls();
> > > +
> >
> > I feel a bit like it's up to the ph to be aware if controls need to be
> > refreshed or not. For 'regular' ph this would be transpared, calling
> > CameraSensor::setFormat() would refresh controls and it is guaranteed
> > that CameraSensorInfo is always up-to-date.
>
> Agree.
>
> >
> > For ph like yours that use CameraSensor but do not set the format
> > directly on it, I think it's up to the ph to refresh controls after
> > you have successfully set a format on the Unicam::Image node (this
> > would require adding a refreshControls() method to CameraSensor
> > though, I'm not 100% sure it is worth it :/)
>
> Yes, we could make it our pipeline handler's business to cause the
> CameraSensor to refresh its V4L2 controls, and then not do it in the
> sensorInfo() method. I think CameraSensor would need some kind of
> public method for us to call, I don't see that there's another way for
> us to get at the stuff within the CameraSensor class.
>

I didn't notice the method was public already, and I was not a 100%
sure making it public would be worth it. Although I'm not super-happy
even about my proposal, missing a call to refreshControl() might cause nasty
errors during pipeline development, if that operation could be done
automatically that would be better. Your proposal does that for
sensorInfo(), we could do the same in CameraSensor::controls(), but
that would cause unnecessary refreshes if the state is not kept
somewhere...

All in all I would prefer calling refresh() at setFormat() time as a
public method that ph that knows what they're doing have to call by
themselves.. Also, I would add a note in the CameraSensor::sensorInfo()
and controls() documentation that states that ph that do not use
setFormat() have to refresh controls before accessing them. But I'm
not thrilled, so just take this a suggestion..

> >
> > This would allow unnecessary refresh of controls, as keeping the state
> > in the V4L2Device class through a flag would require to tweak into the
> > subclasses' setFormat() methods, and it might get nasty, if even
> > doable in clean way.
>
> Ah, are we suggesting a public CameraSensor::refresh() method which
> merely sets a flag? CameraSensor::setFormat would call it, and indeed
> so would our pipeline handler.
>

Not really, I was just mumbling on how hard it would be to have the state
flag kept at the video device level :) but your idea is good too!

> Then there's a private method which checks the flag, does the actual
> work if it's set, and clears it. CameraSensor::sensorInfo would call
> this, as indeed could other parts of the CameraSensor class if they
> are using the ControlInfos and feeling a bit nervous. This approach
> sounds like it would work too if we prefer it.
>
> Anyway, please let me know what you think on these points and I can
> produce a v2 to look at.
>
> Thanks and best regards
>
> David
>
> >
> > All in all, I would be happy with this patch if it would provide a way
> > to update ControlInfo first, and then use it to update the
> > V4L2Controls during a refresh. Do you think it would be worth it ?
> >
> > Thanks
> >    j
> >
> > >       /*
> > >        * Retrieve the pixel rate, line length and minimum/maximum frame
> > >        * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index decd19ef..6d396f89 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -514,6 +514,32 @@ void V4L2Device::listControls()
> > >       controls_ = std::move(ctrls);
> > >  }
> > >
> > > +/*
> > > + * \brief Update the control information that was previously stored by
> > > + * listControls(). Some parts of this information, such as min and max
> > > + * values of some controls, are liable to change when a new format is set.
> > > + */
> > > +void V4L2Device::refreshControls()
> > > +{
> >
> > And I would here add a flag to the class, something like needRefersh,
> > to be set to true in
> > > +     for (auto &controlId : controlIds_) {
> > > +             unsigned int id = controlId->id();
> > > +
> > > +             struct v4l2_query_ext_ctrl ctrl = {};
> > > +             ctrl.id = id;
> > > +
> > > +             if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> > > +                     LOG(V4L2, Debug)
> > > +                             << "Could not refresh control "
> > > +                             << utils::hex(ctrl.id);
> > > +                     continue;
> > > +             }
> > > +
> > > +             /* Assume these are present - listControls() made them. */
> > > +             controlInfo_[id] = ctrl;
> > > +             controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);
> > > +     }
> > > +}
> > > +
> > >  /*
> > >   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
> > >   * values in \a v4l2Ctrls
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index d006bf68..6efcb0db 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -41,6 +41,8 @@  public:
 	int setFrameStartEnabled(bool enable);
 	Signal<uint32_t> frameStart;
 
+	void refreshControls();
+
 protected:
 	V4L2Device(const std::string &deviceNode);
 	~V4L2Device();
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index f7ed91d9..9f99ce3e 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -796,6 +796,13 @@  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
 	info->bitsPerPixel = format.bitsPerPixel();
 	info->outputSize = format.size;
 
+	/*
+	 * Some controls, specifically the vblanking, may have different min
+	 * or max values if the sensor format has been changed since it was
+	 * opened, so be sure to update them first.
+	 */
+	subdev_->refreshControls();
+
 	/*
 	 * Retrieve the pixel rate, line length and minimum/maximum frame
 	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index decd19ef..6d396f89 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -514,6 +514,32 @@  void V4L2Device::listControls()
 	controls_ = std::move(ctrls);
 }
 
+/*
+ * \brief Update the control information that was previously stored by
+ * listControls(). Some parts of this information, such as min and max
+ * values of some controls, are liable to change when a new format is set.
+ */
+void V4L2Device::refreshControls()
+{
+	for (auto &controlId : controlIds_) {
+		unsigned int id = controlId->id();
+
+		struct v4l2_query_ext_ctrl ctrl = {};
+		ctrl.id = id;
+
+		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
+			LOG(V4L2, Debug)
+				<< "Could not refresh control "
+				<< utils::hex(ctrl.id);
+			continue;
+		}
+
+		/* Assume these are present - listControls() made them. */
+		controlInfo_[id] = ctrl;
+		controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);
+	}
+}
+
 /*
  * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
  * values in \a v4l2Ctrls