[PATCH v5 0/7] v4l2: add support for colorspace conversion API (CSC) for video capture and subdevices

[PATCH v5 0/7] v4l2: add support for colorspace conversion API (CSC) for video capture and subdevices

[Dafna Hirschfeld]

For video capture it is the driver that reports the colorspace,
Y'CbCr/HSV encoding, quantization range and transfer function
used by the video, and there is no way to request something
different, even though many HDTV receivers have some sort of
colorspace conversion capabilities.

For output video this feature already exists since the application
specifies this information for the video format it will send out, and
the transmitter will enable any available CSC if a format conversion has
to be performed in order to match the capabilities of the sink.

For video capture we propose adding new v4l2_pix_format flag:
V4L2_PIX_FMT_FLAG_SET_CSC. The flag is set by the application,
the driver will interpret the fields 'colorspace', 'ycbcr_enc/hsv_enc',
'quantization' and 'xfer_func' as the requested colorspace information
and will attempt to do the conversion it supports.

Drivers set the flags
V4L2_FMT_FLAG_CSC_COLORSPACE,
V4L2_FMT_FLAG_CSC_YCBCR_ENC,
V4L2_FMT_FLAG_CSC_HSV_ENC,
V4L2_FMT_FLAG_CSC_QUANTIZATION,
V4L2_FMT_FLAG_CSC_XFER_FUNC,
in the flags field of the struct v4l2_fmtdesc during enumeration to
indicate that they support colorspace conversion for the respective field.

The same API is added for the subdevices. With the flag
V4L2_MBUS_FRAMEFMT_SET_CSC set by the application in VIDIOC_SUBDEV_S_FMT
ioctl and the flags
V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE,
V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION,
V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC
set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.

For subdevices, new 'flags' fields were added to the structs
v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed from the
'reserved' field. In the struct 'v4l2_mbus_framefmt' and reserved2 field
is added before the flags due to padding.

Drivers do not have to actually look at the flags. If the flags are not
set, then the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func
fields are set to the default values by the core, i.e. just pass on the
received format without conversion.

The patchset includes a patch in vivid that implements the API for video capture devices
and a patch in rkisp1 driver that implement the API for subdevices.

In rkisp1, the API is used to allow userspace to set the quantizaion for
YUV formats on the source pad of the isp entity. The quantization is read-only
on the other entities and pads and do not habe to propagate.

I also removed the TODO item 'Handle quantization'
and added the TODO item 'Add uapi docs. Remeber to add documentation of how quantization is handled.'

The patchset is rebased on top of v4 of the patchset
"media: staging: rkisp1: bugs fixes and vars renames"
https://patchwork.kernel.org/cover/11611867/

A patchset for v4l-utils to support the API will also be sent

Patches Summary:
----------------
patch 1 - moves the description of the v4l2_pix_format flags in the Documentation to the right
place where it should be. This is just a fix.

patch 2 - Adds the API for video devices - Adds the new macros and fields and their documentation.
This is only for video devices.

patch 3 - uses the API in the capture device of vivid. Using the API, userspace is allowed to:
1. Set the ycbcr_enc function for YUV formats.
2. Set the hsv_enc function for HSV formats
3. Set the quantization for YUV and RGB formats.
4. Set the colorspace
5. set the xfer_func

patch 4 - add the API for subdevices - Adds the new macros and fields and their documentation.

patch 5 - replace the ycbcr_enc field of 'struct v4l2_mbus_framefmt' with a union that includes
	  also hsv_enc and add support for CSC on hsv_enc for subdevices.

patch 6 - use the API to allow userspace to set the quantizaion on the isp source pad.

patch 7 - sets the flags to 0 in the mbus enumeration of the resizer.

Changes from v4:
----------------
- This time I didn't to this patchset the patches from 'media: staging: rkisp1: bugs fixes and vars renames'
but note that this patchset is rebased on that patchset.

- I added the fields 'colorspace' 'xfer_func' to the list of fields that the CSC API supports due to
comment from Hans Verkuil. Also the vivid implementation is extended to support those fields.

- I split the patch that adds the API into two patches. One for video devices and one patch for subdevices.
This is because the two APIs are actually independent and it is easier to maintain and review two small patches
than one big patch.
Also, it can be decided that only one of those patches be accpeted.

- I changed the flags fields in 'struct v4l2_mbus_framefmt'  to __u32 and added a reserved2 field before it due to
padding.

- I changed the patch for rkisp1 so that the quantizaion can be set by userspace only on the source pad of
the isp entity and is read-only and set always to default for all other entities and pads.
The driver also does not validate matching of the quantizaion between entities.

- Added a patch to the rkisp1-resizer that sets the flags to 0 when enumerating the mbus codes.
This is because the resizer reports the same formats as the source pad of the isp entity but does
not support the quantizaion conversion.

- various small fixes due to feedback.

Changes from v3:
----------------
- patches 1,2,3,7,8 are new (rkisp1 bug fixes, move v4l2-pixfmt flags docs, vivid patch, support for HSV media bus)

- patch 4 (The API):
1) reformulate the Documentation according to comments from Verkuil and Tomasz Figa
2) remove the code in drivers/media/v4l2-core/v4l2-subdev.c and drivers/media/v4l2-core/v4l2-ioctl.c
so that except of the new macros and fields no new code is added to the core

- patches 5,6 - refactor the rkisp1 implementation so that the userspace should set the quantization separately
to each entity. In patch 6 the validation callbacks make sure the quantizations matche.


Dafna Hirschfeld (7):
  media: Documentation: v4l: move table of v4l2_pix_format(_mplane)
    flags to pixfmt-v4l2.rst
  v4l2: add support for colorspace conversion API (CSC) for video
    capture
  media: vivid: Add support to the CSC API
  v4l2: extend the CSC API to subdevice.
  media: v4l2: add support for the subdev CSC API for hsv_enc on
    mediabus
  media: staging: rkisp1: allow quantization setting by userspace on the
    isp source pad
  media: staging: rkisp1: rsz: set flags to 0 in enum_mbus_code cb

 .../media/v4l/pixfmt-reserved.rst             | 17 ----
 .../media/v4l/pixfmt-v4l2-mplane.rst          | 16 +--
 .../userspace-api/media/v4l/pixfmt-v4l2.rst   | 81 +++++++++++++++-
 .../media/v4l/subdev-formats.rst              | 97 +++++++++++++++++--
 .../media/v4l/vidioc-enum-fmt.rst             | 35 +++++++
 .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++-
 .../media/videodev2.h.rst.exceptions          |  7 +-
 .../media/test-drivers/vivid/vivid-vid-cap.c  | 74 ++++++++++++--
 .../test-drivers/vivid/vivid-vid-common.c     | 24 +++++
 drivers/staging/media/rkisp1/TODO             |  2 +-
 drivers/staging/media/rkisp1/rkisp1-capture.c | 10 --
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 +++-
 drivers/staging/media/rkisp1/rkisp1-resizer.c |  1 +
 include/uapi/linux/v4l2-mediabus.h            | 17 +++-
 include/uapi/linux/v4l2-subdev.h              |  9 +-
 include/uapi/linux/videodev2.h                |  6 ++
 16 files changed, 391 insertions(+), 67 deletions(-)

-- 
2.17.1

[PATCH v5 1/7] media: Documentation: v4l: move table of v4l2_pix_format(_mplane) flags to pixfmt-v4l2.rst

[Dafna Hirschfeld]

The table of the flags of the structs
v4l2_pix_format(_mplane) is currently in pixfmt-reserved.rst
which is wrong, it should be in pixfmt-v4l2.rst

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../userspace-api/media/v4l/pixfmt-reserved.rst | 17 -----------------
 .../userspace-api/media/v4l/pixfmt-v4l2.rst     | 17 +++++++++++++++++
 .../media/videodev2.h.rst.exceptions            |  2 +-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst b/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
index 59b9e7238f90..74ab6b5ce294 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
@@ -263,20 +263,3 @@ please make a proposal on the linux-media mailing list.
 	of tiles, resulting in 32-aligned resolutions for the luminance plane
 	and 16-aligned resolutions for the chrominance plane (with 2x2
 	subsampling).
-
-.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
-
-.. _format-flags:
-
-.. flat-table:: Format Flags
-    :header-rows:  0
-    :stub-columns: 0
-    :widths:       3 1 4
-
-    * - ``V4L2_PIX_FMT_FLAG_PREMUL_ALPHA``
-      - 0x00000001
-      - The color values are premultiplied by the alpha channel value. For
-	example, if a light blue pixel with 50% transparency was described
-	by RGBA values (128, 192, 255, 128), the same pixel described with
-	premultiplied colors would be described by RGBA values (64, 96,
-	128, 128)
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
index e0ee2823ab1f..56a2952de873 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
@@ -174,3 +174,20 @@ Single-planar format structure
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
+
+.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
+
+.. _format-flags:
+
+.. flat-table:: Format Flags
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - ``V4L2_PIX_FMT_FLAG_PREMUL_ALPHA``
+      - 0x00000001
+      - The color values are premultiplied by the alpha channel value. For
+        example, if a light blue pixel with 50% transparency was described
+	by RGBA values (128, 192, 255, 128), the same pixel described with
+	premultiplied colors would be described by RGBA values (64, 96,
+	128, 128)
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index ca05e4e126b2..659799cc1eca 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -180,7 +180,7 @@ replace define V4L2_CAP_IO_MC device-capabilities
 
 # V4L2 pix flags
 replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
-replace define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA reserved-formats
+replace define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA format-flags
 
 # V4L2 format flags
 replace define V4L2_FMT_FLAG_COMPRESSED fmtdesc-flags
-- 
2.17.1

[PATCH v5 2/7] v4l2: add support for colorspace conversion API (CSC) for video capture

[Dafna Hirschfeld]

For video capture it is the driver that reports the colorspace,
Y'CbCr/HSV encoding, quantization range and transfer function
used by the video, and there is no way to request something
different, even though many HDTV receivers have some sort of
colorspace conversion capabilities.

For output video this feature already exists since the application
specifies this information for the video format it will send out, and
the transmitter will enable any available CSC if a format conversion has
to be performed in order to match the capabilities of the sink.

For video capture we propose adding new v4l2_pix_format flag:
V4L2_PIX_FMT_FLAG_SET_CSC. The flag is set by the application,
the driver will interpret the colorspace, ycbcr_enc/hsv_enc, quantization
and xfer_func fields as the requested colorspace information and will
attempt to do the conversion it supports.

Drivers set the flags
V4L2_FMT_FLAG_CSC_COLORSPACE,
V4L2_FMT_FLAG_CSC_YCBCR_ENC,
V4L2_FMT_FLAG_CSC_HSV_ENC,
V4L2_FMT_FLAG_CSC_QUANTIZATION,
V4L2_FMT_FLAG_CSC_XFER_FUNC,
in the flags field of the struct v4l2_fmtdesc during enumeration to
indicate that they support colorspace conversion for the respective field.

Drivers do not have to actually look at the flags. If the flags are not
set, then the fields 'colorspace', 'ycbcr_enc/hsv_enc', 'quantization'
and 'xfer_func' are set to the default values by the core, i.e. just
pass on the received format without conversion.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../media/v4l/pixfmt-v4l2-mplane.rst          | 16 ++---
 .../userspace-api/media/v4l/pixfmt-v4l2.rst   | 64 +++++++++++++++++--
 .../media/v4l/vidioc-enum-fmt.rst             | 35 ++++++++++
 .../media/videodev2.h.rst.exceptions          |  5 ++
 include/uapi/linux/videodev2.h                |  6 ++
 5 files changed, 109 insertions(+), 17 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst b/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
index 444b4082684c..66f3365d7b72 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
@@ -105,29 +105,21 @@ describing all planes of that format.
     * - __u8
       - ``ycbcr_enc``
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``hsv_enc``
       - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - }
       -
     * - __u8
       - ``quantization``
       - Quantization range, from enum :c:type:`v4l2_quantization`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``xfer_func``
       - Transfer function, from enum :c:type:`v4l2_xfer_func`.
-        This information supplements the ``colorspace`` and must be set by
-	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	See struct :c:type:`v4l2_pix_format`.
     * - __u8
       - ``reserved[7]``
       - Reserved for future extensions. Should be zeroed by drivers and
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
index 56a2952de873..d9f8f7eb7098 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
@@ -116,7 +116,14 @@ Single-planar format structure
       - Image colorspace, from enum :c:type:`v4l2_colorspace`.
         This information supplements the ``pixelformat`` and must be set
 	by the driver for capture streams and by the application for
-	output streams, see :ref:`colorspaces`.
+	output streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific colorspace
+	for the captured image data. If the driver cannot handle requested
+	conversion, it will return another supported colorspace.
+	The driver indicates that colorspace conversion is supported by setting
+	the flag V4L2_FMT_FLAG_CSC_COLORSPACE in the corresponding struct
+	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
     * - __u32
       - ``priv``
       - This field indicates whether the remaining fields of the
@@ -153,13 +160,29 @@ Single-planar format structure
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific Y'CbCr encoding
+	for the captured image data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	This field is ignored for HSV pixelformats. The driver indicates that
+	ycbcr_enc conversion is supported by setting the flag
+	V4L2_FMT_FLAG_CSC_YCBCR_ENC in the corresponding struct
+	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
     * - __u32
       - ``hsv_enc``
       - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set this
+	field for a capture stream to request a specific HSV encoding for the
+	captured image data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	This field is ignored for non-HSV pixelformats. The driver indicates
+	that hsv_enc conversion is supported by setting the flag
+	V4L2_FMT_FLAG_CSC_HSV_ENC in the corresponding struct
+	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
     * - }
       -
     * - __u32
@@ -167,13 +190,27 @@ Single-planar format structure
       - Quantization range, from enum :c:type:`v4l2_quantization`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific quantization
+	range for the captured image data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	The driver indicates that quantization conversion is supported by setting
+	the flag V4L2_FMT_FLAG_CSC_QUANTIZATION in the corresponding struct
+	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
     * - __u32
       - ``xfer_func``
       - Transfer function, from enum :c:type:`v4l2_xfer_func`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the flag
+	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific Transfer function
+	for the captured image data. If the driver cannot handle requested
+	conversion, it will return another supported Transfer function.
+	The driver indicates that xfer_func conversion is supported by setting
+	the flag V4L2_FMT_FLAG_CSC_XFER_FUNC in the corresponding struct
+	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
 
 .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
 
@@ -191,3 +228,20 @@ Single-planar format structure
 	by RGBA values (128, 192, 255, 128), the same pixel described with
 	premultiplied colors would be described by RGBA values (64, 96,
 	128, 128)
+    * .. _`v4l2-pix-fmt-flag-set-csc`:
+
+      - ``V4L2_PIX_FMT_FLAG_SET_CSC``
+      - 0x00000002
+      - Set by the application. It is only used for capture and is
+        ignored for output streams. If set, then request the device to do
+	colorspace conversion from the received colorspace to the requested
+	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
+	``hsv_enc``, ``quantization`` or ``xfer_func``) is set to 0, then that
+	colorimetry setting will remain unchanged from what was received. So to
+	change the quantization only the ``quantization`` field shall be set to
+	non-zero values (``V4L2_QUANTIZATION_FULL_RANGE`` or
+	``V4L2_QUANTIZATION_LIM_RANGE``) and all other colorimetry fields shall
+	be set to 0.
+
+	To check which conversions are supported by the hardware for the current
+	pixel format, see :ref:`fmtdesc-flags`.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
index 05835e04c20b..98595dd48557 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
@@ -198,6 +198,41 @@ the ``mbus_code`` field is handled differently:
 	This flag can only be used in combination with the
 	``V4L2_FMT_FLAG_COMPRESSED`` flag, since this applies to
         compressed formats only. This flag is valid for stateful encoders only.
+    * - ``V4L2_FMT_FLAG_CSC_COLORSPACE``
+      - 0x0020
+      - The driver allows the application to try to change the default
+	colorspace. This flag is relevant only for capture devices.
+	The application can ask to configure the colorspace of the capture device
+	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
+	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
+    * - ``V4L2_FMT_FLAG_CSC_YCBCR_ENC``
+      - 0x0040
+      - The driver allows the application to try to change the default
+	Y'CbCr encoding. This flag is relevant only for capture devices.
+	The application can ask to configure the ycbcr_enc of the capture device
+	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
+	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
+    * - ``V4L2_FMT_FLAG_CSC_HSV_ENC``
+      - 0x0040
+      - The driver allows the application to try to change the default
+	HSV encoding. This flag is relevant only for capture devices.
+	The application can ask to configure the hsv_enc of the capture device
+	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
+	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
+    * - ``V4L2_FMT_FLAG_CSC_QUANTIZATION``
+      - 0x0080
+      - The driver allows the application to try to change the default
+	quantization. This flag is relevant only for capture devices.
+	The application can ask to configure the quantization of the capture
+	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
+	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
+    * - ``V4L2_FMT_FLAG_CSC_XFER_FUNC``
+      - 0x0100
+      - The driver allows the application to try to change the default
+	quantization. This flag is relevant only for capture devices.
+	The application can ask to configure the quantization of the capture
+	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
+	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
 
 
 Return Value
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 659799cc1eca..faa19e7d241b 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -188,6 +188,11 @@ replace define V4L2_FMT_FLAG_EMULATED fmtdesc-flags
 replace define V4L2_FMT_FLAG_CONTINUOUS_BYTESTREAM fmtdesc-flags
 replace define V4L2_FMT_FLAG_DYN_RESOLUTION fmtdesc-flags
 replace define V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL fmtdesc-flags
+replace define V4L2_FMT_FLAG_CSC_COLORSPACE fmtdesc-flags
+replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
+replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
+replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
+replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags
 
 # V4L2 timecode types
 replace define V4L2_TC_TYPE_24FPS timecode-type
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 303805438814..76610509d670 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -776,6 +776,7 @@ struct v4l2_pix_format {
 
 /* Flags */
 #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
+#define V4L2_PIX_FMT_FLAG_SET_CSC	0x00000002
 
 /*
  *	F O R M A T   E N U M E R A T I O N
@@ -795,6 +796,11 @@ struct v4l2_fmtdesc {
 #define V4L2_FMT_FLAG_CONTINUOUS_BYTESTREAM	0x0004
 #define V4L2_FMT_FLAG_DYN_RESOLUTION		0x0008
 #define V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL	0x0010
+#define V4L2_FMT_FLAG_CSC_COLORSPACE		0x0020
+#define V4L2_FMT_FLAG_CSC_YCBCR_ENC		0x0040
+#define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
+#define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0080
+#define V4L2_FMT_FLAG_CSC_XFER_FUNC		0x0100
 
 	/* Frame Size and frame rate enumeration */
 /*
-- 
2.17.1

[PATCH v5 3/7] media: vivid: Add support to the CSC API

[Dafna Hirschfeld]

The CSC API (Colorspace conversion) allows userspace to try
to configure the ycbcr/hsv_enc function and the quantization
for capture devices. This patch adds support to the CSC API
in vivid.
Using the CSC API, userspace is allowed to do the following:

- Set the ycbcr_enc function for YUV formats.
- Set the hsv_enc function for HSV formats
- Set the quantization for YUV and RGB formats.
- Set the xfer_func.
- Set the colorspace.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../media/test-drivers/vivid/vivid-vid-cap.c  | 74 +++++++++++++++++--
 .../test-drivers/vivid/vivid-vid-common.c     | 24 ++++++
 2 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
index e94beef008c8..8916def70ffe 100644
--- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
@@ -549,6 +549,45 @@ int vivid_g_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
+static bool vivid_is_colorspace_valid(__u32 colorspace)
+{
+	if (colorspace > V4L2_COLORSPACE_DEFAULT &&
+	    colorspace <= V4L2_COLORSPACE_DCI_P3)
+		return true;
+	return false;
+}
+
+static bool vivid_is_hsv_enc_valid(__u8 hsv_enc)
+{
+	if (hsv_enc == V4L2_HSV_ENC_180 || hsv_enc == V4L2_HSV_ENC_256)
+		return true;
+	return false;
+}
+
+static bool vivid_is_ycbcr_enc_valid(__u8 ycbcr_enc)
+{
+	/* V4L2_YCBCR_ENC_SMPTE240M is the last ycbcr_enc enum */
+	if (ycbcr_enc && ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M)
+		return true;
+	return false;
+}
+
+static bool vivid_is_quant_valid(__u8 quantization)
+{
+	if (quantization == V4L2_QUANTIZATION_FULL_RANGE ||
+	    quantization == V4L2_QUANTIZATION_LIM_RANGE)
+		return true;
+	return false;
+}
+
+static bool vivid_is_xfer_func_valid(__u32 xfer_func)
+{
+	if (xfer_func > V4L2_XFER_FUNC_DEFAULT &&
+	    xfer_func <= V4L2_XFER_FUNC_SMPTE2084)
+		return true;
+	return false;
+}
+
 int vivid_try_fmt_vid_cap(struct file *file, void *priv,
 			struct v4l2_format *f)
 {
@@ -560,6 +599,7 @@ int vivid_try_fmt_vid_cap(struct file *file, void *priv,
 	unsigned factor = 1;
 	unsigned w, h;
 	unsigned p;
+	bool user_set_csc = !!(mp->flags & V4L2_PIX_FMT_FLAG_SET_CSC);
 
 	fmt = vivid_get_format(dev, mp->pixelformat);
 	if (!fmt) {
@@ -633,13 +673,26 @@ int vivid_try_fmt_vid_cap(struct file *file, void *priv,
 			(fmt->bit_depth[p] / fmt->vdownsampling[p])) /
 			(fmt->bit_depth[0] / fmt->vdownsampling[0]);
 
-	mp->colorspace = vivid_colorspace_cap(dev);
-	if (fmt->color_enc == TGP_COLOR_ENC_HSV)
-		mp->hsv_enc = vivid_hsv_enc_cap(dev);
-	else
+	if (!user_set_csc || !vivid_is_colorspace_valid(mp->colorspace))
+		mp->colorspace = vivid_colorspace_cap(dev);
+	if (fmt->color_enc == TGP_COLOR_ENC_HSV) {
+		if (!user_set_csc || !vivid_is_hsv_enc_valid(mp->hsv_enc))
+			mp->hsv_enc = vivid_hsv_enc_cap(dev);
+	} else if (fmt->color_enc == TGP_COLOR_ENC_YCBCR) {
+		if (!user_set_csc || !vivid_is_ycbcr_enc_valid(mp->ycbcr_enc))
+			mp->ycbcr_enc = vivid_ycbcr_enc_cap(dev);
+	} else {
 		mp->ycbcr_enc = vivid_ycbcr_enc_cap(dev);
-	mp->xfer_func = vivid_xfer_func_cap(dev);
-	mp->quantization = vivid_quantization_cap(dev);
+	}
+	if (!user_set_csc || !vivid_is_xfer_func_valid(mp->xfer_func))
+		mp->xfer_func = vivid_xfer_func_cap(dev);
+	if (fmt->color_enc == TGP_COLOR_ENC_YCBCR ||
+	    fmt->color_enc == TGP_COLOR_ENC_RGB) {
+		if (!user_set_csc || !vivid_is_quant_valid(mp->quantization))
+			mp->quantization = vivid_quantization_cap(dev);
+	} else {
+		mp->quantization = vivid_quantization_cap(dev);
+	}
 	memset(mp->reserved, 0, sizeof(mp->reserved));
 	return 0;
 }
@@ -769,6 +822,15 @@ int vivid_s_fmt_vid_cap(struct file *file, void *priv,
 	if (vivid_is_sdtv_cap(dev))
 		dev->tv_field_cap = mp->field;
 	tpg_update_mv_step(&dev->tpg);
+	dev->tpg.colorspace = mp->colorspace;
+	dev->tpg.quantization = mp->quantization;
+	if (dev->fmt_cap->color_enc == TGP_COLOR_ENC_YCBCR)
+		dev->tpg.ycbcr_enc = mp->ycbcr_enc;
+	else
+		dev->tpg.hsv_enc = mp->hsv_enc;
+	dev->tpg.quantization = mp->quantization;
+	dev->tpg.xfer_func = mp->xfer_func;
+
 	return 0;
 }
 
diff --git a/drivers/media/test-drivers/vivid/vivid-vid-common.c b/drivers/media/test-drivers/vivid/vivid-vid-common.c
index 76b0be670ebb..5cab9b2a74bd 100644
--- a/drivers/media/test-drivers/vivid/vivid-vid-common.c
+++ b/drivers/media/test-drivers/vivid/vivid-vid-common.c
@@ -920,6 +920,30 @@ int vivid_enum_fmt_vid(struct file *file, void  *priv,
 	fmt = &vivid_formats[f->index];
 
 	f->pixelformat = fmt->fourcc;
+
+	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+	    f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+		return 0;
+	/*
+	 * For capture devices, we support the CSC API.
+	 * We allow userspace to:
+	 * 1. set the ycbcr_enc on yuv format
+	 * 2. set the hsv_enc on hsv format
+	 * 3. set the quantization on yuv and rgb formats
+	 * 4. set the colorspace
+	 * 5. set the xfer_func
+	 */
+	if (fmt->color_enc == TGP_COLOR_ENC_YCBCR) {
+		f->flags |= V4L2_FMT_FLAG_CSC_YCBCR_ENC;
+		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION;
+	} else if (fmt->color_enc == TGP_COLOR_ENC_HSV) {
+		f->flags |= V4L2_FMT_FLAG_CSC_HSV_ENC;
+	} else if (fmt->color_enc == TGP_COLOR_ENC_RGB) {
+		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION;
+	}
+	f->flags |= V4L2_FMT_FLAG_CSC_COLORSPACE;
+	f->flags |= V4L2_FMT_FLAG_CSC_XFER_FUNC;
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Dafna Hirschfeld]

This patch extends the CSC API in video devices to be supported
also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
The flags:

V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,

are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.

New 'flags' fields were added to the structs
v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
from the 'reserved' field

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
 .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
 include/uapi/linux/v4l2-mediabus.h            |  9 ++-
 include/uapi/linux/v4l2-subdev.h              |  8 +-
 4 files changed, 129 insertions(+), 10 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
index 9a4d61b0d76f..7362ee0b1e96 100644
--- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
+++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
@@ -41,32 +41,96 @@ Media Bus Formats
 	:ref:`field-order` for details.
     * - __u32
       - ``colorspace``
-      - Image colorspace, from enum
-	:c:type:`v4l2_colorspace`. See
-	:ref:`colorspaces` for details.
+      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
+        Must be set by the driver for capture streams and by the application
+	for output streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific colorspace
+	for the media bus data. If the driver cannot handle requested
+	conversion, it will return another supported colorspace.
+	The driver indicates that colorspace conversion is supported by setting
+	the flag V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE in the corresponding struct
+	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
+	See :ref:`colorspaces`.
     * - __u16
       - ``ycbcr_enc``
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific Y'CbCr encoding
+	for the media bus data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	This field is ignored for HSV media bus formats. The driver indicates
+	that ycbcr_enc conversion is supported by setting the flag
+	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC in the corresponding struct
+	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
+	See :ref:`v4l2-subdev-mbus-code-flags`.
+
     * - __u16
       - ``quantization``
       - Quantization range, from enum :c:type:`v4l2_quantization`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific quantization
+	encoding for the media bus data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	The driver indicates that quantization conversion is supported by
+	setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION in the
+	corresponding struct :c:type:`v4l2_subdev_mbus_code_enum`
+	during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`.
+
     * - __u16
       - ``xfer_func``
       - Transfer function, from enum :c:type:`v4l2_xfer_func`.
         This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-	streams, see :ref:`colorspaces`.
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific transfer
+	function for the media bus data. If the driver cannot handle the requested
+	conversion, it will return another supported transfer function.
+	The driver indicates that the transfer function conversion is supported by
+	setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC in the
+	corresponding struct :c:type:`v4l2_subdev_mbus_code_enum`
+	during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`.
+    * - __u16
+      - ``reserved2``
+      - Reserved for future extensions.
+    * - __u32
+      - ``flags``
+      - flags See:  :ref:v4l2-mbus-framefmt-flags
     * - __u16
-      - ``reserved``\ [11]
+      - ``reserved``\ [8]
       - Reserved for future extensions. Applications and drivers must set
 	the array to zero.
 
+.. _v4l2-mbus-framefmt-flags:
+
+.. flat-table:: v4l2_mbus_framefmt Flags
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * .. _`mbus-framefmt-set-csc`:
+
+      - ``V4L2_MBUS_FRAMEFMT_SET_CSC``
+      - 0x00000001
+      - Set by the application. It is only used for capture and is
+	ignored for output streams. If set, then request the subdevice to do
+	colorspace conversion from the received colorspace to the requested
+	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
+	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry
+	setting will remain unchanged from what was received. So to change the
+	quantization, only the ``quantization`` field shall be set to non-zero values
+	(``V4L2_QUANTIZATION_FULL_RANGE`` or ``V4L2_QUANTIZATION_LIM_RANGE``)
+	and all other colorimetry fields shall be set to 0.
+
+	To check which conversions are supported by the hardware for the current
+	media bus frame format, see :ref:`v4l2-subdev-mbus-code-flags`.
 
 
 .. _v4l2-mbus-pixelcode:
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
index 35b8607203a4..8ed355a285e9 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
@@ -79,11 +79,53 @@ information about the try formats.
       - Media bus format codes to be enumerated, from enum
 	:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
     * - __u32
-      - ``reserved``\ [8]
+      - ``flags``
+      - See :ref:`v4l2-subdev-mbus-code-flags`
+    * - __u32
+      - ``reserved``\ [7]
       - Reserved for future extensions. Applications and drivers must set
 	the array to zero.
 
 
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{7.7cm}|
+
+.. _v4l2-subdev-mbus-code-flags:
+
+.. flat-table:: Subdev Media Bus Code Enumerate Flags
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE
+      - 0x00000001
+      - The driver allows the application to try to change the default colorspace
+        encoding. The application can ask to configure the colorspace of the
+	subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
+	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
+	See :ref:`v4l2-mbus-format` on how to do this.
+    * - V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
+      - 0x00000002
+      - The driver allows the application to try to change the default Y'CbCr
+	encoding. The application can ask to configure the ycbcr_enc of the
+	subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
+	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
+	See :ref:`v4l2-mbus-format` on how to do this.
+    * - V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION
+      - 0x00000004
+      - The driver allows the application to try to change the default
+	quantization. The application can ask to configure the quantization of
+	the subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
+	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
+	See :ref:`v4l2-mbus-format` on how to do this.
+    * - V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC
+      - 0x00000008
+      - The driver allows the application to try to change the default transform function.
+	The application can ask to configure the transform function of
+	the subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
+	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
+	See :ref:`v4l2-mbus-format` on how to do this.
+
 Return Value
 ============
 
diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
index 123a231001a8..3b7d692b4015 100644
--- a/include/uapi/linux/v4l2-mediabus.h
+++ b/include/uapi/linux/v4l2-mediabus.h
@@ -16,6 +16,8 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
+#define V4L2_MBUS_FRAMEFMT_SET_CSC	0x00000001
+
 /**
  * struct v4l2_mbus_framefmt - frame format on the media bus
  * @width:	image width
@@ -26,6 +28,9 @@
  * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
+ * @reserved2:  two reserved bytes that can be later used
+ * @flags:	flags (V4L2_MBUS_FRAMEFMT_*)
+ * @reserved:  reserved bytes that can be later used
  */
 struct v4l2_mbus_framefmt {
 	__u32			width;
@@ -36,7 +41,9 @@ struct v4l2_mbus_framefmt {
 	__u16			ycbcr_enc;
 	__u16			quantization;
 	__u16			xfer_func;
-	__u16			reserved[11];
+	__u16			reserved2;
+	__u32			flags;
+	__u16			reserved[8];
 };
 
 #ifndef __KERNEL__
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index 5d2a1dab7911..c20aa9a89864 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -65,19 +65,25 @@ struct v4l2_subdev_crop {
 	__u32 reserved[8];
 };
 
+#define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
+#define V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC	0x00000002
+#define V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION	0x00000004
+#define V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC	0x00000008
 /**
  * struct v4l2_subdev_mbus_code_enum - Media bus format enumeration
  * @pad: pad number, as reported by the media API
  * @index: format index during enumeration
  * @code: format code (MEDIA_BUS_FMT_ definitions)
  * @which: format type (from enum v4l2_subdev_format_whence)
+ * @flags: flags set by the driver, (V4L2_SUBDEV_MBUS_CODE_*)
  */
 struct v4l2_subdev_mbus_code_enum {
 	__u32 pad;
 	__u32 index;
 	__u32 code;
 	__u32 which;
-	__u32 reserved[8];
+	__u32 flags;
+	__u32 reserved[7];
 };
 
 /**
-- 
2.17.1

[PATCH v5 5/7] media: v4l2: add support for the subdev CSC API for hsv_enc on mediabus

[Dafna Hirschfeld]

Add the flag V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC that drivers
can set when enumerating the supported mediabus formats
on subdevices to indicate that the userspace can ask to
set the 'hsv_enc'. The patch also replaces the 'ycbcr_enc'
field in 'struct v4l2_mbus_framefmt' with a union that
includes 'hsv_enc'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../media/v4l/subdev-formats.rst              | 33 +++++++++++++++----
 include/uapi/linux/v4l2-mediabus.h            |  8 ++++-
 include/uapi/linux/v4l2-subdev.h              |  1 +
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
index 7362ee0b1e96..dddc38191547 100644
--- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
+++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
@@ -51,7 +51,9 @@ Media Bus Formats
 	The driver indicates that colorspace conversion is supported by setting
 	the flag V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE in the corresponding struct
 	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
-	See :ref:`colorspaces`.
+	See :ref:`v4l2-subdev-mbus-code-flags`.
+    * - union {
+      - (anonymous)
     * - __u16
       - ``ycbcr_enc``
       - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
@@ -67,7 +69,23 @@ Media Bus Formats
 	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC in the corresponding struct
 	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
 	See :ref:`v4l2-subdev-mbus-code-flags`.
-
+    * - __u16
+      - ``hsv_enc``
+      - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
+        This information supplements the ``colorspace`` and must be set by
+	the driver for capture streams and by the application for output
+	streams, see :ref:`colorspaces`. If the application sets the
+	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
+	this field for a capture stream to request a specific HSV encoding
+	for the media bus data. If the driver cannot handle requested
+	conversion, it will return another supported encoding.
+	This field is ignored for Y'CbCr media bus formats. The driver indicates
+	that hsv_enc conversion is supported by setting the flag
+	V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC in the corresponding struct
+	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
+	See :ref:`v4l2-subdev-mbus-code-flags`
+    * - }
+      -
     * - __u16
       - ``quantization``
       - Quantization range, from enum :c:type:`v4l2_quantization`.
@@ -123,11 +141,12 @@ Media Bus Formats
 	ignored for output streams. If set, then request the subdevice to do
 	colorspace conversion from the received colorspace to the requested
 	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
-	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry
-	setting will remain unchanged from what was received. So to change the
-	quantization, only the ``quantization`` field shall be set to non-zero values
-	(``V4L2_QUANTIZATION_FULL_RANGE`` or ``V4L2_QUANTIZATION_LIM_RANGE``)
-	and all other colorimetry fields shall be set to 0.
+	''hsv_enc``, ``quantization`` or ``xfer_func``) is set to 0, then that
+	colorimetry setting will remain unchanged from what was received. So to
+	change the quantization, only the ``quantization`` field shall be set to
+	non-zero values (``V4L2_QUANTIZATION_FULL_RANGE`` or
+	``V4L2_QUANTIZATION_LIM_RANGE``) and all other colorimetry fields shall
+	be set to 0.
 
 	To check which conversions are supported by the hardware for the current
 	media bus frame format, see :ref:`v4l2-subdev-mbus-code-flags`.
diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
index 3b7d692b4015..d0bc8df18ad5 100644
--- a/include/uapi/linux/v4l2-mediabus.h
+++ b/include/uapi/linux/v4l2-mediabus.h
@@ -26,6 +26,7 @@
  * @field:	used interlacing type (from enum v4l2_field)
  * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
  * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
+ * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
  * @reserved2:  two reserved bytes that can be later used
@@ -38,7 +39,12 @@ struct v4l2_mbus_framefmt {
 	__u32			code;
 	__u32			field;
 	__u32			colorspace;
-	__u16			ycbcr_enc;
+	union {
+		/* enum v4l2_ycbcr_encoding */
+		__u16			ycbcr_enc;
+		/* enum v4l2_hsv_encoding */
+		__u16			hsv_enc;
+	};
 	__u16			quantization;
 	__u16			xfer_func;
 	__u16			reserved2;
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index c20aa9a89864..08e25a961d15 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -67,6 +67,7 @@ struct v4l2_subdev_crop {
 
 #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
 #define V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC	0x00000002
+#define V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
 #define V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION	0x00000004
 #define V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC	0x00000008
 /**
-- 
2.17.1

[PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Dafna Hirschfeld]

The isp entity has a hardware support to force full range quantization
for YUV formats. Use the subdev CSC API to allow userspace to set the
quantization for YUV formats on the isp entity.

- The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
YUV formats during enumeration of the isp formats on the source pad.
- The full quantization is set on YUV formats if userspace ask it
on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.

On the capture and the resizer, the quantization is read-only
and always set to the default quantization.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/TODO             |  2 +-
 drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
index c0cbec0a164d..db05288949bd 100644
--- a/drivers/staging/media/rkisp1/TODO
+++ b/drivers/staging/media/rkisp1/TODO
@@ -2,7 +2,7 @@
 * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
 * Fix checkpatch errors.
 * Review and comment every lock
-* Handle quantization
+* Add uapi docs. Remeber to add documentation of how quantization is handled.
 * Document rkisp1-common.h
 * streaming paths (mainpath and selfpath) check if the other path is streaming
 in several places of the code, review this, specially that it doesn't seem it
diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index f69235f82c45..93d6846886f2 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
 			   const struct v4l2_format_info **fmt_info)
 {
 	const struct rkisp1_capture_config *config = cap->config;
-	struct rkisp1_capture *other_cap =
-			&cap->rkisp1->capture_devs[cap->id ^ 1];
 	const struct rkisp1_capture_fmt_cfg *fmt;
 	const struct v4l2_format_info *info;
 	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
@@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
 
 	info = rkisp1_fill_pixfmt(pixm, cap->id);
 
-	/* can not change quantization when stream-on */
-	if (other_cap->is_streaming)
-		pixm->quantization = other_cap->pix.fmt.quantization;
-	/* output full range by default, take effect in params */
-	else if (!pixm->quantization ||
-		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
-		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-
 	if (fmt_cfg)
 		*fmt_cfg = fmt;
 	if (fmt_info)
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 58c90c67594d..d575c1e4dc4b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
 
 		if (code->index == pos - 1) {
 			code->code = fmt->mbus_code;
+			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
+			    dir == RKISP1_ISP_SD_SRC)
+				code->flags =
+					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
 			return 0;
 		}
 	}
@@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
 					     RKISP1_ISP_PAD_SOURCE_VIDEO);
 	*src_fmt = *sink_fmt;
 	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
-	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 
 	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
 					    RKISP1_ISP_PAD_SOURCE_VIDEO);
@@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 		isp->src_fmt = mbus_info;
 	src_fmt->width  = src_crop->width;
 	src_fmt->height = src_crop->height;
-	src_fmt->quantization = format->quantization;
-	/* full range by default */
-	if (!src_fmt->quantization)
+
+	/*
+	 * The CSC API is used to allow userspace to force full
+	 * quantization on YUV formats.
+	 */
+	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
+	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
+	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
 		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	else
+		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
 
 	*format = *src_fmt;
 }
-- 
2.17.1

[PATCH v5 7/7] media: staging: rkisp1: rsz: set flags to 0 in enum_mbus_code cb

[Dafna Hirschfeld]

The resizer calls the enum_mbus_code cb on the source pad of
the isp entity since they support the same formats.
The only difference is that the isp entity allows setting the
quantization and sets the flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION.
The resizer should therefore set the flags to 0.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 8bc907ffa09b..31b5ac1f27a9 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -444,6 +444,7 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
 
 	/* restore pad */
 	code->pad = pad;
+	code->flags = 0;
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_enum_mbus_code':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: 'RKISP1_ISP_SD_SRC' undeclared (first use in this function); did you mean 'RKISP1_RSZ_PAD_SRC'?
     597 |        dir == RKISP1_ISP_SD_SRC)
         |               ^~~~~~~~~~~~~~~~~
         |               RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: note: each undeclared identifier is reported only once for each function it appears in

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73547 bytes --]

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_enum_mbus_code':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: 'RKISP1_ISP_SD_SRC' undeclared (first use in this function); did you mean 'RKISP1_RSZ_PAD_SRC'?
     597 |        dir == RKISP1_ISP_SD_SRC)
         |               ^~~~~~~~~~~~~~~~~
         |               RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: note: each undeclared identifier is reported only once for each function it appears in

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73547 bytes --]

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_enum_mbus_code':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: 'RKISP1_ISP_SD_SRC' undeclared (first use in this function); did you mean 'RKISP1_RSZ_PAD_SRC'?
     597 |        dir == RKISP1_ISP_SD_SRC)
         |               ^~~~~~~~~~~~~~~~~
         |               RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: note: each undeclared identifier is reported only once for each function it appears in

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73547 bytes --]

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Helen Koike]

Hi Dafna,

Thanks for your patch.

