[libcamera-devel] [PATCH v3 0/2] Add read-only flag to ControlInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 6 01:00:44 CET 2023
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(-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list