Hi Hans, Thanks for the patch. On Monday 07 January 2013 13:09:47 Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > The documentation of the error_idx field was incomplete and confusing. > This patch improves it. > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > --- > .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 51 ++++++++++++++--- > 1 file changed, 44 insertions(+), 7 deletions(-) (Text reflowed for easier review) > diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index > 0a4b90f..c07c657 100644 > --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > @@ -199,13 +199,50 @@ also be zero.</entry> > <row> > <entry>__u32</entry> > <entry><structfield>error_idx</structfield></entry> > - <entry>Set by the driver in case of an error. If it is equal > -to <structfield>count</structfield>, then no actual changes were made to > -controls. In other words, the error was not associated with setting a > -particular control. If it is another value, then only the controls up to > -<structfield>error_idx-1</structfield> were modified and control > -<structfield>error_idx</structfield> is the one that caused the error. The > -<structfield>error_idx</structfield> value is undefined if the ioctl > -returned 0 (success).</entry> > + <entry><para>Set by the driver in case of an error. If the error is > +associated with a particular control, then > +<structfield>error_idx</structfield> is set to the index of that control. > +If the error is not related to a specific control, then > +<structfield>error_idx</structfield> is set to > +<structfield>count</structfield>.</para> > +<para>The behavior is different for <constant>VIDIOC_G_EXT_CTRLS</constant> > +and <constant>VIDIOC_S_EXT_CTRLS</constant>: if > +<structfield>error_idx</structfield> is equal to > +<structfield>count</structfield>, then no actual changes were made to the > +controls. For example, if you try to write to a read-only control, then > +<constant>VIDIOC_TRY_EXT_CTRLS</constant> will set > +<structfield>error_idx</structfield> to the index of that read-only > +control, but <constant>VIDIOC_S_EXT_CTRLS</constant> will set > +<structfield>error_idx</structfield> to <structfield>count</structfield> > +instead and none of the controls in the list will be set.</para> I find this a bit confusing (although I understand what you mean, as I'm familiar with the API). You start by mentioning [GS]_EXT_CTRLS only, and then you introduce TRY_EXT_CTRLS in the example. I think it would be clearer if you restructed the explanation as follows: - explain the error_idx principle globally, withotu examples - explain that [GS]_EXT_CTRLS will perform pre-validation and will thus set error_idx to count in specific cases (they should be listed) - explain that TRY_EXT_CTRLS doesn't perform pre-validation and will thus set error_idx to the index of the erroneous control in all cases, including the specific cases listed for [GS]_EXT_CTRLS > +<para>The same is true when trying to e.g. read a write-only control: > +<constant>VIDIOC_G_EXT_CTRLS</constant> will set > +<structfield>error_idx</structfield> to <structfield>count</structfield> > +and none of the controls in the list will have been retrieved.</para> > + > +<para>The reason for this behavior is that it is important when setting and > +getting controls to do this as atomically as possible, so simple sanity > +checks like testing for read-only controls are done first before an > +attempt is made to apply/retrieve the new control values to/from the > +hardware. It is important for an application to know whether > +<constant>VIDIOC_S_EXT_CTRLS</constant> or > +<constant>VIDIOC_G_EXT_CTRLS</constant> actually made changes to controls > +or not. So if <structfield>error_idx</structfield> is equal to > +<structfield>count</structfield>, then you know that no actual controls > +were set or retrieved. In the case of > +<constant>VIDIOC_S_EXT_CTRLS</constant> you can call > +<constant>VIDIOC_TRY_EXT_CTRLS</constant> with the same control list to > +see if the problem was with a specific control or not > +(<constant>VIDIOC_TRY_EXT_CTRLS</constant> never modifies controls, so > +that ioctl will just set <structfield>error_idx</structfield> to the > +control that caused the problem). No such option exists for > +<constant>VIDIOC_G_EXT_CTRLS</constant> though, unfortunately.</para> It's very unfortunate indeed :-) Given that the hardware state must not be changed by a read operation, are you sure G_EXT_CTRLS couldn't retrieve controls up to the erroneous one ? ;-) I think some flexibility should still probably be left to drivers (and I'm not talking about UVC here), as some drivers might not be able to know that a control is write-only before trying to read it and getting an error. > +<para>If <structfield>error_idx</structfield> as returned by > +<constant>VIDIOC_S_EXT_CTRLS</constant> or > +<constant>VIDIOC_G_EXT_CTRLS</constant> is less than > +<structfield>count</structfield>, then only the controls up to > +<structfield>error_idx-1</structfield> were modified and control > +<structfield>error_idx</structfield> is the one that caused the error. In > +the case of <constant>VIDIOC_S_EXT_CTRLS</constant> this might have left > +the hardware in an inconsistent state. These types of errors should not > +normally happen, and such errors are typically caused by problems in > +communicating with the hardware.</para> > + > +<para>Finally, note that the <structfield>error_idx</structfield> value is > +undefined if the ioctl returned 0 (success).</para></entry> > </row> > <row> > <entry>__u32</entry> -- Regards, Laurent Pinchart -- 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
