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

[Jacopo Mondi]

Hi Kieran,

On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:
> 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 ;-)

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 *

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.
>
> --
> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel