Message ID | 20190626101308.19099-1-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, On 26/06/2019 11:13, Jacopo Mondi wrote: > When enumerating the available V4L2 controls at video device open > 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@jmondi.org> Reviewed-by: Kieran Bingham <kieran.bingham@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; > } > >
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@jmondi.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@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; } }
Hi Laurent, On Wed, Jun 26, 2019 at 04:44:57PM +0300, Laurent Pinchart wrote: > 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@jmondi.org> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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. Thanks j > > 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
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@jmondi.org> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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; > > } > > } > >
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@jmondi.org> > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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; }
When enumerating the available V4L2 controls at video device open time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported control type not to loop forever. While at it, downgrade the message reporting the control type in not 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@jmondi.org> --- src/libcamera/v4l2_device.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)