On 7/3/20 2:10 PM, Dafna Hirschfeld wrote:
> The isp entity has a hardware support to force full range quantization
> for YUV formats. Use the subdev CSC API to allow userspace to set the
> quantization for YUV formats on the isp entity.
> 
> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
> YUV formats during enumeration of the isp formats on the source pad.
> - The full quantization is set on YUV formats if userspace ask it
> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> 
> On the capture and the resizer, the quantization is read-only
> and always set to the default quantization.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO             |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index c0cbec0a164d..db05288949bd 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -2,7 +2,7 @@
>  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
>  * Fix checkpatch errors.
>  * Review and comment every lock
> -* Handle quantization
> +* Add uapi docs. Remeber to add documentation of how quantization is handled.
>  * Document rkisp1-common.h
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index f69235f82c45..93d6846886f2 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  			   const struct v4l2_format_info **fmt_info)
>  {
>  	const struct rkisp1_capture_config *config = cap->config;
> -	struct rkisp1_capture *other_cap =
> -			&cap->rkisp1->capture_devs[cap->id ^ 1];
>  	const struct rkisp1_capture_fmt_cfg *fmt;
>  	const struct v4l2_format_info *info;
>  	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  
>  	info = rkisp1_fill_pixfmt(pixm, cap->id);
>  
> -	/* can not change quantization when stream-on */
> -	if (other_cap->is_streaming)
> -		pixm->quantization = other_cap->pix.fmt.quantization;
> -	/* output full range by default, take effect in params */
> -	else if (!pixm->quantization ||
> -		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> -		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -
>  	if (fmt_cfg)
>  		*fmt_cfg = fmt;
>  	if (fmt_info)
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 58c90c67594d..d575c1e4dc4b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
>  
>  		if (code->index == pos - 1) {
>  			code->code = fmt->mbus_code;
> +			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> +			    dir == RKISP1_ISP_SD_SRC)
> +				code->flags =
> +					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
>  			return 0;
>  		}
>  	}
> @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
>  					     RKISP1_ISP_PAD_SOURCE_VIDEO);
>  	*src_fmt = *sink_fmt;
>  	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> -	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  
>  	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
>  					    RKISP1_ISP_PAD_SOURCE_VIDEO);
> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>  		isp->src_fmt = mbus_info;
>  	src_fmt->width  = src_crop->width;
>  	src_fmt->height = src_crop->height;
> -	src_fmt->quantization = format->quantization;
> -	/* full range by default */
> -	if (!src_fmt->quantization)
> +
> +	/*
> +	 * The CSC API is used to allow userspace to force full
> +	 * quantization on YUV formats.
> +	 */
> +	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> +	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> +	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>  		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	else
> +		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;

According to the docs [1]:

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization

"The default colorspace. This can be used by applications to let the driver fill in the colorspace."

So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace.

We have the same issue in the resizer.

I also see an issue in the capture that allows userspace to change quantization, and this should be fixed.

In my understanding, since you are leaving all the other pads and video nodes with read-only quantization,
changing quantization in the ISP source pad should change the readings in all the other pads and video nodes.

So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets
V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report
V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver.

Regards,
Helen

>  
>  	*format = *src_fmt;
>  }
> 

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Tomasz Figa]

Hi Helen,

On Fri, Jul 3, 2020 at 10:00 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Dafna,
>
> Thanks for your patch.
>
> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote:
> > The isp entity has a hardware support to force full range quantization
> > for YUV formats. Use the subdev CSC API to allow userspace to set the
> > quantization for YUV formats on the isp entity.
> >
> > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
> > YUV formats during enumeration of the isp formats on the source pad.
> > - The full quantization is set on YUV formats if userspace ask it
> > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> >
> > On the capture and the resizer, the quantization is read-only
> > and always set to the default quantization.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/TODO             |  2 +-
> >  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
> >  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
> >  3 files changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> > index c0cbec0a164d..db05288949bd 100644
> > --- a/drivers/staging/media/rkisp1/TODO
> > +++ b/drivers/staging/media/rkisp1/TODO
> > @@ -2,7 +2,7 @@
> >  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
> >  * Fix checkpatch errors.
> >  * Review and comment every lock
> > -* Handle quantization
> > +* Add uapi docs. Remeber to add documentation of how quantization is handled.
> >  * Document rkisp1-common.h
> >  * streaming paths (mainpath and selfpath) check if the other path is streaming
> >  in several places of the code, review this, specially that it doesn't seem it
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index f69235f82c45..93d6846886f2 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >                          const struct v4l2_format_info **fmt_info)
> >  {
> >       const struct rkisp1_capture_config *config = cap->config;
> > -     struct rkisp1_capture *other_cap =
> > -                     &cap->rkisp1->capture_devs[cap->id ^ 1];
> >       const struct rkisp1_capture_fmt_cfg *fmt;
> >       const struct v4l2_format_info *info;
> >       const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >
> >       info = rkisp1_fill_pixfmt(pixm, cap->id);
> >
> > -     /* can not change quantization when stream-on */
> > -     if (other_cap->is_streaming)
> > -             pixm->quantization = other_cap->pix.fmt.quantization;
> > -     /* output full range by default, take effect in params */
> > -     else if (!pixm->quantization ||
> > -              pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> > -             pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > -
> >       if (fmt_cfg)
> >               *fmt_cfg = fmt;
> >       if (fmt_info)
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > index 58c90c67594d..d575c1e4dc4b 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
> >
> >               if (code->index == pos - 1) {
> >                       code->code = fmt->mbus_code;
> > +                     if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> > +                         dir == RKISP1_ISP_SD_SRC)
> > +                             code->flags =
> > +                                     V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
> >                       return 0;
> >               }
> >       }
> > @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
> >                                            RKISP1_ISP_PAD_SOURCE_VIDEO);
> >       *src_fmt = *sink_fmt;
> >       src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> > -     src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >
> >       src_crop = v4l2_subdev_get_try_crop(sd, cfg,
> >                                           RKISP1_ISP_PAD_SOURCE_VIDEO);
> > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> >               isp->src_fmt = mbus_info;
> >       src_fmt->width  = src_crop->width;
> >       src_fmt->height = src_crop->height;
> > -     src_fmt->quantization = format->quantization;
> > -     /* full range by default */
> > -     if (!src_fmt->quantization)
> > +
> > +     /*
> > +      * The CSC API is used to allow userspace to force full
> > +      * quantization on YUV formats.
> > +      */
> > +     if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> > +         format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> > +         mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
> >               src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +     else
> > +             src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>
> According to the docs [1]:
>
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization
>
> "The default colorspace. This can be used by applications to let the driver fill in the colorspace."
>
> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace.
>
> We have the same issue in the resizer.
>
> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed.
>
> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization,
> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes.
>
> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets
> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report
> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver.

That's my interpretation of the spec and I would choose to implement
such behavior, however not everyone agrees on that. Please check the
discussion over the v3 series [1].

[1] https://lore.kernel.org/linux-media/20200702184324.GP12562@pendragon.ideasonboard.com/

Best regards,
Tomasz

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ca464639a1c9dd3944eb055ffd2796e8c2e7639f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: use of undeclared identifier 'RKISP1_ISP_SD_SRC'; did you mean 'RKISP1_RSZ_PAD_SRC'?
                               dir == RKISP1_ISP_SD_SRC)
                                      ^~~~~~~~~~~~~~~~~
                                      RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-common.h:47:2: note: 'RKISP1_RSZ_PAD_SRC' declared here
           RKISP1_RSZ_PAD_SRC,
           ^
   1 error generated.

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75323 bytes --]

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ca464639a1c9dd3944eb055ffd2796e8c2e7639f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: use of undeclared identifier 'RKISP1_ISP_SD_SRC'; did you mean 'RKISP1_RSZ_PAD_SRC'?
                               dir == RKISP1_ISP_SD_SRC)
                                      ^~~~~~~~~~~~~~~~~
                                      RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-common.h:47:2: note: 'RKISP1_RSZ_PAD_SRC' declared here
           RKISP1_RSZ_PAD_SRC,
           ^
   1 error generated.

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75323 bytes --]

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]

Hi Dafna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ca464639a1c9dd3944eb055ffd2796e8c2e7639f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: use of undeclared identifier 'RKISP1_ISP_SD_SRC'; did you mean 'RKISP1_RSZ_PAD_SRC'?
                               dir == RKISP1_ISP_SD_SRC)
                                      ^~~~~~~~~~~~~~~~~
                                      RKISP1_RSZ_PAD_SRC
   drivers/staging/media/rkisp1/rkisp1-common.h:47:2: note: 'RKISP1_RSZ_PAD_SRC' declared here
           RKISP1_RSZ_PAD_SRC,
           ^
   1 error generated.

vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c

   562	
   563	/* ----------------------------------------------------------------------------
   564	 * Subdev pad operations
   565	 */
   566	
   567	static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
   568					     struct v4l2_subdev_pad_config *cfg,
   569					     struct v4l2_subdev_mbus_code_enum *code)
   570	{
   571		unsigned int i, dir;
   572		int pos = 0;
   573	
   574		if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
   575			dir = RKISP1_DIR_SINK;
   576		} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
   577			dir = RKISP1_DIR_SRC;
   578		} else {
   579			if (code->index > 0)
   580				return -EINVAL;
   581			code->code = MEDIA_BUS_FMT_FIXED;
   582			return 0;
   583		}
   584	
   585		if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
   586			return -EINVAL;
   587	
   588		for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
   589			const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
   590	
   591			if (fmt->direction & dir)
   592				pos++;
   593	
   594			if (code->index == pos - 1) {
   595				code->code = fmt->mbus_code;
   596				if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
 > 597				    dir == RKISP1_ISP_SD_SRC)
   598					code->flags =
   599						V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
   600				return 0;
   601			}
   602		}
   603	
   604		return -EINVAL;
   605	}
   606	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75323 bytes --]

Re: [PATCH v5 2/7] v4l2: add support for colorspace conversion API (CSC) for video capture

[Hans Verkuil]

On 03/07/2020 19:10, Dafna Hirschfeld wrote:
> For video capture it is the driver that reports the colorspace,
> Y'CbCr/HSV encoding, quantization range and transfer function

In a lot of places in this series the colorimetry parameters are
ordered as colorspace, Y'CbCr/HSV encoding, quantization range and
transfer function, but 'transfer function' should come after
'colorspace'.

I.e. the linear RGB values using a specific colorspace are passed
through the transfer function (aka gamma), pixel encoding and finally
the quantization step. So this order should be kept in both code
and documentation wherever possible.

> used by the video, and there is no way to request something
> different, even though many HDTV receivers have some sort of
> colorspace conversion capabilities.
> 
> For output video this feature already exists since the application
> specifies this information for the video format it will send out, and
> the transmitter will enable any available CSC if a format conversion has
> to be performed in order to match the capabilities of the sink.
> 
> For video capture we propose adding new v4l2_pix_format flag:
> V4L2_PIX_FMT_FLAG_SET_CSC. The flag is set by the application,
> the driver will interpret the colorspace, ycbcr_enc/hsv_enc, quantization
> and xfer_func fields as the requested colorspace information and will

So reorder above,

> attempt to do the conversion it supports.
> 
> Drivers set the flags
> V4L2_FMT_FLAG_CSC_COLORSPACE,
> V4L2_FMT_FLAG_CSC_YCBCR_ENC,
> V4L2_FMT_FLAG_CSC_HSV_ENC,
> V4L2_FMT_FLAG_CSC_QUANTIZATION,
> V4L2_FMT_FLAG_CSC_XFER_FUNC,

