| Message ID | 20210311122040.1264410-5-ribalda@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: Pass v4l2-compliance test | expand |
As discussed in the IRC with Hans We need to specify in the commit message that this is most likely due to hw error. On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > test VIDIOC_G/S_CTRL: FAIL > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index f2f565281e63..5442e9be1c55 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > case 6: /* Invalid control */ > case 7: /* Invalid Request */ > case 8: /* Invalid value within range */ > - return -EINVAL; > + return -EIO; > default: /* reserved or unknown */ > break; > } > -- > 2.31.0.rc2.261.g7f71774620-goog >
On 11/03/2021 13:20, Ricardo Ribalda wrote: > Fixes v4l2-compliance: > > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > test VIDIOC_G/S_CTRL: FAIL > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL -EIO is specifically meant for FW/HW issues. So make clear in this commit log that if an error occurs in the code at that place, then that's because of the device doing something unexpected. Actually, that should be in a comment before the 'return -EIO;'. Regards, Hans > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index f2f565281e63..5442e9be1c55 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > case 6: /* Invalid control */ > case 7: /* Invalid Request */ > case 8: /* Invalid value within range */ > - return -EINVAL; > + return -EIO; > default: /* reserved or unknown */ > break; > } >
Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote: > As discussed in the IRC with Hans > > We need to specify in the commit message that this is most likely due > to hw error. > > On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Fixes v4l2-compliance: > > > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > test VIDIOC_G/S_CTRL: FAIL > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL As this isn't supposed to happen, how do you reproduce this ? > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index f2f565281e63..5442e9be1c55 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > case 6: /* Invalid control */ > > case 7: /* Invalid Request */ For cases 5-7 I think -EIO is fine, as the driver should really not call this function with an invalid unit, control or request. If it does, it's a bug in the driver (we can check the units and controls the device claims to support, and the requests are defined by the UVC specification), if it doesn't and the device still returns this error, it's a bug on the device side. > > case 8: /* Invalid value within range */ For this case, however, isn't it valid for a device to return an error if the control value isn't valid ? There's one particular code path I'm concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) -> uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for userspace to know that the value it sets isn't valid. > > - return -EINVAL; > > + return -EIO; > > default: /* reserved or unknown */ > > break; > > }
Hi Laurent On Thu, Mar 11, 2021 at 4:59 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote: > > As discussed in the IRC with Hans > > > > We need to specify in the commit message that this is most likely due > > to hw error. > > > > On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > > > Fixes v4l2-compliance: > > > > > > Control ioctls (Input 0): > > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > > test VIDIOC_G/S_CTRL: FAIL > > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > As this isn't supposed to happen, how do you reproduce this ? Just run v4l2-compliance in my notebook camera. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > index f2f565281e63..5442e9be1c55 100644 > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > case 6: /* Invalid control */ > > > case 7: /* Invalid Request */ > > For cases 5-7 I think -EIO is fine, as the driver should really not call > this function with an invalid unit, control or request. If it does, it's > a bug in the driver (we can check the units and controls the device > claims to support, and the requests are defined by the UVC > specification), if it doesn't and the device still returns this error, > it's a bug on the device side. > > > > case 8: /* Invalid value within range */ > > For this case, however, isn't it valid for a device to return an error > if the control value isn't valid ? There's one particular code path I'm > concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) -> > uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for > userspace to know that the value it sets isn't valid. > Will fix in v2 Thanks! > > > - return -EINVAL; > > > + return -EIO; > > > default: /* reserved or unknown */ > > > break; > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index f2f565281e63..5442e9be1c55 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, case 6: /* Invalid control */ case 7: /* Invalid Request */ case 8: /* Invalid value within range */ - return -EINVAL; + return -EIO; default: /* reserved or unknown */ break; }
Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) test VIDIOC_G/S_CTRL: FAIL fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)