[RFC PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[RFC PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The type field in struct v4l2_selection is supposed to never use the
_MPLANE variants. E.g. if the driver supports V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
then userspace should still pass V4L2_BUF_TYPE_VIDEO_CAPTURE.

The reasons for this are lost in the mists of time, but it is really
annoying. In addition, the exynos drivers didn't follow this rule and
instead expected the _MPLANE type.

To fix that code is added to the v4l2 core that maps the _MPLANE buffer
types to their regular equivalents before calling the driver.

Effectively this allows for userspace to use either _MPLANE or the regular
buffer type. This keeps backwards compatibility while making things easier
for userspace.

Since drivers now never see the _MPLANE buffer types the exynos drivers
had to be adapted as well.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/platform/exynos-gsc/gsc-core.c     |  4 +-
 drivers/media/platform/exynos-gsc/gsc-m2m.c      |  8 ++--
 drivers/media/platform/exynos4-is/fimc-capture.c |  4 +-
 drivers/media/platform/exynos4-is/fimc-lite.c    |  4 +-
 drivers/media/v4l2-core/v4l2-ioctl.c             | 53 +++++++++++++++++++++---
 5 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 59a634201830..107faa04c947 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -569,9 +569,9 @@ int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop *cr)
 	}
 	pr_debug("user put w: %d, h: %d", cr->c.width, cr->c.height);
 
-	if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+	if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		f = &ctx->d_frame;
-	else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+	else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		f = &ctx->s_frame;
 	else
 		return -EINVAL;
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 82505025d96c..33611a46ce35 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -460,8 +460,8 @@ static int gsc_m2m_g_selection(struct file *file, void *fh,
 	struct gsc_frame *frame;
 	struct gsc_ctx *ctx = fh_to_ctx(fh);
 
-	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
-	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
 		return -EINVAL;
 
 	frame = ctx_get_frame(ctx, s->type);
@@ -503,8 +503,8 @@ static int gsc_m2m_s_selection(struct file *file, void *fh,
 	cr.type = s->type;
 	cr.c = s->r;
 
-	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
-	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
 		return -EINVAL;
 
 	ret = gsc_try_crop(ctx, &cr);
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 8a7cd07dbe28..d876fc3e0ef7 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1270,7 +1270,7 @@ static int fimc_cap_g_selection(struct file *file, void *fh,
 	struct fimc_ctx *ctx = fimc->vid_cap.ctx;
 	struct fimc_frame *f = &ctx->s_frame;
 
-	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
 	switch (s->target) {
@@ -1320,7 +1320,7 @@ static int fimc_cap_s_selection(struct file *file, void *fh,
 	struct fimc_frame *f;
 	unsigned long flags;
 
-	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
 	if (s->target == V4L2_SEL_TGT_COMPOSE)
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index b4c4a33784c4..7d3ec5cc6608 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -901,7 +901,7 @@ static int fimc_lite_g_selection(struct file *file, void *fh,
 	struct fimc_lite *fimc = video_drvdata(file);
 	struct flite_frame *f = &fimc->out_frame;
 
-	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
 	switch (sel->target) {
@@ -929,7 +929,7 @@ static int fimc_lite_s_selection(struct file *file, void *fh,
 	struct v4l2_rect rect = sel->r;
 	unsigned long flags;
 
-	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
+	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
 	    sel->target != V4L2_SEL_TGT_COMPOSE)
 		return -EINVAL;
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index e5a2187381db..fe2a677139df 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2141,6 +2141,47 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 					-EINVAL;
 }
 
+/*
+ * The selection API specified originally that the _MPLANE buffer types
+ * shouldn't be used. The reasons for this are lost in the mists of time
+ * (or just really crappy memories). Regardless, this is really annoying
+ * for userspace. So to keep things simple we map _MPLANE buffer types
+ * to their 'regular' counterparts before calling the driver. And we
+ * restore it afterwards. This way applications can use either buffer
+ * type and drivers don't need to check for both.
+ */
+static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
+			   struct file *file, void *fh, void *arg)
+{
+	struct v4l2_selection *p = arg;
+	u32 old_type = p->type;
+	int ret;
+
+	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	ret = ops->vidioc_g_selection(file, fh, p);
+	p->type = old_type;
+	return ret;
+}
+
+static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
+			   struct file *file, void *fh, void *arg)
+{
+	struct v4l2_selection *p = arg;
+	u32 old_type = p->type;
+	int ret;
+
+	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	ret = ops->vidioc_s_selection(file, fh, p);
+	p->type = old_type;
+	return ret;
+}
+
 static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -2160,7 +2201,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
 	else
 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
 
-	ret = ops->vidioc_g_selection(file, fh, &s);
+	ret = v4l_g_selection(ops, file, fh, &s);
 
 	/* copying results to old structure on success */
 	if (!ret)
@@ -2187,7 +2228,7 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
 	else
 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
 
-	return ops->vidioc_s_selection(file, fh, &s);
+	return v4l_s_selection(ops, file, fh, &s);
 }
 
 static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
@@ -2229,7 +2270,7 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 	else
 		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
 
-	ret = ops->vidioc_g_selection(file, fh, &s);
+	ret = v4l_g_selection(ops, file, fh, &s);
 	if (ret)
 		return ret;
 	p->bounds = s.r;
@@ -2240,7 +2281,7 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 	else
 		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
 
-	ret = ops->vidioc_g_selection(file, fh, &s);
+	ret = v4l_g_selection(ops, file, fh, &s);
 	if (ret)
 		return ret;
 	p->defrect = s.r;
@@ -2550,8 +2591,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
 	IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop, INFO_FL_CLEAR(v4l2_crop, type)),
 	IOCTL_INFO_FNC(VIDIOC_S_CROP, v4l_s_crop, v4l_print_crop, INFO_FL_PRIO),
-	IOCTL_INFO_STD(VIDIOC_G_SELECTION, vidioc_g_selection, v4l_print_selection, INFO_FL_CLEAR(v4l2_selection, r)),
-	IOCTL_INFO_STD(VIDIOC_S_SELECTION, vidioc_s_selection, v4l_print_selection, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_selection, r)),
+	IOCTL_INFO_FNC(VIDIOC_G_SELECTION, v4l_g_selection, v4l_print_selection, INFO_FL_CLEAR(v4l2_selection, r)),
+	IOCTL_INFO_FNC(VIDIOC_S_SELECTION, v4l_s_selection, v4l_print_selection, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_selection, r)),
 	IOCTL_INFO_STD(VIDIOC_G_JPEGCOMP, vidioc_g_jpegcomp, v4l_print_jpegcompression, 0),
 	IOCTL_INFO_STD(VIDIOC_S_JPEGCOMP, vidioc_s_jpegcomp, v4l_print_jpegcompression, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_QUERYSTD, v4l_querystd, v4l_print_std, 0),
