Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

2017-06-19 Thread Sylwester Nawrocki 

Hi Hans,

On 06/19/2017 09:35 AM, Hans Verkuil wrote:


diff --git a/Documentation/media/uapi/v4l/vidioc-cropcap.rst 
b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
index f21a69b554e1..d354216846e6 100644
--- a/Documentation/media/uapi/v4l/vidioc-cropcap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
@@ -39,16 +39,19 @@ structure. Drivers fill the rest of the structure. The 
results are
   constant except when switching the video standard. Remember this switch
   can occur implicit when switching the video input or output.
   
-Do not use the multiplanar buffer types. Use

-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
   This ioctl must be implemented for video capture or output devices that
   support cropping and/or scaling and/or have non-square pixels, and for
   overlay devices.
   
+.. note::

+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_cropcap` 
``type`` field
+   should be filled in. The Samsung Exynos drivers only accepted the


I propose I change this to "Some drivers only..." here and at the other places I
refer to Exynos.

Do you agree?


Yes, perhaps we could move the note paragraphs on the G_CROP,
G_SELECTION pages after v4l2_crop, v4l2_selection tables?

--
Regards,
Sylwester

Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

2017-06-19 Thread Hans Verkuil 

Hi Sylwester,

On 05/08/2017 04:35 PM, Hans Verkuil wrote:

From: Hans Verkuil 

Unfortunately the use of 'type' was inconsistent for multiplanar
buffer types. Starting with 4.12 both the normal and _MPLANE variants
are allowed, thus making it possible to write sensible code.

Yes, we messed up :-(

Signed-off-by: Hans Verkuil 
---
  Documentation/media/uapi/v4l/vidioc-cropcap.rst| 21 -
  Documentation/media/uapi/v4l/vidioc-g-crop.rst | 22 +-
  .../media/uapi/v4l/vidioc-g-selection.rst  | 22 --
  3 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-cropcap.rst 
b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
index f21a69b554e1..d354216846e6 100644
--- a/Documentation/media/uapi/v4l/vidioc-cropcap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
@@ -39,16 +39,19 @@ structure. Drivers fill the rest of the structure. The 
results are
  constant except when switching the video standard. Remember this switch
  can occur implicit when switching the video input or output.
  
-Do not use the multiplanar buffer types. Use

-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
  This ioctl must be implemented for video capture or output devices that
  support cropping and/or scaling and/or have non-square pixels, and for
  overlay devices.
  
+.. note::

+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_cropcap` 
``type`` field
+   should be filled in. The Samsung Exynos drivers only accepted the


I propose I change this to "Some drivers only..." here and at the other places I
refer to Exynos.

Do you agree?

Hans


+   ``_MPLANE`` buffer type while other drivers only accepted a non-multiplanar
+   buffer type (i.e. without the ``_MPLANE`` at the end).
+
+   Starting with kernel 4.12 both variations are allowed.
  
  .. c:type:: v4l2_cropcap
  
@@ -62,9 +65,9 @@ overlay devices.

  * - __u32
- ``type``
- Type of the data stream, set by the application. Only these types
-   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``,
-   ``V4L2_BUF_TYPE_VIDEO_OUTPUT`` and
-   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type`.
+   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``, 
``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``,
+   ``V4L2_BUF_TYPE_VIDEO_OUTPUT``, ``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE`` 
and
+   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type` and the 
note above.
  * - struct :ref:`v4l2_rect `
- ``bounds``
- Defines the window within capturing or output is possible, this
diff --git a/Documentation/media/uapi/v4l/vidioc-g-crop.rst 
b/Documentation/media/uapi/v4l/vidioc-g-crop.rst
index 56a36340f565..8aabe33c8da7 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-crop.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-crop.rst
@@ -45,12 +45,6 @@ and struct :c:type:`v4l2_rect` substructure named ``c`` of a
  v4l2_crop structure and call the :ref:`VIDIOC_S_CROP ` ioctl 
with a pointer
  to this structure.
  
-Do not use the multiplanar buffer types. Use

-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
  The driver first adjusts the requested dimensions against hardware
  limits, i. e. the bounds given by the capture/output window, and it
  rounds to the closest possible values of horizontal and vertical offset,
@@ -74,6 +68,16 @@ been negotiated.
  When cropping is not supported then no parameters are changed and
  :ref:`VIDIOC_S_CROP ` returns the ``EINVAL`` error code.
  
+.. note::

+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_crop` ``type`` 
field
+   should be filled in. The Samsung Exynos drivers only accepted the
+   ``_MPLANE`` buffer type while other drivers only accepted a non-multiplanar
+   buffer type (i.e. without the ``_MPLANE`` at the end).
+
+   Starting with kernel 4.12 both variations are allowed.
+
  
  .. c:type:: v4l2_crop
  
@@ -87,9 +91,9 @@ When cropping is not supported then no parameters are changed and

  * - __u32
- ``type``
- Type of the data stream, set by the application. Only these types
-   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``,
-   ``V4L2_BUF_TYPE_VIDEO_OUTPUT`` and
-   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type`.
+   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``,

Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

2017-06-18 Thread Sylwester Nawrocki 
On 06/16/2017 03:08 PM, Sakari Ailus wrote:
> On Mon, May 08, 2017 at 04:35:06PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> Unfortunately the use of 'type' was inconsistent for multiplanar
>> buffer types. Starting with 4.12 both the normal and _MPLANE variants
>> are allowed, thus making it possible to write sensible code.
>>
>> Yes, we messed up :-(
> 
> Things worse than this have happened. :-)
> 
> I don't think in general I would write about driver bugs that have already
> been fixed in developer documentation. That said, I'm not sure how otherwise
> developers would learn about this, but OTOH has it been reported to us as a
> bug?
> 
> Marek, Sylwester: any idea how widely the drivers in question are in use? If
> there's a real chance of hitting this, then it does make sense to mention it
> in the documentation.

