[libcamera-devel,v3,0/2] Add read-only flag to ControlInfo

19187 mbox series

This small patch series adds a read-only flag to ControlInfo.  This is used
to test if a control, in particular V4L2_CID_HBLANK, can be written to by
userland.  It replaces the slightly fragile workaround we have right now where
we test if the min and max values of a control are the same to determine if
a control is read-only.

Only modified one ControlInfo constructor is modified which is used by the
V4L2Device class to allow this flag to be set, as setting it for a non-v4l2
control probably does not make sense at this point.

Modifying other ControlInfo constructors fails quite quickly falling
into a range of template issues due to multiple signatures matching.

The alternative here would be to rework this such that the ReadOnly flag
could be set /after/ construction with a setter allowing something like:

V4L2_CTRL_TYPE_INTEGER64:
 		return ControlInfo(static_cast<int64_t>(ctrl.minimum),
 				   static_cast<int64_t>(ctrl.maximum),
-				   static_cast<int64_t>(ctrl.default_value));
+				   static_cast<int64_t>(ctrl.default_value))
					.setReadOnly(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY);

Of course that feels like it could remove the 'contract' of the read
only property by opening up the ability for a ReadOnly control to have
the ReadOnly flag unset - or by opening up the ability to arbitrarily
affect a 'property' of a ControlInfo after it has been made.


Since v2, I have made the following changes to Naush' patches:
 - Initialised ReadOnly flag as false for ControlInfo(Span<>)
 - Improved documentation

Naushir Patuck (2):
  libcamera: controls: Add read-only flag to ControlInfo
  libcamera:camera_sensor, ipa: raspberrypi: Test readOnly() for HBLANK
    control

 include/libcamera/controls.h    |  5 ++++-
 src/ipa/rpi/common/ipa_base.cpp | 12 +-----------
 src/libcamera/camera_sensor.cpp | 14 ++------------
 src/libcamera/controls.cpp      | 22 ++++++++++++++++++----
 src/libcamera/v4l2_device.cpp   | 12 ++++++++----
 5 files changed, 33 insertions(+), 32 deletions(-)

Hi Kieran,

Thank you for the patches.

On Thu, Nov 02, 2023 at 05:24:37PM +0000, Kieran Bingham via libcamera-devel wrote:
> This small patch series adds a read-only flag to ControlInfo.  This is used
> to test if a control, in particular V4L2_CID_HBLANK, can be written to by
> userland.  It replaces the slightly fragile workaround we have right now where
> we test if the min and max values of a control are the same to determine if
> a control is read-only.

While that's indeed a bit of a hack, do we have drivers that expose
HBLANK as read-only with different minimum and maximum values ? I don't
see any mention in the cover letter or in the patches of what problem(s)
this series fixes.

> Only modified one ControlInfo constructor is modified which is used by the
> V4L2Device class to allow this flag to be set, as setting it for a non-v4l2
> control probably does not make sense at this point.

This is the part that bothers me a bit. If the feature is only used
internally, it shouldn't be exposed in the public API.

One possible workaround would be to add flag controls as being settable
in a request and as being reported in metadata. This is a feature that
is useful for applications, and it could then be used internally do
indicate read-only internal controls.

> Modifying other ControlInfo constructors fails quite quickly falling
> into a range of template issues due to multiple signatures matching.
> 
> The alternative here would be to rework this such that the ReadOnly flag
> could be set /after/ construction with a setter allowing something like:
> 
> V4L2_CTRL_TYPE_INTEGER64:
>  		return ControlInfo(static_cast<int64_t>(ctrl.minimum),
>  				   static_cast<int64_t>(ctrl.maximum),
> -				   static_cast<int64_t>(ctrl.default_value));
> +				   static_cast<int64_t>(ctrl.default_value))
> 					.setReadOnly(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY);
> 
> Of course that feels like it could remove the 'contract' of the read
> only property by opening up the ability for a ReadOnly control to have
> the ReadOnly flag unset - or by opening up the ability to arbitrarily
> affect a 'property' of a ControlInfo after it has been made.
> 
> 
> Since v2, I have made the following changes to Naush' patches:
>  - Initialised ReadOnly flag as false for ControlInfo(Span<>)
>  - Improved documentation
> 
> Naushir Patuck (2):
>   libcamera: controls: Add read-only flag to ControlInfo
>   libcamera:camera_sensor, ipa: raspberrypi: Test readOnly() for HBLANK
>     control
> 
>  include/libcamera/controls.h    |  5 ++++-
>  src/ipa/rpi/common/ipa_base.cpp | 12 +-----------
>  src/libcamera/camera_sensor.cpp | 14 ++------------
>  src/libcamera/controls.cpp      | 22 ++++++++++++++++++----
>  src/libcamera/v4l2_device.cpp   | 12 ++++++++----
>  5 files changed, 33 insertions(+), 32 deletions(-)