and here (and combine the two ENCs to:

V4L2_FMT_FLAG_CSC_YCBCR_ENC/V4L2_FMT_FLAG_CSC_HSV_ENC,

> in the flags field of the struct v4l2_fmtdesc during enumeration to
> indicate that they support colorspace conversion for the respective field.
> 
> Drivers do not have to actually look at the flags. If the flags are not
> set, then the fields 'colorspace', 'ycbcr_enc/hsv_enc', 'quantization'
> and 'xfer_func' are set to the default values by the core, i.e. just

Reorder.

> pass on the received format without conversion.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/v4l/pixfmt-v4l2-mplane.rst          | 16 ++---
>  .../userspace-api/media/v4l/pixfmt-v4l2.rst   | 64 +++++++++++++++++--
>  .../media/v4l/vidioc-enum-fmt.rst             | 35 ++++++++++
>  .../media/videodev2.h.rst.exceptions          |  5 ++
>  include/uapi/linux/videodev2.h                |  6 ++
>  5 files changed, 109 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst b/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
> index 444b4082684c..66f3365d7b72 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst
> @@ -105,29 +105,21 @@ describing all planes of that format.
>      * - __u8
>        - ``ycbcr_enc``
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``hsv_enc``
>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - }
>        -
>      * - __u8
>        - ``quantization``
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``xfer_func``
>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
> -        This information supplements the ``colorspace`` and must be set by
> -	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	See struct :c:type:`v4l2_pix_format`.
>      * - __u8
>        - ``reserved[7]``
>        - Reserved for future extensions. Should be zeroed by drivers and
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
> index 56a2952de873..d9f8f7eb7098 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
> @@ -116,7 +116,14 @@ Single-planar format structure
>        - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>          This information supplements the ``pixelformat`` and must be set
>  	by the driver for capture streams and by the application for
> -	output streams, see :ref:`colorspaces`.
> +	output streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific colorspace
> +	for the captured image data. If the driver cannot handle requested
> +	conversion, it will return another supported colorspace.
> +	The driver indicates that colorspace conversion is supported by setting
> +	the flag V4L2_FMT_FLAG_CSC_COLORSPACE in the corresponding struct
> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
>      * - __u32
>        - ``priv``
>        - This field indicates whether the remaining fields of the
> @@ -153,13 +160,29 @@ Single-planar format structure
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific Y'CbCr encoding
> +	for the captured image data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	This field is ignored for HSV pixelformats. The driver indicates that
> +	ycbcr_enc conversion is supported by setting the flag
> +	V4L2_FMT_FLAG_CSC_YCBCR_ENC in the corresponding struct
> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
>      * - __u32
>        - ``hsv_enc``
>        - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set this
> +	field for a capture stream to request a specific HSV encoding for the
> +	captured image data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	This field is ignored for non-HSV pixelformats. The driver indicates
> +	that hsv_enc conversion is supported by setting the flag
> +	V4L2_FMT_FLAG_CSC_HSV_ENC in the corresponding struct
> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
>      * - }
>        -
>      * - __u32
> @@ -167,13 +190,27 @@ Single-planar format structure
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific quantization
> +	range for the captured image data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	The driver indicates that quantization conversion is supported by setting
> +	the flag V4L2_FMT_FLAG_CSC_QUANTIZATION in the corresponding struct
> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
>      * - __u32
>        - ``xfer_func``
>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the flag
> +	``V4L2_PIX_FMT_FLAG_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific Transfer function
> +	for the captured image data. If the driver cannot handle requested
> +	conversion, it will return another supported Transfer function.
> +	The driver indicates that xfer_func conversion is supported by setting
> +	the flag V4L2_FMT_FLAG_CSC_XFER_FUNC in the corresponding struct
> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`.
>  
>  .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>  
> @@ -191,3 +228,20 @@ Single-planar format structure
>  	by RGBA values (128, 192, 255, 128), the same pixel described with
>  	premultiplied colors would be described by RGBA values (64, 96,
>  	128, 128)
> +    * .. _`v4l2-pix-fmt-flag-set-csc`:
> +
> +      - ``V4L2_PIX_FMT_FLAG_SET_CSC``
> +      - 0x00000002
> +      - Set by the application. It is only used for capture and is
> +        ignored for output streams. If set, then request the device to do
> +	colorspace conversion from the received colorspace to the requested
> +	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,

If -> If the

> +	``hsv_enc``, ``quantization`` or ``xfer_func``) is set to 0, then that

reorder ``xfer_func``.

> +	colorimetry setting will remain unchanged from what was received. So to
> +	change the quantization only the ``quantization`` field shall be set to
> +	non-zero values (``V4L2_QUANTIZATION_FULL_RANGE`` or
> +	``V4L2_QUANTIZATION_LIM_RANGE``) and all other colorimetry fields shall
> +	be set to 0.
> +
> +	To check which conversions are supported by the hardware for the current
> +	pixel format, see :ref:`fmtdesc-flags`.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 05835e04c20b..98595dd48557 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -198,6 +198,41 @@ the ``mbus_code`` field is handled differently:
>  	This flag can only be used in combination with the
>  	``V4L2_FMT_FLAG_COMPRESSED`` flag, since this applies to
>          compressed formats only. This flag is valid for stateful encoders only.
> +    * - ``V4L2_FMT_FLAG_CSC_COLORSPACE``
> +      - 0x0020
> +      - The driver allows the application to try to change the default
> +	colorspace. This flag is relevant only for capture devices.
> +	The application can ask to configure the colorspace of the capture device
> +	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> +	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> +    * - ``V4L2_FMT_FLAG_CSC_YCBCR_ENC``
> +      - 0x0040
> +      - The driver allows the application to try to change the default
> +	Y'CbCr encoding. This flag is relevant only for capture devices.
> +	The application can ask to configure the ycbcr_enc of the capture device
> +	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> +	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> +    * - ``V4L2_FMT_FLAG_CSC_HSV_ENC``
> +      - 0x0040
> +      - The driver allows the application to try to change the default
> +	HSV encoding. This flag is relevant only for capture devices.
> +	The application can ask to configure the hsv_enc of the capture device
> +	when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> +	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> +    * - ``V4L2_FMT_FLAG_CSC_QUANTIZATION``
> +      - 0x0080
> +      - The driver allows the application to try to change the default
> +	quantization. This flag is relevant only for capture devices.
> +	The application can ask to configure the quantization of the capture
> +	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> +	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> +    * - ``V4L2_FMT_FLAG_CSC_XFER_FUNC``
> +      - 0x0100

Reorder, the flag values also change.

> +      - The driver allows the application to try to change the default
> +	quantization. This flag is relevant only for capture devices.
> +	The application can ask to configure the quantization of the capture
> +	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> +	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
>  
>  
>  Return Value
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 659799cc1eca..faa19e7d241b 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -188,6 +188,11 @@ replace define V4L2_FMT_FLAG_EMULATED fmtdesc-flags
>  replace define V4L2_FMT_FLAG_CONTINUOUS_BYTESTREAM fmtdesc-flags
>  replace define V4L2_FMT_FLAG_DYN_RESOLUTION fmtdesc-flags
>  replace define V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL fmtdesc-flags
> +replace define V4L2_FMT_FLAG_CSC_COLORSPACE fmtdesc-flags
> +replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
> +replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
> +replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
> +replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags

Reorder

>  
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 303805438814..76610509d670 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -776,6 +776,7 @@ struct v4l2_pix_format {
>  
>  /* Flags */
>  #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
> +#define V4L2_PIX_FMT_FLAG_SET_CSC	0x00000002
>  
>  /*
>   *	F O R M A T   E N U M E R A T I O N
> @@ -795,6 +796,11 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CONTINUOUS_BYTESTREAM	0x0004
>  #define V4L2_FMT_FLAG_DYN_RESOLUTION		0x0008
>  #define V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL	0x0010
> +#define V4L2_FMT_FLAG_CSC_COLORSPACE		0x0020
> +#define V4L2_FMT_FLAG_CSC_YCBCR_ENC		0x0040
> +#define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> +#define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0080
> +#define V4L2_FMT_FLAG_CSC_XFER_FUNC		0x0100

Reorder and renumber the values accordingly.

>  
>  	/* Frame Size and frame rate enumeration */
>  /*
> 

Regards,

	Hans

Re: [PATCH v5 3/7] media: vivid: Add support to the CSC API

[Hans Verkuil]

On 03/07/2020 19:10, Dafna Hirschfeld wrote:
> The CSC API (Colorspace conversion) allows userspace to try
> to configure the ycbcr/hsv_enc function and the quantization
> for capture devices. This patch adds support to the CSC API

Not just ycbcr/hsv_enc and quantization but also colorspace and
transfer function!

> in vivid.
> Using the CSC API, userspace is allowed to do the following:
> 
> - Set the ycbcr_enc function for YUV formats.
> - Set the hsv_enc function for HSV formats
> - Set the quantization for YUV and RGB formats.
> - Set the xfer_func.
> - Set the colorspace.

Reorder.

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/test-drivers/vivid/vivid-vid-cap.c  | 74 +++++++++++++++++--
>  .../test-drivers/vivid/vivid-vid-common.c     | 24 ++++++
>  2 files changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> index e94beef008c8..8916def70ffe 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> @@ -549,6 +549,45 @@ int vivid_g_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +static bool vivid_is_colorspace_valid(__u32 colorspace)
> +{
> +	if (colorspace > V4L2_COLORSPACE_DEFAULT &&
> +	    colorspace <= V4L2_COLORSPACE_DCI_P3)
> +		return true;
> +	return false;
> +}
> +
> +static bool vivid_is_hsv_enc_valid(__u8 hsv_enc)
> +{
> +	if (hsv_enc == V4L2_HSV_ENC_180 || hsv_enc == V4L2_HSV_ENC_256)
> +		return true;
> +	return false;
> +}
> +
> +static bool vivid_is_ycbcr_enc_valid(__u8 ycbcr_enc)
> +{
> +	/* V4L2_YCBCR_ENC_SMPTE240M is the last ycbcr_enc enum */
> +	if (ycbcr_enc && ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M)
> +		return true;
> +	return false;
> +}
> +
> +static bool vivid_is_quant_valid(__u8 quantization)
> +{
> +	if (quantization == V4L2_QUANTIZATION_FULL_RANGE ||
> +	    quantization == V4L2_QUANTIZATION_LIM_RANGE)
> +		return true;
> +	return false;
> +}
> +
> +static bool vivid_is_xfer_func_valid(__u32 xfer_func)
> +{
> +	if (xfer_func > V4L2_XFER_FUNC_DEFAULT &&
> +	    xfer_func <= V4L2_XFER_FUNC_SMPTE2084)
> +		return true;
> +	return false;
> +}

Reorder these functions. But wouldn't this be useful to add to v4l2-common.h
as static inline helpers?

> +
>  int vivid_try_fmt_vid_cap(struct file *file, void *priv,
>  			struct v4l2_format *f)
>  {
> @@ -560,6 +599,7 @@ int vivid_try_fmt_vid_cap(struct file *file, void *priv,
>  	unsigned factor = 1;
>  	unsigned w, h;
>  	unsigned p;
> +	bool user_set_csc = !!(mp->flags & V4L2_PIX_FMT_FLAG_SET_CSC);
>  
>  	fmt = vivid_get_format(dev, mp->pixelformat);
>  	if (!fmt) {
> @@ -633,13 +673,26 @@ int vivid_try_fmt_vid_cap(struct file *file, void *priv,
>  			(fmt->bit_depth[p] / fmt->vdownsampling[p])) /
>  			(fmt->bit_depth[0] / fmt->vdownsampling[0]);
>  
> -	mp->colorspace = vivid_colorspace_cap(dev);
> -	if (fmt->color_enc == TGP_COLOR_ENC_HSV)
> -		mp->hsv_enc = vivid_hsv_enc_cap(dev);
> -	else
> +	if (!user_set_csc || !vivid_is_colorspace_valid(mp->colorspace))
> +		mp->colorspace = vivid_colorspace_cap(dev);
> +	if (fmt->color_enc == TGP_COLOR_ENC_HSV) {
> +		if (!user_set_csc || !vivid_is_hsv_enc_valid(mp->hsv_enc))
> +			mp->hsv_enc = vivid_hsv_enc_cap(dev);
> +	} else if (fmt->color_enc == TGP_COLOR_ENC_YCBCR) {
> +		if (!user_set_csc || !vivid_is_ycbcr_enc_valid(mp->ycbcr_enc))
> +			mp->ycbcr_enc = vivid_ycbcr_enc_cap(dev);
> +	} else {
>  		mp->ycbcr_enc = vivid_ycbcr_enc_cap(dev);
> -	mp->xfer_func = vivid_xfer_func_cap(dev);
> -	mp->quantization = vivid_quantization_cap(dev);
> +	}
> +	if (!user_set_csc || !vivid_is_xfer_func_valid(mp->xfer_func))
> +		mp->xfer_func = vivid_xfer_func_cap(dev);
> +	if (fmt->color_enc == TGP_COLOR_ENC_YCBCR ||
> +	    fmt->color_enc == TGP_COLOR_ENC_RGB) {
> +		if (!user_set_csc || !vivid_is_quant_valid(mp->quantization))
> +			mp->quantization = vivid_quantization_cap(dev);
> +	} else {
> +		mp->quantization = vivid_quantization_cap(dev);
> +	}
>  	memset(mp->reserved, 0, sizeof(mp->reserved));
>  	return 0;
>  }
> @@ -769,6 +822,15 @@ int vivid_s_fmt_vid_cap(struct file *file, void *priv,
>  	if (vivid_is_sdtv_cap(dev))
>  		dev->tv_field_cap = mp->field;
>  	tpg_update_mv_step(&dev->tpg);
> +	dev->tpg.colorspace = mp->colorspace;
> +	dev->tpg.quantization = mp->quantization;
> +	if (dev->fmt_cap->color_enc == TGP_COLOR_ENC_YCBCR)
> +		dev->tpg.ycbcr_enc = mp->ycbcr_enc;
> +	else
> +		dev->tpg.hsv_enc = mp->hsv_enc;
> +	dev->tpg.quantization = mp->quantization;

Duplicate of the same assignment above.

> +	dev->tpg.xfer_func = mp->xfer_func;

Reorder.

> +
>  	return 0;
>  }
>  
> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-common.c b/drivers/media/test-drivers/vivid/vivid-vid-common.c
> index 76b0be670ebb..5cab9b2a74bd 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vid-common.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vid-common.c
> @@ -920,6 +920,30 @@ int vivid_enum_fmt_vid(struct file *file, void  *priv,
>  	fmt = &vivid_formats[f->index];
>  
>  	f->pixelformat = fmt->fourcc;
> +
> +	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	    f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		return 0;
> +	/*
> +	 * For capture devices, we support the CSC API.
> +	 * We allow userspace to:
> +	 * 1. set the ycbcr_enc on yuv format
> +	 * 2. set the hsv_enc on hsv format
> +	 * 3. set the quantization on yuv and rgb formats
> +	 * 4. set the colorspace
> +	 * 5. set the xfer_func
> +	 */
> +	if (fmt->color_enc == TGP_COLOR_ENC_YCBCR) {
> +		f->flags |= V4L2_FMT_FLAG_CSC_YCBCR_ENC;
> +		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION;
> +	} else if (fmt->color_enc == TGP_COLOR_ENC_HSV) {
> +		f->flags |= V4L2_FMT_FLAG_CSC_HSV_ENC;
> +	} else if (fmt->color_enc == TGP_COLOR_ENC_RGB) {
> +		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION;
> +	}
> +	f->flags |= V4L2_FMT_FLAG_CSC_COLORSPACE;
> +	f->flags |= V4L2_FMT_FLAG_CSC_XFER_FUNC;

Reorder.

Regards,

	Hans

> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Hans Verkuil]

On 03/07/2020 19:10, Dafna Hirschfeld wrote:
> This patch extends the CSC API in video devices to be supported
> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
> The flags:
> 
> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,

Reorder.

> 
> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
> 
> New 'flags' fields were added to the structs
> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
> from the 'reserved' field
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
>  .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
>  include/uapi/linux/v4l2-mediabus.h            |  9 ++-
>  include/uapi/linux/v4l2-subdev.h              |  8 +-
>  4 files changed, 129 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> index 9a4d61b0d76f..7362ee0b1e96 100644
> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> @@ -41,32 +41,96 @@ Media Bus Formats
>  	:ref:`field-order` for details.
>      * - __u32
>        - ``colorspace``
> -      - Image colorspace, from enum
> -	:c:type:`v4l2_colorspace`. See
> -	:ref:`colorspaces` for details.
> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
> +        Must be set by the driver for capture streams and by the application
> +	for output streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific colorspace
> +	for the media bus data. If the driver cannot handle requested
> +	conversion, it will return another supported colorspace.
> +	The driver indicates that colorspace conversion is supported by setting
> +	the flag V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE in the corresponding struct
> +	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
> +	See :ref:`colorspaces`.
>      * - __u16
>        - ``ycbcr_enc``
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific Y'CbCr encoding
> +	for the media bus data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	This field is ignored for HSV media bus formats. The driver indicates
> +	that ycbcr_enc conversion is supported by setting the flag
> +	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC in the corresponding struct
> +	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
> +	See :ref:`v4l2-subdev-mbus-code-flags`.
> +
>      * - __u16
>        - ``quantization``
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific quantization
> +	encoding for the media bus data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	The driver indicates that quantization conversion is supported by
> +	setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION in the
> +	corresponding struct :c:type:`v4l2_subdev_mbus_code_enum`
> +	during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`.
> +
>      * - __u16
>        - ``xfer_func``
>        - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>          This information supplements the ``colorspace`` and must be set by
>  	the driver for capture streams and by the application for output
> -	streams, see :ref:`colorspaces`.
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific transfer
> +	function for the media bus data. If the driver cannot handle the requested
> +	conversion, it will return another supported transfer function.
> +	The driver indicates that the transfer function conversion is supported by
> +	setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC in the
> +	corresponding struct :c:type:`v4l2_subdev_mbus_code_enum`
> +	during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`.
> +    * - __u16
> +      - ``reserved2``
> +      - Reserved for future extensions.
> +    * - __u32
> +      - ``flags``
> +      - flags See:  :ref:v4l2-mbus-framefmt-flags
>      * - __u16
> -      - ``reserved``\ [11]
> +      - ``reserved``\ [8]
>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
>  
> +.. _v4l2-mbus-framefmt-flags:
> +
> +.. flat-table:: v4l2_mbus_framefmt Flags
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * .. _`mbus-framefmt-set-csc`:
> +
> +      - ``V4L2_MBUS_FRAMEFMT_SET_CSC``
> +      - 0x00000001
> +      - Set by the application. It is only used for capture and is
> +	ignored for output streams. If set, then request the subdevice to do
> +	colorspace conversion from the received colorspace to the requested
> +	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,

If -> If the

> +	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry

Reorder.

> +	setting will remain unchanged from what was received. So to change the
> +	quantization, only the ``quantization`` field shall be set to non-zero values
> +	(``V4L2_QUANTIZATION_FULL_RANGE`` or ``V4L2_QUANTIZATION_LIM_RANGE``)
> +	and all other colorimetry fields shall be set to 0.
> +
> +	To check which conversions are supported by the hardware for the current
> +	media bus frame format, see :ref:`v4l2-subdev-mbus-code-flags`.
>  
>  
>  .. _v4l2-mbus-pixelcode:
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> index 35b8607203a4..8ed355a285e9 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> @@ -79,11 +79,53 @@ information about the try formats.
>        - Media bus format codes to be enumerated, from enum
>  	:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>      * - __u32
> -      - ``reserved``\ [8]
> +      - ``flags``
> +      - See :ref:`v4l2-subdev-mbus-code-flags`
> +    * - __u32
> +      - ``reserved``\ [7]
>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
>  
>  
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{7.7cm}|
> +
> +.. _v4l2-subdev-mbus-code-flags:
> +
> +.. flat-table:: Subdev Media Bus Code Enumerate Flags
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE
> +      - 0x00000001
> +      - The driver allows the application to try to change the default colorspace
> +        encoding. The application can ask to configure the colorspace of the
> +	subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
> +	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
> +	See :ref:`v4l2-mbus-format` on how to do this.
> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
> +      - 0x00000002
> +      - The driver allows the application to try to change the default Y'CbCr
> +	encoding. The application can ask to configure the ycbcr_enc of the
> +	subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
> +	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
> +	See :ref:`v4l2-mbus-format` on how to do this.
> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION
> +      - 0x00000004
> +      - The driver allows the application to try to change the default
> +	quantization. The application can ask to configure the quantization of
> +	the subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
> +	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
> +	See :ref:`v4l2-mbus-format` on how to do this.
> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC
> +      - 0x00000008
> +      - The driver allows the application to try to change the default transform function.
> +	The application can ask to configure the transform function of
> +	the subdevice when calling the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`
> +	ioctl with :ref:`V4L2_MBUS_FRAMEFMT_SET_CSC <mbus-framefmt-set-csc>` set.
> +	See :ref:`v4l2-mbus-format` on how to do this.

