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

[Jacopo Mondi]

Hello,

On Thu, Jun 27, 2019 at 11:08:18AM +0300, Laurent Pinchart wrote:
> 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>
>

Thanks, now pushed to master

> > > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190627/dc27979d/attachment.sig>