| Message ID | 20210311122040.1264410-9-ribalda@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: Pass v4l2-compliance test | expand |
Hi Ricardo, Thank you for the patch. 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. > 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'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. > return ret; > } > }
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(-)