Reorder and renumber the V4L2_SUBDEV_MBUS_CODE_CSC_ values.

> +
>  Return Value
>  ============
>  
> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> index 123a231001a8..3b7d692b4015 100644
> --- a/include/uapi/linux/v4l2-mediabus.h
> +++ b/include/uapi/linux/v4l2-mediabus.h
> @@ -16,6 +16,8 @@
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
>  
> +#define V4L2_MBUS_FRAMEFMT_SET_CSC	0x00000001
> +
>  /**
>   * struct v4l2_mbus_framefmt - frame format on the media bus
>   * @width:	image width
> @@ -26,6 +28,9 @@
>   * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> + * @reserved2:  two reserved bytes that can be later used
> + * @flags:	flags (V4L2_MBUS_FRAMEFMT_*)
> + * @reserved:  reserved bytes that can be later used
>   */
>  struct v4l2_mbus_framefmt {
>  	__u32			width;
> @@ -36,7 +41,9 @@ struct v4l2_mbus_framefmt {
>  	__u16			ycbcr_enc;
>  	__u16			quantization;
>  	__u16			xfer_func;
> -	__u16			reserved[11];
> +	__u16			reserved2;
> +	__u32			flags;
> +	__u16			reserved[8];
>  };
>  
>  #ifndef __KERNEL__
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 5d2a1dab7911..c20aa9a89864 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -65,19 +65,25 @@ struct v4l2_subdev_crop {
>  	__u32 reserved[8];
>  };
>  
> +#define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
> +#define V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC	0x00000002
> +#define V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION	0x00000004
> +#define V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC	0x00000008

Reorder and renumber.

Regards,

	Hans

>  /**
>   * struct v4l2_subdev_mbus_code_enum - Media bus format enumeration
>   * @pad: pad number, as reported by the media API
>   * @index: format index during enumeration
>   * @code: format code (MEDIA_BUS_FMT_ definitions)
>   * @which: format type (from enum v4l2_subdev_format_whence)
> + * @flags: flags set by the driver, (V4L2_SUBDEV_MBUS_CODE_*)
>   */
>  struct v4l2_subdev_mbus_code_enum {
>  	__u32 pad;
>  	__u32 index;
>  	__u32 code;
>  	__u32 which;
> -	__u32 reserved[8];
> +	__u32 flags;
> +	__u32 reserved[7];
>  };
>  
>  /**
> 

Re: [PATCH v5 5/7] media: v4l2: add support for the subdev CSC API for hsv_enc on mediabus

[Hans Verkuil]

On 03/07/2020 19:10, Dafna Hirschfeld wrote:
> Add the flag V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC that drivers
> can set when enumerating the supported mediabus formats
> on subdevices to indicate that the userspace can ask to
> set the 'hsv_enc'. The patch also replaces the 'ycbcr_enc'
> field in 'struct v4l2_mbus_framefmt' with a union that
> includes 'hsv_enc'

I would just squash this patch with the previous patch. There is really
no need to split this into two patches.

Regards,

	Hans

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/v4l/subdev-formats.rst              | 33 +++++++++++++++----
>  include/uapi/linux/v4l2-mediabus.h            |  8 ++++-
>  include/uapi/linux/v4l2-subdev.h              |  1 +
>  3 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> index 7362ee0b1e96..dddc38191547 100644
> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> @@ -51,7 +51,9 @@ Media Bus Formats
>  	The driver indicates that colorspace conversion is supported by setting
>  	the flag V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE in the corresponding struct
>  	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
> -	See :ref:`colorspaces`.
> +	See :ref:`v4l2-subdev-mbus-code-flags`.
> +    * - union {
> +      - (anonymous)
>      * - __u16
>        - ``ycbcr_enc``
>        - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
> @@ -67,7 +69,23 @@ Media Bus Formats
>  	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC in the corresponding struct
>  	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
>  	See :ref:`v4l2-subdev-mbus-code-flags`.
> -
> +    * - __u16
> +      - ``hsv_enc``
> +      - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
> +        This information supplements the ``colorspace`` and must be set by
> +	the driver for capture streams and by the application for output
> +	streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific HSV encoding
> +	for the media bus data. If the driver cannot handle requested
> +	conversion, it will return another supported encoding.
> +	This field is ignored for Y'CbCr media bus formats. The driver indicates
> +	that hsv_enc conversion is supported by setting the flag
> +	V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC in the corresponding struct
> +	:c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
> +	See :ref:`v4l2-subdev-mbus-code-flags`
> +    * - }
> +      -
>      * - __u16
>        - ``quantization``
>        - Quantization range, from enum :c:type:`v4l2_quantization`.
> @@ -123,11 +141,12 @@ Media Bus Formats
>  	ignored for output streams. If set, then request the subdevice to do
>  	colorspace conversion from the received colorspace to the requested
>  	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
> -	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry
> -	setting will remain unchanged from what was received. So to change the
> -	quantization, only the ``quantization`` field shall be set to non-zero values
> -	(``V4L2_QUANTIZATION_FULL_RANGE`` or ``V4L2_QUANTIZATION_LIM_RANGE``)
> -	and all other colorimetry fields shall be set to 0.
> +	''hsv_enc``, ``quantization`` or ``xfer_func``) is set to 0, then that
> +	colorimetry setting will remain unchanged from what was received. So to
> +	change the quantization, only the ``quantization`` field shall be set to
> +	non-zero values (``V4L2_QUANTIZATION_FULL_RANGE`` or
> +	``V4L2_QUANTIZATION_LIM_RANGE``) and all other colorimetry fields shall
> +	be set to 0.
>  
>  	To check which conversions are supported by the hardware for the current
>  	media bus frame format, see :ref:`v4l2-subdev-mbus-code-flags`.
> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> index 3b7d692b4015..d0bc8df18ad5 100644
> --- a/include/uapi/linux/v4l2-mediabus.h
> +++ b/include/uapi/linux/v4l2-mediabus.h
> @@ -26,6 +26,7 @@
>   * @field:	used interlacing type (from enum v4l2_field)
>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
> + * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
>   * @reserved2:  two reserved bytes that can be later used
> @@ -38,7 +39,12 @@ struct v4l2_mbus_framefmt {
>  	__u32			code;
>  	__u32			field;
>  	__u32			colorspace;
> -	__u16			ycbcr_enc;
> +	union {
> +		/* enum v4l2_ycbcr_encoding */
> +		__u16			ycbcr_enc;
> +		/* enum v4l2_hsv_encoding */
> +		__u16			hsv_enc;
> +	};
>  	__u16			quantization;
>  	__u16			xfer_func;
>  	__u16			reserved2;
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index c20aa9a89864..08e25a961d15 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -67,6 +67,7 @@ struct v4l2_subdev_crop {
>  
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC	0x00000002
> +#define V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC	V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION	0x00000004
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC	0x00000008
>  /**
> 

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Hans Verkuil]

On 03/07/2020 19:10, Dafna Hirschfeld wrote:
> The isp entity has a hardware support to force full range quantization

s/a hardware support/hardware support/

> for YUV formats. Use the subdev CSC API to allow userspace to set the
> quantization for YUV formats on the isp entity.
> 
> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on

s/on/for/

> YUV formats during enumeration of the isp formats on the source pad.
> - The full quantization is set on YUV formats if userspace ask it

s/ask/asks for/

> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> 
> On the capture and the resizer, the quantization is read-only
> and always set to the default quantization.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO             |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index c0cbec0a164d..db05288949bd 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -2,7 +2,7 @@
>  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
>  * Fix checkpatch errors.
>  * Review and comment every lock
> -* Handle quantization
> +* Add uapi docs. Remeber to add documentation of how quantization is handled.
>  * Document rkisp1-common.h
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index f69235f82c45..93d6846886f2 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  			   const struct v4l2_format_info **fmt_info)
>  {
>  	const struct rkisp1_capture_config *config = cap->config;
> -	struct rkisp1_capture *other_cap =
> -			&cap->rkisp1->capture_devs[cap->id ^ 1];
>  	const struct rkisp1_capture_fmt_cfg *fmt;
>  	const struct v4l2_format_info *info;
>  	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  
>  	info = rkisp1_fill_pixfmt(pixm, cap->id);
>  
> -	/* can not change quantization when stream-on */
> -	if (other_cap->is_streaming)
> -		pixm->quantization = other_cap->pix.fmt.quantization;
> -	/* output full range by default, take effect in params */
> -	else if (!pixm->quantization ||
> -		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> -		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -
>  	if (fmt_cfg)
>  		*fmt_cfg = fmt;
>  	if (fmt_info)
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 58c90c67594d..d575c1e4dc4b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
>  
>  		if (code->index == pos - 1) {
>  			code->code = fmt->mbus_code;
> +			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> +			    dir == RKISP1_ISP_SD_SRC)
> +				code->flags =
> +					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
>  			return 0;
>  		}
>  	}
> @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
>  					     RKISP1_ISP_PAD_SOURCE_VIDEO);
>  	*src_fmt = *sink_fmt;
>  	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> -	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  
>  	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
>  					    RKISP1_ISP_PAD_SOURCE_VIDEO);
> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>  		isp->src_fmt = mbus_info;
>  	src_fmt->width  = src_crop->width;
>  	src_fmt->height = src_crop->height;
> -	src_fmt->quantization = format->quantization;
> -	/* full range by default */
> -	if (!src_fmt->quantization)
> +
> +	/*
> +	 * The CSC API is used to allow userspace to force full
> +	 * quantization on YUV formats.
> +	 */
> +	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> +	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> +	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>  		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	else
> +		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>  
>  	*format = *src_fmt;
>  }
> 

Regards,

	Hans

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Tomasz Figa]

Hi Dafna,

On Fri, Jul 03, 2020 at 07:10:16PM +0200, Dafna Hirschfeld wrote:
> This patch extends the CSC API in video devices to be supported
> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
> The flags:
> 
> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
> 
> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
> 
> New 'flags' fields were added to the structs
> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
> from the 'reserved' field
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
>  .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
>  include/uapi/linux/v4l2-mediabus.h            |  9 ++-
>  include/uapi/linux/v4l2-subdev.h              |  8 +-
>  4 files changed, 129 insertions(+), 10 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> index 9a4d61b0d76f..7362ee0b1e96 100644
> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> @@ -41,32 +41,96 @@ Media Bus Formats
>  	:ref:`field-order` for details.
>      * - __u32
>        - ``colorspace``
> -      - Image colorspace, from enum
> -	:c:type:`v4l2_colorspace`. See
> -	:ref:`colorspaces` for details.
> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
> +        Must be set by the driver for capture streams and by the application
> +	for output streams, see :ref:`colorspaces`. If the application sets the
> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> +	this field for a capture stream to request a specific colorspace

