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

[Kieran Bingham]

On 06/02/2021 10:12, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 06/02/2021 07:43, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> 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 ;-)

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