On Thu December 27 2012 12:59:15 Hans Verkuil wrote:
> On Wed December 26 2012 12:33:58 Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Tuesday 25 December 2012 12:50:51 Hans Verkuil wrote:
> > > On Tue December 25 2012 12:23:00 Laurent Pinchart wrote:
> > > > On Tuesday 25 December 2012 12:15:25 Hans Verkuil wrote:
> > > > > On Mon December 24 2012 13:27:08 Laurent Pinchart wrote:
> > > > > > On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> > > > > > > When one of the requested controls doesn't exist the error_idx field
> > > > > > > must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS,
> > > > > > > error_idx must be set to the control count. For TRY_EXT_CTRLS, it
> > > > > > > must be set to the index of the unexisting control.
> > > > > > >
> > > > > > > This issue was found by the v4l2-compliance tool.
> > > > > >
> > > > > > I'm revisiting this patch as it has been reverted in v3.8-rc1.
> > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >
> > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 17 ++++++++++-------
> > > > > > > drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++-------
> > > > > > > 2 files changed, 22 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > > > > @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
> > > > > > > if (ret < 0) {
> > > > > > > uvc_ctrl_rollback(handle);
> > > > > > >
> > > > > > > - ctrls->error_idx = i;
> > > > > > > - return ret;
> > > > > > > + ctrls->error_idx = ret == -ENOENT
> > > > > > > + ? ctrls->count : i;
> > > > > > > + return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > }
> > > > > > > }
> > > > > > > ctrls->error_idx = 0;
> > > > > > > @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file
> > > > > > > *file,
> > > > > > > unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
> > > > > > > if (ret < 0) {
> > > > > > > uvc_ctrl_rollback(handle);
> > > > > > >
> > > > > > > - ctrls->error_idx = i;
> > > > > > > - return ret;
> > > > > > > + ctrls->error_idx = (ret == -ENOENT &&
> > > > > > > + cmd == VIDIOC_S_EXT_CTRLS)
> > > > > > > + ? ctrls->count : i;
> > > > > > > + return ret == -ENOENT ? -EINVAL : ret;
> > > > > > > }
> > > > > > > }
> > > > > >
> > > > > > I've reread the V4L2 specification, and the least I can say is that
> > > > > > the text is pretty ambiguous. Let's clarify it.
> > > > > >
> > > > > > Is there a reason to differentiate between invalid control IDs and
> > > > > > other errors as far as error_idx is concerned ? It would be simpler if
> > > > > > error_idx was set to the index of the first error for get and try
> > > > > > operations, regardless of the error type. What do you think ?
> > > > >
> > > > > There is a good reason for doing this: the G/S_EXT_CTRLS ioctls have to
> > > > > be as atomic as possible, i.e. it should try hard to prevent leaving the
> > > > > hardware in an inconsistent state because not all controls could be set.
> > > > > It can never be fully atomic since writing multiple registers over usb
> > > > > or i2c can always return errors for one of those writes, but it should
> > > > > certainly check for all the obvious errors first that do not require
> > > > > actually writing to the hardware, such as whether all the controls in
> > > > > the control list actually exist.
> > > > >
> > > > > And for such errors error_idx should be set to the number of controls to
> > > > > indicate that none of the controls were actually set but that there was
> > > > > a problem with the list of controls itself.
> > > >
> > > > For S_EXT_CTRLS, sure, but G_EXT_CTRLS doesn't modify the hardware state,
> > > > so it could get all controls up to the erroneous one.
> > >
> > > I have thought about that but I decided against it. One reason is to have
> > > get and set behave the same since both access the hardware. The other
> > > reason is that even getting a control value might change the hardware
> > > state, for example by resetting some internal hardware counter when a
> > > register is read (it's rare but there is hardware like that). Furthermore,
> > > reading hardware registers can be slow so why not do the sanity check
> > > first?
> >
> > Get can indeed change the device state in rare cases, but the information
> > won't be lost, as the value of all controls before error_idx will be returned.
> >
> > What bothers me with the current G_EXT_CTRLS implementation (beside that it's
> > very slightly more complex for the uvcvideo driver than the one I propose) is
> > that an application will have no way to know for which control G_EXT_CTRLS
> > failed. This is especially annoying during development.
>
> For S_EXT_CTRLS you can call TRY_EXT_CTRLS first to check which control failed,
> but you don't have that option for G_EXT_CTRLS. That's actually something I
> hadn't considered.
>
> > Maybe we could leave this behaviour as driver-specific ?
I don't like the idea of leaving this driver-specific. That always bites you
in the end.
I thought seriously about changing the spec so G_EXT_CTRLS behaves like
TRY_EXT_CTRLS when it comes to setting error_idx, but I decided against that.
The main reason is that if G_EXT_CTRLS returns an error_idx < count, then you no
longer know whether the values of the controls up to error_idx-1 were actually
retrieved or not. v4l2-ctrls.c does sanity checks up front, so if it returns
an error for control index 2, does that mean that the sanity checks failed
in which case no controls were retrieved yet, or that getting control index
2 failed due to some hardware-related problem, in which case controls 0 and 1
*were* successfully retrieved.
In addition, changing the behavior means changing the API, albeit in a very
minor way, and I don't think it is worth doing that in this case.
I will prepare a patch that clarifies the API, though. It can certainly be
improved.
Also note that I agree that the situation is not ideal and if I would write
the API from scratch I'd probably handle this a bit differently, most likely
by adding some flags field that can be used to give more precise information.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
