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

[Kieran Bingham]

Hi,

On 08/02/2021 16:21, Laurent Pinchart wrote:
> Hello,
> 
> 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".

>> 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).

> 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?

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....)

--
Kieran



>>>>>> 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
--
Kieran