What is a "capture stream" in terms of the subdev API? Should this
perhaps refer to "source pads" instead?

[snip]
> +.. _v4l2-mbus-framefmt-flags:
> +
> +.. flat-table:: v4l2_mbus_framefmt Flags
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * .. _`mbus-framefmt-set-csc`:
> +
> +      - ``V4L2_MBUS_FRAMEFMT_SET_CSC``
> +      - 0x00000001
> +      - Set by the application. It is only used for capture and is
> +	ignored for output streams. If set, then request the subdevice to do

Ditto.

> +	colorspace conversion from the received colorspace to the requested
> +	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,

nit: a colorimetry field

> +	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry

Is it okay to explicitly mention 0 here, rather than the defined
"_DEFAULT" values?

Best regards,
Tomasz

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Tomasz Figa]

On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote:
> Hi Dafna,
> 
> Thanks for your patch.
> 
> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote:
> > The isp entity has a hardware support to force full range quantization
> > for YUV formats. Use the subdev CSC API to allow userspace to set the
> > quantization for YUV formats on the isp entity.
> > 
> > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
> > YUV formats during enumeration of the isp formats on the source pad.
> > - The full quantization is set on YUV formats if userspace ask it
> > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> > 
> > On the capture and the resizer, the quantization is read-only
> > and always set to the default quantization.
> > 
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/TODO             |  2 +-
> >  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
> >  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
> >  3 files changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> > index c0cbec0a164d..db05288949bd 100644
> > --- a/drivers/staging/media/rkisp1/TODO
> > +++ b/drivers/staging/media/rkisp1/TODO
> > @@ -2,7 +2,7 @@
> >  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
> >  * Fix checkpatch errors.
> >  * Review and comment every lock
> > -* Handle quantization
> > +* Add uapi docs. Remeber to add documentation of how quantization is handled.
> >  * Document rkisp1-common.h
> >  * streaming paths (mainpath and selfpath) check if the other path is streaming
> >  in several places of the code, review this, specially that it doesn't seem it
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index f69235f82c45..93d6846886f2 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >  			   const struct v4l2_format_info **fmt_info)
> >  {
> >  	const struct rkisp1_capture_config *config = cap->config;
> > -	struct rkisp1_capture *other_cap =
> > -			&cap->rkisp1->capture_devs[cap->id ^ 1];
> >  	const struct rkisp1_capture_fmt_cfg *fmt;
> >  	const struct v4l2_format_info *info;
> >  	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >  
> >  	info = rkisp1_fill_pixfmt(pixm, cap->id);
> >  
> > -	/* can not change quantization when stream-on */
> > -	if (other_cap->is_streaming)
> > -		pixm->quantization = other_cap->pix.fmt.quantization;
> > -	/* output full range by default, take effect in params */
> > -	else if (!pixm->quantization ||
> > -		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> > -		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > -
> >  	if (fmt_cfg)
> >  		*fmt_cfg = fmt;
> >  	if (fmt_info)
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > index 58c90c67594d..d575c1e4dc4b 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
> >  
> >  		if (code->index == pos - 1) {
> >  			code->code = fmt->mbus_code;
> > +			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> > +			    dir == RKISP1_ISP_SD_SRC)
> > +				code->flags =
> > +					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
> >  			return 0;
> >  		}
> >  	}
> > @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
> >  					     RKISP1_ISP_PAD_SOURCE_VIDEO);
> >  	*src_fmt = *sink_fmt;
> >  	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> > -	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >  
> >  	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
> >  					    RKISP1_ISP_PAD_SOURCE_VIDEO);
> > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> >  		isp->src_fmt = mbus_info;
> >  	src_fmt->width  = src_crop->width;
> >  	src_fmt->height = src_crop->height;
> > -	src_fmt->quantization = format->quantization;
> > -	/* full range by default */
> > -	if (!src_fmt->quantization)
> > +
> > +	/*
> > +	 * The CSC API is used to allow userspace to force full
> > +	 * quantization on YUV formats.
> > +	 */
> > +	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> > +	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> > +	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
> >  		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	else
> > +		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> 
> According to the docs [1]:
> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization
> 
> "The default colorspace. This can be used by applications to let the driver fill in the colorspace."
> 
> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace.
> 

The part of the documentation you quoted comes from the description of
V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT.

For the latter it says:

"Use the default quantization encoding as defined by the colorspace.
 This is always full range for R’G’B’ (except for the BT.2020 colorspace)
 and HSV. It is usually limited range for Y’CbCr."

Which, in my understanding, doesn't suggest in any way that it shouldn't
be returned by the driver. There is also a valid case when the driver
simply doesn't know or care what quantization the captured signal uses.

However, in this case, I don't see why we would return DEFAULT here
indeed, as the driver has the full knowledge, RGB is full range and YUV
is limited range unless full range was requested.

> We have the same issue in the resizer.
> 
> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed.
> 
> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization,
> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes.
> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets
> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report
> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver.

This was extensively discussed at v3:
https://patchwork.kernel.org/patch/11493105/

The conclusion is that there is no value from both the kernel and the userspace point of
view in propagating this to the end of the pipeline, because the
hardware components later in the pipeline don't care and the userspace,
given the DEFAULT quantization value, can infer the exact quantziation from the
pipeline - here from what it configured at the ISP entity.

Best regards,
Tomasz

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Fri, Jul 03, 2020 at 07:10:18PM +0200, Dafna Hirschfeld wrote:
> The isp entity has a hardware support to force full range quantization
> for YUV formats. Use the subdev CSC API to allow userspace to set the
> quantization for YUV formats on the isp entity.
> 
> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
> YUV formats during enumeration of the isp formats on the source pad.
> - The full quantization is set on YUV formats if userspace ask it
> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> 
> On the capture and the resizer, the quantization is read-only
> and always set to the default quantization.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO             |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index c0cbec0a164d..db05288949bd 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -2,7 +2,7 @@
>  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
>  * Fix checkpatch errors.
>  * Review and comment every lock
> -* Handle quantization
> +* Add uapi docs. Remeber to add documentation of how quantization is handled.
>  * Document rkisp1-common.h
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index f69235f82c45..93d6846886f2 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  			   const struct v4l2_format_info **fmt_info)
>  {
>  	const struct rkisp1_capture_config *config = cap->config;
> -	struct rkisp1_capture *other_cap =
> -			&cap->rkisp1->capture_devs[cap->id ^ 1];
>  	const struct rkisp1_capture_fmt_cfg *fmt;
>  	const struct v4l2_format_info *info;
>  	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>  
>  	info = rkisp1_fill_pixfmt(pixm, cap->id);
>  
> -	/* can not change quantization when stream-on */
> -	if (other_cap->is_streaming)
> -		pixm->quantization = other_cap->pix.fmt.quantization;
> -	/* output full range by default, take effect in params */
> -	else if (!pixm->quantization ||
> -		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> -		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -
>  	if (fmt_cfg)
>  		*fmt_cfg = fmt;
>  	if (fmt_info)
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 58c90c67594d..d575c1e4dc4b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
>  
>  		if (code->index == pos - 1) {
>  			code->code = fmt->mbus_code;
> +			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> +			    dir == RKISP1_ISP_SD_SRC)
> +				code->flags =
> +					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
>  			return 0;
>  		}
>  	}
> @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
>  					     RKISP1_ISP_PAD_SOURCE_VIDEO);
>  	*src_fmt = *sink_fmt;
>  	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> -	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  
>  	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
>  					    RKISP1_ISP_PAD_SOURCE_VIDEO);
> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>  		isp->src_fmt = mbus_info;
>  	src_fmt->width  = src_crop->width;
>  	src_fmt->height = src_crop->height;
> -	src_fmt->quantization = format->quantization;
> -	/* full range by default */
> -	if (!src_fmt->quantization)
> +
> +	/*
> +	 * The CSC API is used to allow userspace to force full
> +	 * quantization on YUV formats.
> +	 */
> +	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> +	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> +	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>  		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	else
> +		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;

Apart from the usage of DEFAULT, as discussed by Helen and Tomasz, this
patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	*format = *src_fmt;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Helen Koike]

Hi,

On 7/22/20 10:12 AM, Tomasz Figa wrote:
> On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote:
>> Hi Dafna,
>>
>> Thanks for your patch.
>>
>> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote:
>>> The isp entity has a hardware support to force full range quantization
>>> for YUV formats. Use the subdev CSC API to allow userspace to set the
>>> quantization for YUV formats on the isp entity.
>>>
>>> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
>>> YUV formats during enumeration of the isp formats on the source pad.
>>> - The full quantization is set on YUV formats if userspace ask it
>>> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
>>>
>>> On the capture and the resizer, the quantization is read-only
>>> and always set to the default quantization.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>  drivers/staging/media/rkisp1/TODO             |  2 +-
>>>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
>>>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
>>>  3 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
>>> index c0cbec0a164d..db05288949bd 100644
>>> --- a/drivers/staging/media/rkisp1/TODO
>>> +++ b/drivers/staging/media/rkisp1/TODO
>>> @@ -2,7 +2,7 @@
>>>  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
>>>  * Fix checkpatch errors.
>>>  * Review and comment every lock
>>> -* Handle quantization
>>> +* Add uapi docs. Remeber to add documentation of how quantization is handled.
>>>  * Document rkisp1-common.h
>>>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>>>  in several places of the code, review this, specially that it doesn't seem it
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index f69235f82c45..93d6846886f2 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>>>  			   const struct v4l2_format_info **fmt_info)
>>>  {
>>>  	const struct rkisp1_capture_config *config = cap->config;
>>> -	struct rkisp1_capture *other_cap =
>>> -			&cap->rkisp1->capture_devs[cap->id ^ 1];
>>>  	const struct rkisp1_capture_fmt_cfg *fmt;
>>>  	const struct v4l2_format_info *info;
>>>  	const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
>>> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
>>>  
>>>  	info = rkisp1_fill_pixfmt(pixm, cap->id);
>>>  
>>> -	/* can not change quantization when stream-on */
>>> -	if (other_cap->is_streaming)
>>> -		pixm->quantization = other_cap->pix.fmt.quantization;
>>> -	/* output full range by default, take effect in params */
>>> -	else if (!pixm->quantization ||
>>> -		 pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
>>> -		pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> -
>>>  	if (fmt_cfg)
>>>  		*fmt_cfg = fmt;
>>>  	if (fmt_info)
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> index 58c90c67594d..d575c1e4dc4b 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>>> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
>>>  
>>>  		if (code->index == pos - 1) {
>>>  			code->code = fmt->mbus_code;
>>> +			if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
>>> +			    dir == RKISP1_ISP_SD_SRC)
>>> +				code->flags =
>>> +					V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
>>>  			return 0;
>>>  		}
>>>  	}
>>> @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
>>>  					     RKISP1_ISP_PAD_SOURCE_VIDEO);
>>>  	*src_fmt = *sink_fmt;
>>>  	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
>>> -	src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>>  
>>>  	src_crop = v4l2_subdev_get_try_crop(sd, cfg,
>>>  					    RKISP1_ISP_PAD_SOURCE_VIDEO);
>>> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>>>  		isp->src_fmt = mbus_info;
>>>  	src_fmt->width  = src_crop->width;
>>>  	src_fmt->height = src_crop->height;
>>> -	src_fmt->quantization = format->quantization;
>>> -	/* full range by default */
>>> -	if (!src_fmt->quantization)
>>> +
>>> +	/*
>>> +	 * The CSC API is used to allow userspace to force full
>>> +	 * quantization on YUV formats.
>>> +	 */
>>> +	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
>>> +	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
>>> +	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>>>  		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> +	else
>>> +		src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>>
>> According to the docs [1]:
>>
>> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization
>>
>> "The default colorspace. This can be used by applications to let the driver fill in the colorspace."
>>
>> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace.
>>
> 
> The part of the documentation you quoted comes from the description of
> V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT.
> 
> For the latter it says:
> 
> "Use the default quantization encoding as defined by the colorspace.
>  This is always full range for R’G’B’ (except for the BT.2020 colorspace)
>  and HSV. It is usually limited range for Y’CbCr."
> 
> Which, in my understanding, doesn't suggest in any way that it shouldn't
> be returned by the driver. There is also a valid case when the driver
> simply doesn't know or care what quantization the captured signal uses.

Thanks for this clarification.

> 
> However, in this case, I don't see why we would return DEFAULT here
> indeed, as the driver has the full knowledge, RGB is full range and YUV
> is limited range unless full range was requested.
> 
>> We have the same issue in the resizer.
>>
>> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed.
>>
>> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization,
>> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes.
>> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets
>> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report
>> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver.
> 
> This was extensively discussed at v3:
> https://patchwork.kernel.org/patch/11493105/
> 
> The conclusion is that there is no value from both the kernel and the userspace point of
> view in propagating this to the end of the pipeline, because the
> hardware components later in the pipeline don't care and the userspace,
> given the DEFAULT quantization value, can infer the exact quantziation from the
> pipeline - here from what it configured at the ISP entity.

Let's use DEFAULT then, sounds the simplest solution imho, unless you disagree.

Acked-by: Helen Koike <helen.koike@collabora.com>

Regards,
Helen

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v5 6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad

[Tomasz Figa]

