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

[Kieran Bingham]

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.

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