I'm not sure how widely are used those drivers, I think we should just assume 
they are deployed and whatever we do should be backwards compatible. I don't 
think it is much helpful for Exynos to add notes like this in the 
documentation, 
so far I didn't receive any related bug report.

And even though now there is some confusion because drivers see "regular" 
buffer type while user space uses _MPLANE I like the $subject patch set
as it makes the API clearer from user perspective.

--
Regards,
Sylwester

Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

2017-06-16 Thread Sakari Ailus 
Hi Hans,

On Mon, May 08, 2017 at 04:35:06PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Unfortunately the use of 'type' was inconsistent for multiplanar
> buffer types. Starting with 4.12 both the normal and _MPLANE variants
> are allowed, thus making it possible to write sensible code.
> 
> Yes, we messed up :-(

Things worse than this have happened. :-)

I don't think in general I would write about driver bugs that have already
been fixed in developer documentation. That said, I'm not sure how otherwise
developers would learn about this, but OTOH has it been reported to us as a
bug?

Marek, Sylwester: any idea how widely the drivers in question are in use? If
there's a real chance of hitting this, then it does make sense to mention it
in the documentation.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

2017-05-08 Thread Hans Verkuil 
From: Hans Verkuil 

Unfortunately the use of 'type' was inconsistent for multiplanar
buffer types. Starting with 4.12 both the normal and _MPLANE variants
are allowed, thus making it possible to write sensible code.

Yes, we messed up :-(

Signed-off-by: Hans Verkuil 
---
 Documentation/media/uapi/v4l/vidioc-cropcap.rst| 21 -
 Documentation/media/uapi/v4l/vidioc-g-crop.rst | 22 +-
 .../media/uapi/v4l/vidioc-g-selection.rst  | 22 --
 3 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-cropcap.rst 
b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
index f21a69b554e1..d354216846e6 100644
--- a/Documentation/media/uapi/v4l/vidioc-cropcap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
@@ -39,16 +39,19 @@ structure. Drivers fill the rest of the structure. The 
results are
 constant except when switching the video standard. Remember this switch
 can occur implicit when switching the video input or output.
 
-Do not use the multiplanar buffer types. Use
-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
 This ioctl must be implemented for video capture or output devices that
 support cropping and/or scaling and/or have non-square pixels, and for
 overlay devices.
 
+.. note::
+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_cropcap` 
``type`` field
+   should be filled in. The Samsung Exynos drivers only accepted the
+   ``_MPLANE`` buffer type while other drivers only accepted a non-multiplanar
+   buffer type (i.e. without the ``_MPLANE`` at the end).
+
+   Starting with kernel 4.12 both variations are allowed.
 
 .. c:type:: v4l2_cropcap
 
@@ -62,9 +65,9 @@ overlay devices.
 * - __u32
   - ``type``
   - Type of the data stream, set by the application. Only these types
-   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``,
-   ``V4L2_BUF_TYPE_VIDEO_OUTPUT`` and
-   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type`.
+   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``, 
``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``,
+   ``V4L2_BUF_TYPE_VIDEO_OUTPUT``, ``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE`` 
and
+   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type` and the 
note above.
 * - struct :ref:`v4l2_rect `
   - ``bounds``
   - Defines the window within capturing or output is possible, this
diff --git a/Documentation/media/uapi/v4l/vidioc-g-crop.rst 
b/Documentation/media/uapi/v4l/vidioc-g-crop.rst
index 56a36340f565..8aabe33c8da7 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-crop.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-crop.rst
@@ -45,12 +45,6 @@ and struct :c:type:`v4l2_rect` substructure named ``c`` of a
 v4l2_crop structure and call the :ref:`VIDIOC_S_CROP ` ioctl 
with a pointer
 to this structure.
 
-Do not use the multiplanar buffer types. Use
-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
 The driver first adjusts the requested dimensions against hardware
 limits, i. e. the bounds given by the capture/output window, and it
 rounds to the closest possible values of horizontal and vertical offset,
@@ -74,6 +68,16 @@ been negotiated.
 When cropping is not supported then no parameters are changed and
 :ref:`VIDIOC_S_CROP ` returns the ``EINVAL`` error code.
 
+.. note::
+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_crop` ``type`` 
field
+   should be filled in. The Samsung Exynos drivers only accepted the
+   ``_MPLANE`` buffer type while other drivers only accepted a non-multiplanar
+   buffer type (i.e. without the ``_MPLANE`` at the end).
+
+   Starting with kernel 4.12 both variations are allowed.
+
 
 .. c:type:: v4l2_crop
 
@@ -87,9 +91,9 @@ When cropping is not supported then no parameters are changed 
and
 * - __u32
   - ``type``
   - Type of the data stream, set by the application. Only these types
-   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``,
-   ``V4L2_BUF_TYPE_VIDEO_OUTPUT`` and
-   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type`.
+   are valid here: ``V4L2_BUF_TYPE_VIDEO_CAPTURE``, 
``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``,
+   ``V4L2_BUF_TYPE_VIDEO_OUTPUT``, ``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE`` 
and
+   ``V4L2_BUF_TYPE_VIDEO_OVERLAY``. See :c:type:`v4l2_buf_type` and the 
note above.
 * - struct :c:type:`v4l2_rect`