diff mbox

[REVIEW,PATCHv1,1/2] DocBook: improve the error_idx field documentation.

Message ID 50256813dbb6df25776aed847787d1eac9dbc9fa.1357560529.git.hans.verkuil@cisco.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hans Verkuil Jan. 7, 2013, 12:09 p.m. UTC
From: Hans Verkuil <hans.verkuil@cisco.com>

The documentation of the error_idx field was incomplete and confusing.
This patch improves it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |   51 +++++++++++++++++---
 1 file changed, 44 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart Jan. 7, 2013, 7:56 p.m. UTC | #1
Hi Hans,

Thanks for the patch.

On Monday 07 January 2013 13:09:47 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The documentation of the error_idx field was incomplete and confusing.
> This patch improves it.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  .../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>
Hans Verkuil Jan. 11, 2013, 11:48 a.m. UTC | #2
On Mon January 7 2013 20:56:07 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Monday 07 January 2013 13:09:47 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The documentation of the error_idx field was incomplete and confusing.
> > This patch improves it.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  .../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

I've rewritten the text completely using some of your ideas. I hope it is
now easier to understand.

> > +<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 ? ;-)

No, I don't want to change it. One option we have if it becomes clear in the
future that this information is really needed is to take one of the reserved
fields from struct v4l2_ext_controls and use that to report such information.

Another alternative is to improve debugging in v4l2-ctrls.c and, if debugging is
on, print which control failed the check for what reason.

> 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.

Well, if drivers don't know if a control is e.g. write-only until they try it,
then it can't be done during pre-validation anyway, so that's no problem.

The pre-validation should at minimum check whether ctrl_class is set up correctly,
whether all controls in the list actually exist, and check against READ_ONLY
or WRITE_ONLY (if known upfront).

The v4l2-compliance tool will test those minimum checks.

The control framework will also check whether the GRABBED flag is set for
a control and if the new value of a control is valid.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Jan. 11, 2013, 12:07 p.m. UTC | #3
Hi Hans,

On Friday 11 January 2013 12:48:19 Hans Verkuil wrote:
> On Mon January 7 2013 20:56:07 Laurent Pinchart wrote:
> > On Monday 07 January 2013 13:09:47 Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > The documentation of the error_idx field was incomplete and confusing.
> > > This patch improves it.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---

[snip]

> > 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.
>
> Well, if drivers don't know if a control is e.g. write-only until they try
> it, then it can't be done during pre-validation anyway, so that's no
> problem.

Sure, but my point is that we don't want to enforce in the spec that those 
checks must always be done during pre-validation, otherwise drivers that can't 
do it will violate the spec.

> The pre-validation should at minimum check whether ctrl_class is set up
> correctly, whether all controls in the list actually exist, and check
> against READ_ONLY or WRITE_ONLY (if known upfront).
> 
> The v4l2-compliance tool will test those minimum checks.
> 
> The control framework will also check whether the GRABBED flag is set for
> a control and if the new value of a control is valid.
diff mbox

Patch

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>
+
+<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>
+
+<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>