| Message ID | 20210311122040.1264410-9-ribalda@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvcvideo: Pass v4l2-compliance test | expand |
Hi Laurent On Thu, Mar 11, 2021 at 5:20 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. Thank you for the review :) > > On Thu, Mar 11, 2021 at 01:20:37PM +0100, Ricardo Ribalda wrote: > > According to the doc: > > The previous paragraph states: > > This check is done to avoid leaving the hardware in an inconsistent > state due to easy-to-avoid problems. But it leads to another problem: > the application needs to know whether an error came from the validation > step (meaning that the hardware was not touched) or from an error during > the actual reading from/writing to hardware. I think we wrote his standard when we were young and naive ;) > > > The, in hindsight quite poor, solution for that is to set error_idx to > > count if the validation failed. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(645): invalid error index write only control > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 625c216c46b5..9b6454bb2f28 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > > ret = uvc_ctrl_get(chain, ctrl); > > if (ret < 0) { > > uvc_ctrl_rollback(handle); > > - ctrls->error_idx = i; > > + ctrls->error_idx = (ret == -EACCES) ? > > + ctrls->count : i; > > No need for parentheses. I really like my parenthesis before the ? :. Can I leave it? :) If it bothers you a lot I remove it, but I like to delimit where the ternary operators end. > > I'm not sure this is correct though. -EACCES is returned by > __uvc_ctrl_get() when the control is found and is a write-only control. > The uvc_ctrl_get() calls for the previous controls will have potentially > touched the device to read the current control value if it wasn't cached > already, to this contradicts the rationale from the specification. > > I understand the need for this when setting controls, but when reading > them, it's more puzzling, as the interactions with the hardware to read > the controls are not supposed to affect the hardware state in a way that > applications should care about. It may be an issue in the V4L2 > specification. I have no strong opinions in both directions. If v4l2-compliance is the de-facto standard I believe we should keep this patch or change the compliance test. Hans, what do think? > > > return ret; > > } > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 625c216c46b5..9b6454bb2f28 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, ret = uvc_ctrl_get(chain, ctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - ctrls->error_idx = i; + ctrls->error_idx = (ret == -EACCES) ? + ctrls->count : i; return ret; } }
According to the doc: The, in hindsight quite poor, solution for that is to set error_idx to count if the validation failed. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(645): invalid error index write only control test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)