[REVIEW PATCHv1 0/2] DocBook fixes

[REVIEW PATCHv1 0/2] DocBook fixes

[Hans Verkuil]

Two patches: the first clarifies how the error_idx is used, the second
fixes a number of validation errors in the DocBook code.

Regards,

	Hans

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

[Hans Verkuil]

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(-)

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

[REVIEW PATCHv1 2/2] DocBook: fix various validation errors

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Fixed the following errors (with exception of the SVG errors):

  GEN     /home/hans/work/src/v4l/media-git/Documentation/DocBook//v4l2.xml
rm -rf Documentation/DocBook/index.html; echo '<h1>Linux Kernel HTML Documentation</h1>' >> Documentation/DocBook/index.html && echo '<h2>Kernel Version: 3.8.0-rc1</h2>' >> Documentation/DocBook/index.html && cat Documentation/DocBook/media_api.html >> Documentation/DocBook/index.html
/tmp/x.xml:883: element revremark: validity error : Element structname is not declared in revremark list of possible children
/tmp/x.xml:883: element revremark: validity error : Element xref is not declared in revremark list of possible children
/tmp/x.xml:1829: element footnote: validity error : Element footnote content does not follow the DTD, expecting (calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable)+, got (para para errorcode CDATA para )
/tmp/x.xml:9580: element xref: validity error : Element xref was declared EMPTY this one has content
/tmp/x.xml:13508: element link: validity error : Element link does not carry attribute linkend
/tmp/x.xml:13508: element link: validity error : No declaration for attribute linked of element link
/tmp/x.xml:16986: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
/tmp/x.xml:17003: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
/tmp/x.xml:17022: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
/tmp/x.xml:26795: element refsect1: validity error : Element refsect1 content does not follow the DTD, expecting (refsect1info? , (title , subtitle? , titleabbrev?) , (((calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | caution | important | note | tip | warning | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable | equation | example | figure | table | msgset | procedure | sidebar | qandaset | anchor | bridgehead | remark | highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , refsect2*) | refsect2+)), got (section )
/tmp/x.xml:26852: element refsect1: validity error : Element refsect1 content does not follow the DTD, expecting (refsect1info? , (title , subtitle? , titleabbrev?) , (((calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | caution | important | note | tip | warning | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable | equation | example | figure | table | msgset | procedure | sidebar | qandaset | anchor | bridgehead | remark | highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , refsect2*) | refsect2+)), got (table )

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/common.xml         |    2 +-
 Documentation/DocBook/media/v4l/io.xml             |    4 +--
 .../DocBook/media/v4l/pixfmt-srggb10alaw8.xml      |    2 +-
 Documentation/DocBook/media/v4l/v4l2.xml           |    5 +---
 Documentation/DocBook/media/v4l/vidioc-expbuf.xml  |   28 +++++++++-----------
 5 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/common.xml b/Documentation/DocBook/media/v4l/common.xml
index 73c6847..ae06afb 100644
--- a/Documentation/DocBook/media/v4l/common.xml
+++ b/Documentation/DocBook/media/v4l/common.xml
@@ -609,7 +609,7 @@ to zero and the <constant>VIDIOC_G_STD</constant>,
 	<para>Applications can make use of the <xref linkend="input-capabilities" /> and
 <xref linkend="output-capabilities"/> flags to determine whether the video standard ioctls
 are available for the device.</para>
-&ENOTTY;.
+
 	<para>See <xref linkend="buffer" /> for a rationale. Probably
 even USB cameras follow some well known video standard. It might have
 been better to explicitly indicate elsewhere if a device cannot live
diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 2c4646d..e6c5855 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -477,7 +477,7 @@ rest should be evident.</para>
 
     <note>
       <title>Experimental</title>
-      <para>This is an <link linkend="experimental"> experimental </link>
+      <para>This is an <link linkend="experimental">experimental</link>
       interface and may change in the future.</para>
     </note>
 
@@ -488,7 +488,7 @@ DMA buffer from userspace using a file descriptor previously exported for a
 different or the same device (known as the importer role), or both. This
 section describes the DMABUF importer role API in V4L2.</para>
 
-    <para>Refer to <link linked="vidioc-expbuf"> DMABUF exporting </link> for
+    <para>Refer to <link linkend="vidioc-expbuf">DMABUF exporting</link> for
 details about exporting V4L2 buffers as DMABUF file descriptors.</para>
 
 <para>Input and output devices support the streaming I/O method when the
diff --git a/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml b/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
index c934192..29acc20 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
@@ -29,6 +29,6 @@
 	    formats with 10 bits per color compressed to 8 bits each,
 	    using the A-LAW algorithm. Each color component consumes 8
 	    bits of memory. In other respects this format is similar to
-	    <xref linkend="V4L2-PIX-FMT-SRGGB8">.</xref></para>
+	    <xref linkend="V4L2-PIX-FMT-SRGGB8"></xref>.</para>
 	  </refsect1>
 	</refentry>
diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml
index 8fe2942..94ab0e1 100644
--- a/Documentation/DocBook/media/v4l/v4l2.xml
+++ b/Documentation/DocBook/media/v4l/v4l2.xml
@@ -143,10 +143,7 @@ applications. -->
 	<revnumber>3.9</revnumber>
 	<date>2012-12-03</date>
 	<authorinitials>sa</authorinitials>
-	<revremark>Added timestamp types to
-	<structname>v4l2_buffer</structname>, see <xref
-	linkend="buffer-flags" />.
-	</revremark>
+	<revremark>Added timestamp types to v4l2_buffer.</revremark>
       </revision>
 
       <revision>
diff --git a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
index 72dfbd2..e287c8f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
@@ -83,15 +83,14 @@ descriptor. The application may pass it to other DMABUF-aware devices. Refer to
 <link linkend="dmabuf">DMABUF importing</link> for details about importing
 DMABUF files into V4L2 nodes. It is recommended to close a DMABUF file when it
 is no longer used to allow the associated memory to be reclaimed. </para>
-
   </refsect1>
+
   <refsect1>
-   <section>
-      <title>Examples</title>
+    <title>Examples</title>
 
-      <example>
-	<title>Exporting a buffer.</title>
-	<programlisting>
+    <example>
+      <title>Exporting a buffer.</title>
+      <programlisting>
 int buffer_export(int v4lfd, &v4l2-buf-type; bt, int index, int *dmafd)
 {
 	&v4l2-exportbuffer; expbuf;
@@ -108,12 +107,12 @@ int buffer_export(int v4lfd, &v4l2-buf-type; bt, int index, int *dmafd)
 
 	return 0;
 }
-        </programlisting>
-      </example>
+      </programlisting>
+    </example>
 
-      <example>
-	<title>Exporting a buffer using the multi-planar API.</title>
-	<programlisting>
+    <example>
+      <title>Exporting a buffer using the multi-planar API.</title>
+      <programlisting>
 int buffer_export_mp(int v4lfd, &v4l2-buf-type; bt, int index,
 	int dmafd[], int n_planes)
 {
@@ -137,12 +136,9 @@ int buffer_export_mp(int v4lfd, &v4l2-buf-type; bt, int index,
 
 	return 0;
 }
-        </programlisting>
-      </example>
-   </section>
-  </refsect1>
+      </programlisting>
+    </example>
 
-  <refsect1>
     <table pgwide="1" frame="none" id="v4l2-exportbuffer">
       <title>struct <structname>v4l2_exportbuffer</structname></title>
       <tgroup cols="3">
-- 
1.7.10.4

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

[Laurent Pinchart]

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>
-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

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

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart