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

[Laurent Pinchart]

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

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.

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