-- 
2.11.0

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

[Hans Verkuil]

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

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 <hans.verkuil@cisco.com>
---
 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 <v4l2-rect-crop>`
       - ``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 <VIDIOC_G_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 <VIDIOC_G_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`
       - ``c``
       - Cropping rectangle. The same co-ordinate system as for struct
diff --git a/Documentation/media/uapi/v4l/vidioc-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-g-selection.rst
index deb1f6fb473b..8d4e7bf49eab 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-selection.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-selection.rst
@@ -40,13 +40,19 @@ Description
 
 The ioctls are used to query and configure selection rectangles.
 
+.. 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_selection` ``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.
+
 To query the cropping (composing) rectangle set struct
 :c:type:`v4l2_selection` ``type`` field to the
-respective buffer type. 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 next step is setting the
+respective buffer type. The next step is setting the
 value of struct :c:type:`v4l2_selection` ``target``
 field to ``V4L2_SEL_TGT_CROP`` (``V4L2_SEL_TGT_COMPOSE``). Please refer
 to table :ref:`v4l2-selections-common` or :ref:`selection-api` for
@@ -64,11 +70,7 @@ pixels.
 
 To change the cropping (composing) rectangle set the struct
 :c:type:`v4l2_selection` ``type`` field to the
-respective buffer type. Do not use multiplanar buffers. Use
-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``. Use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``. The next step is setting the
+respective buffer type. The next step is setting the
 value of struct :c:type:`v4l2_selection` ``target`` to
 ``V4L2_SEL_TGT_CROP`` (``V4L2_SEL_TGT_COMPOSE``). Please refer to table
 :ref:`v4l2-selections-common` or :ref:`selection-api` for additional
-- 
2.11.0

Re: [RFC PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[Sakari Ailus]

Hi Hans,

Cc Sylwester and Marek as well.

On Mon, May 08, 2017 at 04:35:05PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> The type field in struct v4l2_selection is supposed to never use the
> _MPLANE variants. E.g. if the driver supports V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> then userspace should still pass V4L2_BUF_TYPE_VIDEO_CAPTURE.
> 
> The reasons for this are lost in the mists of time, but it is really
> annoying. In addition, the exynos drivers didn't follow this rule and
> instead expected the _MPLANE type.
> 
> To fix that code is added to the v4l2 core that maps the _MPLANE buffer
> types to their regular equivalents before calling the driver.
> 
> Effectively this allows for userspace to use either _MPLANE or the regular
> buffer type. This keeps backwards compatibility while making things easier
> for userspace.
> 
> Since drivers now never see the _MPLANE buffer types the exynos drivers
> had to be adapted as well.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c     |  4 +-
>  drivers/media/platform/exynos-gsc/gsc-m2m.c      |  8 ++--
>  drivers/media/platform/exynos4-is/fimc-capture.c |  4 +-
>  drivers/media/platform/exynos4-is/fimc-lite.c    |  4 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c             | 53 +++++++++++++++++++++---
>  5 files changed, 57 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 59a634201830..107faa04c947 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -569,9 +569,9 @@ int gsc_try_crop(struct gsc_ctx *ctx, struct v4l2_crop *cr)
>  	}
>  	pr_debug("user put w: %d, h: %d", cr->c.width, cr->c.height);
>  
> -	if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +	if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		f = &ctx->d_frame;
> -	else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +	else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>  		f = &ctx->s_frame;
>  	else
>  		return -EINVAL;
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 82505025d96c..33611a46ce35 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -460,8 +460,8 @@ static int gsc_m2m_g_selection(struct file *file, void *fh,
>  	struct gsc_frame *frame;
>  	struct gsc_ctx *ctx = fh_to_ctx(fh);
>  
> -	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
> -	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>  		return -EINVAL;
>  
>  	frame = ctx_get_frame(ctx, s->type);
> @@ -503,8 +503,8 @@ static int gsc_m2m_s_selection(struct file *file, void *fh,
>  	cr.type = s->type;
>  	cr.c = s->r;
>  
> -	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
> -	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>  		return -EINVAL;
>  
>  	ret = gsc_try_crop(ctx, &cr);
> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
> index 8a7cd07dbe28..d876fc3e0ef7 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -1270,7 +1270,7 @@ static int fimc_cap_g_selection(struct file *file, void *fh,
>  	struct fimc_ctx *ctx = fimc->vid_cap.ctx;
>  	struct fimc_frame *f = &ctx->s_frame;
>  
> -	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
>  	switch (s->target) {
> @@ -1320,7 +1320,7 @@ static int fimc_cap_s_selection(struct file *file, void *fh,
>  	struct fimc_frame *f;
>  	unsigned long flags;
>  
> -	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
>  	if (s->target == V4L2_SEL_TGT_COMPOSE)
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> index b4c4a33784c4..7d3ec5cc6608 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -901,7 +901,7 @@ static int fimc_lite_g_selection(struct file *file, void *fh,
>  	struct fimc_lite *fimc = video_drvdata(file);
>  	struct flite_frame *f = &fimc->out_frame;
>  
> -	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
>  	switch (sel->target) {
> @@ -929,7 +929,7 @@ static int fimc_lite_s_selection(struct file *file, void *fh,
>  	struct v4l2_rect rect = sel->r;
>  	unsigned long flags;
>  
> -	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
> +	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>  	    sel->target != V4L2_SEL_TGT_COMPOSE)
>  		return -EINVAL;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index e5a2187381db..fe2a677139df 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2141,6 +2141,47 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					-EINVAL;
>  }
>  
> +/*
> + * The selection API specified originally that the _MPLANE buffer types
> + * shouldn't be used. The reasons for this are lost in the mists of time
> + * (or just really crappy memories). Regardless, this is really annoying
> + * for userspace. So to keep things simple we map _MPLANE buffer types
> + * to their 'regular' counterparts before calling the driver. And we
> + * restore it afterwards. This way applications can use either buffer
> + * type and drivers don't need to check for both.
> + */
> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
> +			   struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_selection *p = arg;
> +	u32 old_type = p->type;
> +	int ret;
> +
> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	ret = ops->vidioc_g_selection(file, fh, p);
> +	p->type = old_type;
> +	return ret;
> +}
> +
> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
> +			   struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_selection *p = arg;
> +	u32 old_type = p->type;
> +	int ret;
> +
> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	ret = ops->vidioc_s_selection(file, fh, p);

Can it be that ops->vidioc_s_selection() is NULL here? I don't think it's
checked anywhere. Same in v4l_g_selection().

With that fixed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +	p->type = old_type;
> +	return ret;
> +}
> +
>  static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -2160,7 +2201,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
>  	else
>  		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
>  
> -	ret = ops->vidioc_g_selection(file, fh, &s);
> +	ret = v4l_g_selection(ops, file, fh, &s);
>  
>  	/* copying results to old structure on success */
>  	if (!ret)
> @@ -2187,7 +2228,7 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
>  	else
>  		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
>  
> -	return ops->vidioc_s_selection(file, fh, &s);
> +	return v4l_s_selection(ops, file, fh, &s);
>  }
>  
>  static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
> @@ -2229,7 +2270,7 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>  	else
>  		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>  
> -	ret = ops->vidioc_g_selection(file, fh, &s);
> +	ret = v4l_g_selection(ops, file, fh, &s);
>  	if (ret)
>  		return ret;
>  	p->bounds = s.r;
> @@ -2240,7 +2281,7 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>  	else
>  		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>  
> -	ret = ops->vidioc_g_selection(file, fh, &s);
> +	ret = v4l_g_selection(ops, file, fh, &s);
>  	if (ret)
>  		return ret;
>  	p->defrect = s.r;
> @@ -2550,8 +2591,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
>  	IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop, INFO_FL_CLEAR(v4l2_crop, type)),
>  	IOCTL_INFO_FNC(VIDIOC_S_CROP, v4l_s_crop, v4l_print_crop, INFO_FL_PRIO),
> -	IOCTL_INFO_STD(VIDIOC_G_SELECTION, vidioc_g_selection, v4l_print_selection, INFO_FL_CLEAR(v4l2_selection, r)),
> -	IOCTL_INFO_STD(VIDIOC_S_SELECTION, vidioc_s_selection, v4l_print_selection, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_selection, r)),
> +	IOCTL_INFO_FNC(VIDIOC_G_SELECTION, v4l_g_selection, v4l_print_selection, INFO_FL_CLEAR(v4l2_selection, r)),
> +	IOCTL_INFO_FNC(VIDIOC_S_SELECTION, v4l_s_selection, v4l_print_selection, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_selection, r)),
>  	IOCTL_INFO_STD(VIDIOC_G_JPEGCOMP, vidioc_g_jpegcomp, v4l_print_jpegcompression, 0),
>  	IOCTL_INFO_STD(VIDIOC_S_JPEGCOMP, vidioc_s_jpegcomp, v4l_print_jpegcompression, INFO_FL_PRIO),
>  	IOCTL_INFO_FNC(VIDIOC_QUERYSTD, v4l_querystd, v4l_print_std, 0),

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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

[Sakari Ailus]

Hi Hans,

On Mon, May 08, 2017 at 04:35:06PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> 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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[Sylwester Nawrocki]

Hi,

On 06/16/2017 02:58 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Cc Sylwester and Marek as well.
> 
> On Mon, May 08, 2017 at 04:35:05PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> The type field in struct v4l2_selection is supposed to never use the
>> _MPLANE variants. E.g. if the driver supports V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
>> then userspace should still pass V4L2_BUF_TYPE_VIDEO_CAPTURE.
>>
>> The reasons for this are lost in the mists of time, but it is really
>> annoying. In addition, the exynos drivers didn't follow this rule and
>> instead expected the _MPLANE type.
>>
>> To fix that code is added to the v4l2 core that maps the _MPLANE buffer
>> types to their regular equivalents before calling the driver.
>>
>> Effectively this allows for userspace to use either _MPLANE or the regular
>> buffer type. This keeps backwards compatibility while making things easier
>> for userspace.
>>
>> Since drivers now never see the _MPLANE buffer types the exynos drivers
>> had to be adapted as well.
>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> ---
>>   drivers/media/platform/exynos-gsc/gsc-core.c     |  4 +-
>>   drivers/media/platform/exynos-gsc/gsc-m2m.c      |  8 ++--
>>   drivers/media/platform/exynos4-is/fimc-capture.c |  4 +-
>>   drivers/media/platform/exynos4-is/fimc-lite.c    |  4 +-
>>   drivers/media/v4l2-core/v4l2-ioctl.c             | 53 +++++++++++++++++++++---
>>   5 files changed, 57 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>> index 59a634201830..107faa04c947 100644
>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c

>> +/*
>> + * The selection API specified originally that the _MPLANE buffer types
>> + * shouldn't be used. The reasons for this are lost in the mists of time
>> + * (or just really crappy memories). Regardless, this is really annoying
>> + * for userspace. So to keep things simple we map _MPLANE buffer types
>> + * to their 'regular' counterparts before calling the driver. And we
>> + * restore it afterwards. This way applications can use either buffer
>> + * type and drivers don't need to check for both.

I agree this is annoying the _MPLANE buffer types are excluded this way. 
I suspect one of the main reasons was bias of the original author of the 
selection API to the multi-planar API. Now the API got messy because I didn't
adhere to this rule of buffer type change for VIDIOC_S_SELECTION, and the
code propagated to the other driver. Sorry about that.  

I checked GStreamer and AFAIU there is no change of buffer type for
S_SELECTION there:
https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c?id=3342d86d9b92cf60c419b728d10944968d77ecac#n3722

>> + */
>> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
>> +			   struct file *file, void *fh, void *arg)
>> +{
>> +	struct v4l2_selection *p = arg;
>> +	u32 old_type = p->type;
>> +	int ret;
>> +
>> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +	ret = ops->vidioc_g_selection(file, fh, p);
>> +	p->type = old_type;
>> +	return ret;
>> +}
>> +
>> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
>> +			   struct file *file, void *fh, void *arg)
>> +{
>> +	struct v4l2_selection *p = arg;
>> +	u32 old_type = p->type;
>> +	int ret;
>> +
>> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +	ret = ops->vidioc_s_selection(file, fh, p);
> 
> Can it be that ops->vidioc_s_selection() is NULL here? I don't think it's
> checked anywhere. Same in v4l_g_selection().

I think it can't be, there is the valid_ioctls bitmap test before a call back 
to the driver, to see if driver actually implements an ioctl. And the bitmap 
is populated beforehand in determine_valid_ioctls().

--
Regards,
Sylwester

Re: [RFC PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[Sakari Ailus]

Hi Sylwester,

On Sun, Jun 18, 2017 at 10:53:48PM +0200, Sylwester Nawrocki wrote:
> >> + */
> >> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
> >> +			   struct file *file, void *fh, void *arg)
> >> +{
> >> +	struct v4l2_selection *p = arg;
> >> +	u32 old_type = p->type;
> >> +	int ret;
> >> +
> >> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> >> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +	ret = ops->vidioc_g_selection(file, fh, p);
> >> +	p->type = old_type;
> >> +	return ret;
> >> +}
> >> +
> >> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
> >> +			   struct file *file, void *fh, void *arg)
> >> +{
> >> +	struct v4l2_selection *p = arg;
> >> +	u32 old_type = p->type;
> >> +	int ret;
> >> +
> >> +	if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> +		p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +	else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> >> +		p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> +	ret = ops->vidioc_s_selection(file, fh, p);
> > 
> > Can it be that ops->vidioc_s_selection() is NULL here? I don't think it's
> > checked anywhere. Same in v4l_g_selection().
> 
> I think it can't be, there is the valid_ioctls bitmap test before a call back 
> to the driver, to see if driver actually implements an ioctl. And the bitmap 
> is populated beforehand in determine_valid_ioctls().

Ack. Looks good to me then.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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

[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 <hans.verkuil@cisco.com>
>>
>> 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

[Hans Verkuil]

Hi Sylwester,

On 05/08/2017 04:35 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> 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 <hans.verkuil@cisco.com>
> ---
>   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 <v4l2-rect-crop>`
>         - ``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 <VIDIOC_G_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 <VIDIOC_G_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`
>         - ``c``
>         - Cropping rectangle. The same co-ordinate system as for struct
> diff --git a/Documentation/media/uapi/v4l/vidioc-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-g-selection.rst
> index deb1f6fb473b..8d4e7bf49eab 100644
> --- a/Documentation/media/uapi/v4l/vidioc-g-selection.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-g-selection.rst
> @@ -40,13 +40,19 @@ Description
>   
>   The ioctls are used to query and configure selection rectangles.
>   
> +.. 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_selection` ``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.
> +
>   To query the cropping (composing) rectangle set struct
>   :c:type:`v4l2_selection` ``type`` field to the
> -respective buffer type. 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 next step is setting the
> +respective buffer type. The next step is setting the
>   value of struct :c:type:`v4l2_selection` ``target``
>   field to ``V4L2_SEL_TGT_CROP`` (``V4L2_SEL_TGT_COMPOSE``). Please refer
>   to table :ref:`v4l2-selections-common` or :ref:`selection-api` for
> @@ -64,11 +70,7 @@ pixels.
>   
>   To change the cropping (composing) rectangle set the struct
>   :c:type:`v4l2_selection` ``type`` field to the
> -respective buffer type. Do not use multiplanar buffers. Use
> -``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
> -``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``. Use
> -``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
> -``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``. The next step is setting the
> +respective buffer type. The next step is setting the
>   value of struct :c:type:`v4l2_selection` ``target`` to
>   ``V4L2_SEL_TGT_CROP`` (``V4L2_SEL_TGT_COMPOSE``). Please refer to table
>   :ref:`v4l2-selections-common` or :ref:`selection-api` for additional
> 

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

[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