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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

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 ?

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