On Wed, Jul 22, 2020 at 7:13 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi,
>
> On 7/22/20 10:12 AM, Tomasz Figa wrote:
> > On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote:
> >> Hi Dafna,
> >>
> >> Thanks for your patch.
> >>
> >> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote:
> >>> The isp entity has a hardware support to force full range quantization
> >>> for YUV formats. Use the subdev CSC API to allow userspace to set the
> >>> quantization for YUV formats on the isp entity.
> >>>
> >>> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on
> >>> YUV formats during enumeration of the isp formats on the source pad.
> >>> - The full quantization is set on YUV formats if userspace ask it
> >>> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC.
> >>>
> >>> On the capture and the resizer, the quantization is read-only
> >>> and always set to the default quantization.
> >>>
> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>> ---
> >>>  drivers/staging/media/rkisp1/TODO             |  2 +-
> >>>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ----------
> >>>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 18 ++++++++++++++----
> >>>  3 files changed, 15 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> >>> index c0cbec0a164d..db05288949bd 100644
> >>> --- a/drivers/staging/media/rkisp1/TODO
> >>> +++ b/drivers/staging/media/rkisp1/TODO
> >>> @@ -2,7 +2,7 @@
> >>>  * Use threaded interrupt for rkisp1_stats_isr(), remove work queue.
> >>>  * Fix checkpatch errors.
> >>>  * Review and comment every lock
> >>> -* Handle quantization
> >>> +* Add uapi docs. Remeber to add documentation of how quantization is handled.
> >>>  * Document rkisp1-common.h
> >>>  * streaming paths (mainpath and selfpath) check if the other path is streaming
> >>>  in several places of the code, review this, specially that it doesn't seem it
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> index f69235f82c45..93d6846886f2 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >>>                        const struct v4l2_format_info **fmt_info)
> >>>  {
> >>>     const struct rkisp1_capture_config *config = cap->config;
> >>> -   struct rkisp1_capture *other_cap =
> >>> -                   &cap->rkisp1->capture_devs[cap->id ^ 1];
> >>>     const struct rkisp1_capture_fmt_cfg *fmt;
> >>>     const struct v4l2_format_info *info;
> >>>     const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH,
> >>> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap,
> >>>
> >>>     info = rkisp1_fill_pixfmt(pixm, cap->id);
> >>>
> >>> -   /* can not change quantization when stream-on */
> >>> -   if (other_cap->is_streaming)
> >>> -           pixm->quantization = other_cap->pix.fmt.quantization;
> >>> -   /* output full range by default, take effect in params */
> >>> -   else if (!pixm->quantization ||
> >>> -            pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE)
> >>> -           pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >>> -
> >>>     if (fmt_cfg)
> >>>             *fmt_cfg = fmt;
> >>>     if (fmt_info)
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> >>> index 58c90c67594d..d575c1e4dc4b 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> >>> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
> >>>
> >>>             if (code->index == pos - 1) {
> >>>                     code->code = fmt->mbus_code;
> >>> +                   if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> >>> +                       dir == RKISP1_ISP_SD_SRC)
> >>> +                           code->flags =
> >>> +                                   V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
> >>>                     return 0;
> >>>             }
> >>>     }
> >>> @@ -620,7 +624,6 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
> >>>                                          RKISP1_ISP_PAD_SOURCE_VIDEO);
> >>>     *src_fmt = *sink_fmt;
> >>>     src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
> >>> -   src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >>>
> >>>     src_crop = v4l2_subdev_get_try_crop(sd, cfg,
> >>>                                         RKISP1_ISP_PAD_SOURCE_VIDEO);
> >>> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> >>>             isp->src_fmt = mbus_info;
> >>>     src_fmt->width  = src_crop->width;
> >>>     src_fmt->height = src_crop->height;
> >>> -   src_fmt->quantization = format->quantization;
> >>> -   /* full range by default */
> >>> -   if (!src_fmt->quantization)
> >>> +
> >>> +   /*
> >>> +    * The CSC API is used to allow userspace to force full
> >>> +    * quantization on YUV formats.
> >>> +    */
> >>> +   if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> >>> +       format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
> >>> +       mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
> >>>             src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >>> +   else
> >>> +           src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> >>
> >> According to the docs [1]:
> >>
> >> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization
> >>
> >> "The default colorspace. This can be used by applications to let the driver fill in the colorspace."
> >>
> >> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace.
> >>
> >
> > The part of the documentation you quoted comes from the description of
> > V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT.
> >
> > For the latter it says:
> >
> > "Use the default quantization encoding as defined by the colorspace.
> >  This is always full range for R’G’B’ (except for the BT.2020 colorspace)
> >  and HSV. It is usually limited range for Y’CbCr."
> >
> > Which, in my understanding, doesn't suggest in any way that it shouldn't
> > be returned by the driver. There is also a valid case when the driver
> > simply doesn't know or care what quantization the captured signal uses.
>
> Thanks for this clarification.
>
> >
> > However, in this case, I don't see why we would return DEFAULT here
> > indeed, as the driver has the full knowledge, RGB is full range and YUV
> > is limited range unless full range was requested.
> >
> >> We have the same issue in the resizer.
> >>
> >> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed.
> >>
> >> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization,
> >> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes.
> >> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets
> >> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report
> >> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver.
> >
> > This was extensively discussed at v3:
> > https://patchwork.kernel.org/patch/11493105/
> >
> > The conclusion is that there is no value from both the kernel and the userspace point of
> > view in propagating this to the end of the pipeline, because the
> > hardware components later in the pipeline don't care and the userspace,
> > given the DEFAULT quantization value, can infer the exact quantziation from the
> > pipeline - here from what it configured at the ISP entity.
>
> Let's use DEFAULT then, sounds the simplest solution imho, unless you disagree.
>
> Acked-by: Helen Koike <helen.koike@collabora.com>

Yes. Laurent convinced me that it is indeed the right behavior here.

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Dafna Hirschfeld]

Am 22.07.20 um 14:53 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Fri, Jul 03, 2020 at 07:10:16PM +0200, Dafna Hirschfeld wrote:
>> This patch extends the CSC API in video devices to be supported
>> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
>> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
>> The flags:
>>
>> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
>> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
>>
>> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
>>
>> New 'flags' fields were added to the structs
>> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
>> from the 'reserved' field
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
>>   .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
>>   include/uapi/linux/v4l2-mediabus.h            |  9 ++-
>>   include/uapi/linux/v4l2-subdev.h              |  8 +-
>>   4 files changed, 129 insertions(+), 10 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> index 9a4d61b0d76f..7362ee0b1e96 100644
>> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> @@ -41,32 +41,96 @@ Media Bus Formats
>>   	:ref:`field-order` for details.
>>       * - __u32
>>         - ``colorspace``
>> -      - Image colorspace, from enum
>> -	:c:type:`v4l2_colorspace`. See
>> -	:ref:`colorspaces` for details.
>> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>> +        Must be set by the driver for capture streams and by the application
>> +	for output streams, see :ref:`colorspaces`. If the application sets the
>> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
>> +	this field for a capture stream to request a specific colorspace
> 
> What is a "capture stream" in terms of the subdev API? Should this
> perhaps refer to "source pads" instead?

Hi, yes, I should change it to 'source pad'. I see that for the other colorimetry fields,
the docs for v4l2_mbus_framefmt already writes
"This information supplements the colorspace and must be set by the driver for capture streams and by the application for output streams,"
I guess this should also change,

Thanks,
Dafna

> 
> [snip]
>> +.. _v4l2-mbus-framefmt-flags:
>> +
>> +.. flat-table:: v4l2_mbus_framefmt Flags
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       3 1 4
>> +
>> +    * .. _`mbus-framefmt-set-csc`:
>> +
>> +      - ``V4L2_MBUS_FRAMEFMT_SET_CSC``
>> +      - 0x00000001
>> +      - Set by the application. It is only used for capture and is
>> +	ignored for output streams. If set, then request the subdevice to do
> 
> Ditto.
> 
>> +	colorspace conversion from the received colorspace to the requested
>> +	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
> 
> nit: a colorimetry field
> 
>> +	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry
> 
> Is it okay to explicitly mention 0 here, rather than the defined
> "_DEFAULT" values?
> 
> Best regards,
> Tomasz
> 

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Dafna Hirschfeld]

Am 22.07.20 um 14:53 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Fri, Jul 03, 2020 at 07:10:16PM +0200, Dafna Hirschfeld wrote:
>> This patch extends the CSC API in video devices to be supported
>> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
>> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
>> The flags:
>>
>> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
>> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
>>
>> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
>>
>> New 'flags' fields were added to the structs
>> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
>> from the 'reserved' field
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
>>   .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
>>   include/uapi/linux/v4l2-mediabus.h            |  9 ++-
>>   include/uapi/linux/v4l2-subdev.h              |  8 +-
>>   4 files changed, 129 insertions(+), 10 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> index 9a4d61b0d76f..7362ee0b1e96 100644
>> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>> @@ -41,32 +41,96 @@ Media Bus Formats
>>   	:ref:`field-order` for details.
>>       * - __u32
>>         - ``colorspace``
>> -      - Image colorspace, from enum
>> -	:c:type:`v4l2_colorspace`. See
>> -	:ref:`colorspaces` for details.
>> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>> +        Must be set by the driver for capture streams and by the application
>> +	for output streams, see :ref:`colorspaces`. If the application sets the
>> +	flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
>> +	this field for a capture stream to request a specific colorspace
> 
> What is a "capture stream" in terms of the subdev API? Should this
> perhaps refer to "source pads" instead?

Hi, yes, I should change it to 'source pad'. I see that for the other colorimetry fields,
the docs for v4l2_mbus_framefmt already writes
"This information supplements the colorspace and must be set by the driver for capture streams and by the application for output streams,"
I guess this should also change,

Thanks,
Dafna

> 
> [snip]
>> +.. _v4l2-mbus-framefmt-flags:
>> +
>> +.. flat-table:: v4l2_mbus_framefmt Flags
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       3 1 4
>> +
>> +    * .. _`mbus-framefmt-set-csc`:
>> +
>> +      - ``V4L2_MBUS_FRAMEFMT_SET_CSC``
>> +      - 0x00000001
>> +      - Set by the application. It is only used for capture and is
>> +	ignored for output streams. If set, then request the subdevice to do
> 
> Ditto.
> 
>> +	colorspace conversion from the received colorspace to the requested
>> +	colorspace values. If colorimetry field (``colorspace``, ``ycbcr_enc``,
> 
> nit: a colorimetry field
> 
>> +	``quantization`` or ``xfer_func``) is set to 0, then that colorimetry
> 
> Is it okay to explicitly mention 0 here, rather than the defined
> "_DEFAULT" values?
> 
> Best regards,
> Tomasz
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Tomasz Figa]

On Mon, Aug 17, 2020 at 12:24 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> Am 22.07.20 um 14:53 schrieb Tomasz Figa:
> > Hi Dafna,
> >
> > On Fri, Jul 03, 2020 at 07:10:16PM +0200, Dafna Hirschfeld wrote:
> >> This patch extends the CSC API in video devices to be supported
> >> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
> >> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
> >> The flags:
> >>
> >> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
> >> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
> >>
> >> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
> >>
> >> New 'flags' fields were added to the structs
> >> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
> >> from the 'reserved' field
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
> >>   .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
> >>   include/uapi/linux/v4l2-mediabus.h            |  9 ++-
> >>   include/uapi/linux/v4l2-subdev.h              |  8 +-
> >>   4 files changed, 129 insertions(+), 10 deletions(-)
> >>
> >
> > Thank you for the patch. Please see my comments inline.
> >
> >> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> index 9a4d61b0d76f..7362ee0b1e96 100644
> >> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> @@ -41,32 +41,96 @@ Media Bus Formats
> >>      :ref:`field-order` for details.
> >>       * - __u32
> >>         - ``colorspace``
> >> -      - Image colorspace, from enum
> >> -    :c:type:`v4l2_colorspace`. See
> >> -    :ref:`colorspaces` for details.
> >> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
> >> +        Must be set by the driver for capture streams and by the application
> >> +    for output streams, see :ref:`colorspaces`. If the application sets the
> >> +    flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> >> +    this field for a capture stream to request a specific colorspace
> >
> > What is a "capture stream" in terms of the subdev API? Should this
> > perhaps refer to "source pads" instead?
>
> Hi, yes, I should change it to 'source pad'. I see that for the other colorimetry fields,
> the docs for v4l2_mbus_framefmt already writes
> "This information supplements the colorspace and must be set by the driver for capture streams and by the application for output streams,"
> I guess this should also change,

Right.

Best regards,
Tomasz

Re: [PATCH v5 4/7] v4l2: extend the CSC API to subdevice.

[Tomasz Figa]

On Mon, Aug 17, 2020 at 12:24 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> Am 22.07.20 um 14:53 schrieb Tomasz Figa:
> > Hi Dafna,
> >
> > On Fri, Jul 03, 2020 at 07:10:16PM +0200, Dafna Hirschfeld wrote:
> >> This patch extends the CSC API in video devices to be supported
> >> also on sub-devices. The flag V4L2_MBUS_FRAMEFMT_SET_CSC set by
> >> the application when calling VIDIOC_SUBDEV_S_FMT ioctl.
> >> The flags:
> >>
> >> V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE, V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC,
> >> V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC,
> >>
> >> are set by the driver in the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl.
> >>
> >> New 'flags' fields were added to the structs
> >> v4l2_subdev_mbus_code_enum, v4l2_mbus_framefmt which are borrowed
> >> from the 'reserved' field
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   .../media/v4l/subdev-formats.rst              | 78 +++++++++++++++++--
> >>   .../v4l/vidioc-subdev-enum-mbus-code.rst      | 44 ++++++++++-
> >>   include/uapi/linux/v4l2-mediabus.h            |  9 ++-
> >>   include/uapi/linux/v4l2-subdev.h              |  8 +-
> >>   4 files changed, 129 insertions(+), 10 deletions(-)
> >>
> >
> > Thank you for the patch. Please see my comments inline.
> >
> >> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> index 9a4d61b0d76f..7362ee0b1e96 100644
> >> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> @@ -41,32 +41,96 @@ Media Bus Formats
> >>      :ref:`field-order` for details.
> >>       * - __u32
> >>         - ``colorspace``
> >> -      - Image colorspace, from enum
> >> -    :c:type:`v4l2_colorspace`. See
> >> -    :ref:`colorspaces` for details.
> >> +      - Image colorspace, from enum :c:type:`v4l2_colorspace`.
> >> +        Must be set by the driver for capture streams and by the application
> >> +    for output streams, see :ref:`colorspaces`. If the application sets the
> >> +    flag ``V4L2_MBUS_FRAMEFMT_SET_CSC`` then the application can set
> >> +    this field for a capture stream to request a specific colorspace
> >
> > What is a "capture stream" in terms of the subdev API? Should this
> > perhaps refer to "source pads" instead?
>
> Hi, yes, I should change it to 'source pad'. I see that for the other colorimetry fields,
> the docs for v4l2_mbus_framefmt already writes
> "This information supplements the colorspace and must be set by the driver for capture streams and by the application for output streams,"
> I guess this should also change,

Right.

Best regards,
Tomasz

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip