[libcamera-devel] [PATCH] libcamera: v4l2_device: Fix control enumeration bug

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jun 27, 2019 at 09:51:28AM +0200, Jacopo Mondi wrote:
> On Wed, Jun 26, 2019 at 04:44:57PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 26, 2019 at 11:17:23AM +0100, Kieran Bingham wrote:
> > > On 26/06/2019 11:13, Jacopo Mondi wrote:
> > > > When enumerating the available V4L2 controls at video device open
> >
> > s/video device/V4L2 device/
> >
> > > > time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> > > > control type not to loop forever.
> > >
> > > "set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
> > > encountered to prevent infinite loops."
> > >
> > > > While at it, downgrade the message reporting the control type in not
> > >
> > > "reporting the unsupported control type to Debug"
> > >
> > >
> > > > supported to Debug, as it is not an error worth being reported
> > > > unconditionally.
> > > >
> > > > Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > ---
> > > >  src/libcamera/v4l2_device.cpp | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > index 84758a811c27..13d4bce5c013 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -341,8 +341,9 @@ void V4L2Device::listControls()
> > > >  			break;
> > > >  		/* \todo Support compound controls. */
> > > >  		default:
> > > > -			LOG(V4L2, Error) << "Control type '" << info.type()
> > > > +			LOG(V4L2, Debug) << "Control type '" << info.type()
> > > >  					 << "' not supported";
> > > > +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > >  			continue;
> > > >  		}
> > > >
> >
> > Would the following (untested) change be a better solution, to avoid
> > or'ing V4L2_CTRL_FLAG_NEXT_CTRL in multiple places ?
> 
> It is. I'll apply and merge. I assume I can ad your tag.

Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 84758a811c27..f74c5b2aff02 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -321,13 +321,15 @@ void V4L2Device::listControls()
> >  	struct v4l2_query_ext_ctrl ctrl = {};
> >
> >  	/* \todo Add support for menu and compound controls. */
> > -	ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
> > -	while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
> > +	while (1) {
> > +		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +		int ret = ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl);
> > +		if (ret < 0)
> > +			break;
> > +
> >  		if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
> > -		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
> > -			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> >  			continue;
> > -		}
> >
> >  		V4L2ControlInfo info(ctrl);
> >  		switch (info.type()) {
> > @@ -347,7 +349,6 @@ void V4L2Device::listControls()
> >  		}
> >
> >  		controls_.emplace(ctrl.id, info);
> > -		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> >  	}
> >  }
> >

-- 
Regards,

Laurent Pinchart