[libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report mandatory control names

[Laurent Pinchart]

Hi Kieran,

On Mon, Feb 08, 2021 at 04:47:46PM +0000, Kieran Bingham wrote:
> On 08/02/2021 16:21, Laurent Pinchart wrote:
> > On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote:
> >> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:
> >>> On 06/02/2021 10:12, Kieran Bingham wrote:
> >>>> On 06/02/2021 07:43, Laurent Pinchart wrote:
> >>>>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:
> >>>>>> We can not obtain the control names directly from V4L2 so create
> >>>>>> a map of the control name, when declaring the list of mandatory
> >>>>>> controls to enable a human readable print of any missing requirements.
> >>>>>
> >>>>> If we want to print V4L2 controls by name to ease debugging, it would be
> >>>>> best not to limit the feature to this particular instance. I've been
> >>>>> toying for a long time with the idea of creating Control instances for
> >>>>> V4L2 controls, which would also allow the usage of a nicer get() and
> >>>>> set() in the ControlList class.
> >>>>
> >>>> But we already establish controls with names from V4L2 when they are
> >>>> available don't we?
> >>>>
> >>>> In src/libcamera/v4l2_controls.cpp, there is a helper
> >>>>
> >>>>   std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
> >>>>
> >>>> which is used to establish a V4L2ControlId after it has been queried
> >>>> from the kernel.
> >>>>
> >>>> The key issue here is that at the point of reporting the Mandatory
> >>>> Controls are not available on the device - well - they don't exist and
> >>>> thus haven't been queried, so there is no name available.
> >>>>
> >>>>
> >>>> Do you envisage a static list of *all* v4l2 controls, over just the 4
> >>>> which are 'required' here? Or that we add all v4l2 controls we use to
> >>>> the static declarations perhaps? (That could be handled just like the
> >>>> other controls then)
> >>>>
> >>>> Right now - with the recent addition of mandatory controls and many
> >>>> sensor drivers not supporting all the required controls, we have several
> >>>> instances of people hitting failures and being told an obscure hex value
> >>>> in the error reporting, which is not very helpful to an end user trying
> >>>> to debug the issue of why their capture which worked a couple of weeks
> >>>> ago now fails.
> >>>>
> >>>> I don't expect that we wish to provide a manual lookup service every
> >>>> time someone reports or queries what those values are - so I'd hope we
> >>>> can have a solution to this quite quickly.
> >>>
> >>> Indeed of course this is more than just the mandatory controls:
> >>>
> >>>> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported
> >>>> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported
> >>>
> >>> It might be optional, but it would be very nice to know what it is ;-)
> >>
> >> Personal opinion: if one is interesting in knowing why the driver
> >> validation fails, I hope she would not only rely on the debug
> >> printouts but rather turn to the code and cross-check for the controls
> 
> The problem is the hex values are opaque from a human-readable
> perspective. The VBLANK control was only added at the end of last week,
> and there was already a misunderstanding that it was a missing HBLANK
> control preventing the running of mainline master, until it was clear
> that actually we were now failing on a missing VBLANK.
> 
> Printing hex values just feels like we're saying "Oh we don't care about
> users, or others having to debug the situation".

I think that Jacopo's point, and mine, was that this message is really
for developers, not end users, and developers who would need to add
controls to a camera sensor kernel driver are expected to be able to
read v4l2-controls.h.

That being said, a name is of course nicer to read, and not just here.
My concern was thus whether printing a name here was important enough to
call for a ad-hoc short term fix, or if we should instead take one extra
step. In hindsight, I've probably underestimated the lack of
user-friendliness of hex values in this case.

> >> presence in the driver. An ad-hoc solution for this is nice, but it's
> >> a bit a waste of effort, considering how much nicer would our control
> >> framework be if we could get rid of the V4L2Control indexed by id and
> >> treat all controls we deal with (libcamera and v4l2) as ControlId *
> 
> Waste of effort or interim solution? It was a 15 minute patch +
> discussion (hence RFC of course).

Turns out the discussion is the most time consuming part :-)

> > I think I generally agree.
> > 
> >> We import v4l2-controls.h so we have all we need to parse and create
> >> ControlId instances at build time from there. It's not a 2 hours task
> >> unfortunately as it require quite some some of parsing and
> >> substitution.
> > 
> > We need type information too, which we can't get from the
> > v4l2-controls.h header. I was thus considering a YAML file listing the
> > V4L2 controls we need.
> 
> So how do we make this happen?

Please see the RFC series on the list, let's continue the discussion
there.

> I agree that a full control interface would be nicer, but that is going
> to take time that none of us have right now.
> 
> (Though we did have volunteers looking for tasks recently didn't we?)
> 
> 
> This is why I would like to still consider an interim solution, because
> we have actively chosen to cause breakage - and that breakage results in
> people seeing hex codes as the 'summary' which isn't friendly or helpful
> to end users (or ... me....)
> 
> >>>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>>>> ---
> >>>>>>  src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------
> >>>>>>  1 file changed, 13 insertions(+), 8 deletions(-)
> >>>>>>
> >>>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> >>>>>> index c9e8d49b7935..9a510108f171 100644
> >>>>>> --- a/src/libcamera/camera_sensor.cpp
> >>>>>> +++ b/src/libcamera/camera_sensor.cpp
> >>>>>> @@ -346,18 +346,23 @@ int CameraSensor::validateSensorDriver()
> >>>>>>  	 * For raw sensors, make sure the sensor driver supports the controls
> >>>>>>  	 * required by the CameraSensor class.
> >>>>>>  	 */
> >>>>>> -	static constexpr uint32_t mandatoryControls[] = {
> >>>>>> -		V4L2_CID_EXPOSURE,
> >>>>>> -		V4L2_CID_HBLANK,
> >>>>>> -		V4L2_CID_PIXEL_RATE,
> >>>>>> -		V4L2_CID_VBLANK,
> >>>>>> +	static struct {
> >>>>>> +		uint32_t ctrl;
> >>>>>> +		std::string name;
> >>>>>> +	} mandatoryControls[] = {
> >>>>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }
> >>>>>> +		MANDATORY_CONTROL(V4L2_CID_EXPOSURE),
> >>>>>> +		MANDATORY_CONTROL(V4L2_CID_HBLANK),
> >>>>>> +		MANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),
> >>>>>> +		MANDATORY_CONTROL(V4L2_CID_VBLANK),
> >>>>>> +#undef MANDATORY_CONTROL
> >>>>>>  	};
> >>>>>>
> >>>>>>  	err = 0;
> >>>>>> -	for (uint32_t ctrl : mandatoryControls) {
> >>>>>> -		if (!controls.count(ctrl)) {
> >>>>>> +	for (auto &c : mandatoryControls) {
> >>>>>> +		if (!controls.count(c.ctrl)) {
> >>>>>>  			LOG(CameraSensor, Error)
> >>>>>> -				<< "Mandatory V4L2 control " << utils::hex(ctrl)
> >>>>>> +				<< "Mandatory V4L2 control " << c.name
> >>>>>>  				<< " not available";
> >>>>>>  			err = -EINVAL;
> >>>>>>  		}

-- 
Regards,

Laurent Pinchart