Re: [RFC v3 1/2] v4l2: add support for colorspace conversion for video capture

[Hans Verkuil <hverkuil@xs4all.nl>]

On 25/05/2020 15:56, Dafna Hirschfeld wrote:
> Hi
> 
> On 07.05.20 15:00, Hans Verkuil wrote:
>> On 16/04/2020 16:56, Dafna Hirschfeld wrote:
>>> From: Philipp Zabel <p.zabel@pengutronix.de>
>>>
>>> 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 pix_format flag:
>>> V4L2_PIX_FMT_FLAG_HAS_CSC. The flag is set by the application,
>>
>> That should be V4L2_PIX_FMT_FLAG_REQUEST_CSC, since the application actively
>> requests the use of the CSC.
> Maybe change to V4L2_PIX_FMT_FLAG_ASK_FOR_CSC so people won't think
> it is related to the request API ?

How about _SET_CSC? I think that's even better than REQUEST_CSC.

>>
>>> the driver will interpret the ycbcr_enc/hsv_enc, and quantization fields
>>> as the requested colorspace information and will attempt to
>>> do the conversion it supports.
>>>
>>> Drivers set the flags
>>> V4L2_FMT_FLAG_CSC_YCBCR_ENC,
>>> V4L2_FMT_FLAG_CSC_HSV_ENC,
>>> V4L2_FMT_FLAG_CSC_HSV_QUANTIZATION,
>>
>> That last define should be 'V4L2_FMT_FLAG_CSC_QUANTIZATION' without 'HSV_'.
>>
>>> in the flags field of the struct v4l2_fmtdesc during enumeration to
>>> indicate that they support colorspace conversion for the respective field.
>>> Currently the conversion of the fields colorspace, xfer_func is not
>>> supported since there are no drivers that support it.
>>>
>>> The same API is added for the subdevices. With the flag V4L2_MBUS_FRAMEFMT_HAS_CSC
>>
>> Should also be REQUEST_CSC.
>>
>>> set by the application in the VIDIOC_SUBDEV_S_FMT ioctl and the flags
>>> V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC, V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION
>>> 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
>>>
>>> Drivers do not have to actually look at the flags: if the flags are not
>>> set, then the colorspace, ycbcr_enc and quantization fields 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>
>>> ---
>>> This is v3 of the RFC suggested originaly by Hans Verkuil:
>>>
>>>   https://patchwork.linuxtv.org/patch/28847/
>>>
>>> And then a v2 from Philipp Zabel:
>>>
>>>   https://patchwork.kernel.org/project/linux-media/list/?series=15483
>>>
>>> changes in v3:
>>> I added the API to subdevices as well and added fixes according to
>>> comments from Hans.
>>> I also added a usecase for the new API for the rkisp1 driver.
>>>
>>> changes in v2 (reported by Philipp Zabel):
>>>   - convert to rst
>>>   - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for
>>>     colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func
>>>   - let driver set flags to indicate supported features
>>>
>>> [1] https://patchwork.linuxtv.org/patch/28847/
>>>
>>>   .../media/v4l/pixfmt-reserved.rst             |  6 +++
>>>   .../media/v4l/pixfmt-v4l2-mplane.rst          | 16 ++-----
>>>   .../userspace-api/media/v4l/pixfmt-v4l2.rst   | 36 +++++++++++++---
>>>   .../media/v4l/subdev-formats.rst              | 42 +++++++++++++++++--
>>>   .../media/v4l/vidioc-enum-fmt.rst             | 18 ++++++++
>>>   .../v4l/vidioc-subdev-enum-mbus-code.rst      | 23 ++++++++++
>>>   drivers/media/v4l2-core/v4l2-ioctl.c          | 19 ++++++++-
>>>   drivers/media/v4l2-core/v4l2-subdev.c         |  7 ++++
>>>   include/uapi/linux/v4l2-mediabus.h            |  5 ++-
>>>   include/uapi/linux/v4l2-subdev.h              |  5 ++-
>>>   include/uapi/linux/videodev2.h                |  4 ++
>>>   11 files changed, 158 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst b/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
>>> index 59b9e7238f90..fa8dada69f8c 100644
>>> --- a/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt-reserved.rst
>>
>> Why on earth did the documentation of these pixel format flags end up in
>> pixfmt-reserved.rst?
>>
>> Can you make a patch that moves this to pixfmt-v4l2.rst?
>>
>>> @@ -280,3 +280,9 @@ please make a proposal on the linux-media mailing list.
>>>   	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_HAS_CSC``
>>> +      - 0x00000002
>>> +      - Set by the application. It is only used for capture and is
>>> +	ignored for output streams. If set, then request the driver to do
>>> +	colorspace conversion from the received colorspace, only conversions
>>> +	of Ycbcr encoding, HSV encoding and quantization are supported.
>>
>> I'd replace the part "If set..." with this:
>>
>> ----------
>> If set, then request the driver to do colorspace conversion from the received
>> colorspace to the requested colorspace values. If a colorimetry field
>> (``colorspace``, ``xfer_func``, ``ycncr_enc`` or ``quantization``) is set to 0,
>> then that colorimetry setting will remain unchanged from what was received.
>> So to just change the quantization only the ``quantization`` field shall be set
>> to a non-zero value (``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/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 759420a872d6..ce57718cd66b 100644
>>> --- a/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt-v4l2.rst
>>> @@ -110,8 +110,8 @@ Single-planar format structure
>>>         - ``colorspace``
>>>         - 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`.
>>> +	by the driver for capture streams and by the application for output
>>> +	streams, see :ref:`colorspace`.
>>
>> This doesn't appear to change anything.
>>
>>>       * - __u32
>>>         - ``priv``
>>>         - This field indicates whether the remaining fields of the
>>> @@ -148,13 +148,31 @@ 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_HAS_CSC`` then the application can set
>>> +	this field for a capture stream to request a specific Y'CbCr encoding
>>> +	for the captured image data. The driver will attempt to do the
>>> +	conversion to the specified Y'CbCr encoding or return the encoding it
>>> +	will use if it can't do the conversion. This field is ignored for
>>> +	non-Y'CbCr pixelformats. The driver indicates that ycbcr_enc conversion
>>> +	is supported by setting the flag V4L2_FMT_FLAG_CSC_YCBCR_ENC on the
>>> +	on the corresponding struct :c:type:`v4l2_fmtdesc` during enumeration.
>>
>> on the on the -> in the
>>
>>> +	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_HAS_CSC`` then the application can set this
>>> +	field for a capture stream to request a specific HSV encoding for the
>>> +	captured image data. The driver will attempt to do the conversion to
>>> +	the specified HSV encoding or return the encoding it will use if it
>>> +	can't do the conversion. 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 on the
>>> +	on the corresponding struct :c:type:`v4l2_fmtdesc` during enumeration.
>>
>> Ditto.
>>
>>> +	See :ref:`fmtdesc-flags`
>>>       * - }
>>>         -
>>>       * - __u32
>>> @@ -162,7 +180,15 @@ 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_HAS_CSC`` then the application can set
>>> +	this field for a capture stream to request a specific quantization
>>> +	range for the captured image data. The driver will attempt to do the
>>> +	conversion to the specified quantization range or return the
>>> +	quantization it will use if it can't do the conversion.  The driver
>>> +	indicates that quantization conversion is supported by setting the flag
>>> +	V4L2_FMT_FLAG_CSC_QUANTIZATION on the on the corresponding struct
>>
>> Ditto
>>
>>> +	:c:type:`v4l2_fmtdesc` during enumeration. See :ref:`fmtdesc-flags`
>>>       * - __u32
>>>         - ``xfer_func``
>>>         - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>>> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>>> index 9a4d61b0d76f..f1d4ca29a3e8 100644
>>> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
>>> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
>>> @@ -49,13 +49,33 @@ Media Bus Formats
>>>         - 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_HAS_CSC`` the the application can set
>>> +	this field for a capture stream to request a specific Y'CbCr encoding
>>> +	for the mbus data. The driver will attempt to do the
>>
>> mbus -> media bus
>>
>>> +	conversion to the specified Y'CbCr encoding or return the encoding it
>>> +	will use if it can't do the conversion. This field is ignored for
>>> +	non-Y'CbCr pixelformats. The driver indicates that ycbcr_enc conversion
>>
>> This is a media bus format, not a pixelformat.
>>
>>> +	is supported by setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC on
>>
>> on -> in
>>
>>> +	the corresponding struct c:type:`v4l2_subdev_mbus_code_enum` during
>>> +	enumeration. See :ref:`vidioc-subdev-enum-mbus-code.rst <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_HAS_CSC`` the the application can set
>>> +	this field for a capture stream to request a specific quantization
>>> +	encoding for the mbus data. The driver will attempt to do the
>>
>> mbus -> media bus
>>
>>> +	conversion to the specified quantization or return the quantization it
>>> +	will use if it can't do the conversion. The driver indicates that
>>> +	quantization conversion is supported by setting the flag
>>> +	V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION on the corresponding struct
>>
>> on -> in
>>
>>> +	c:type:`v4l2_subdev_mbus_code_enum` during enumeration.
>>> +	See :ref:`vidioc-subdev-enum-mbus-code.rst <v4l2-subdev-mbus-code-flags>`
>>> +
>>>       * - __u16
>>>         - ``xfer_func``
>>>         - Transfer function, from enum :c:type:`v4l2_xfer_func`.
>>> @@ -63,10 +83,26 @@ Media Bus Formats
>>>   	the driver for capture streams and by the application for output
>>>   	streams, see :ref:`colorspaces`.
>>>       * - __u16
>>> -      - ``reserved``\ [11]
>>> +      - ``flags``
>>> +      - flags see:  :ref:v4l2-mbus-framefmt-flags
>>
>> flags see: -> See:
>>
>>> +    * - __u16
>>> +      - ``reserved``\ [10]
>>>         - 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
>>> +
>>> +    * - ``V4L2_MBUS_FRAMEFMT_HAS_CSC``
>>> +      - 0x0001
>>> +      - 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, only conversions
>>> +	of Ycbcr encoding, and quantization are supported.
>>
>> Replace by a similar text as for V4L2_PIX_FMT_FLAG_HAS_CSC.
>>
>>>   
>>>   
>>>   .. _v4l2-mbus-pixelcode:
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> index 7e3142e11d77..125f074543af 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> @@ -145,6 +145,24 @@ formats in preference order, where preferred formats are returned before
>>>   	parameters are detected. This flag can only be used in combination
>>>   	with the ``V4L2_FMT_FLAG_COMPRESSED`` flag, since this applies to
>>>   	compressed formats only. It is also only applies to stateful codecs.
>>> +    * - ``V4L2_FMT_FLAG_CSC_YCBCR_ENC``
>>> +      - 0x0010
>>> +      - The driver allows the application to try to change the default
>>> +	ycbcr 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 :c:func:`VIDIOC_S_FMT` ioctl.
>>
>> add: ...ioctl with ``V4L2_FMT_FLAG_REQUEST_CSC`` set. (this should probably be
>> a reference to the REQUEST_CSC flag documentation).
>>
>>> +    * - ``V4L2_FMT_FLAG_CSC_HSV_ENC``
>>> +      - 0x0010
>>> +      - 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 :c:func:`VIDIOC_S_FMT` ioctl.
>>
>> Ditto. Also: hsv -> HSV.
>>
>>> +    * - ``V4L2_FMT_FLAG_CSC_QUANTIZATION``
>>> +      - 0x0020
>>> +      - 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 :c:func:`VIDIOC_S_FMT` ioctl.
>>
>> Ditto.
>>
>>>   
>>>   
>>>   Return Value
>>> 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..4ad87cb74f57 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
>>> @@ -78,11 +78,34 @@ information about the try formats.
>>>         - ``which``
>>>         - Media bus format codes to be enumerated, from enum
>>>   	:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>>> +    * - __u32
>>> +      - ``flags``
>>> +      - see :ref:`v4l2-subdev-mbus-code-flags`
>>
>> see -> See
>>
>>>       * - __u32
>>>         - ``reserved``\ [8]
>>>         - Reserved for future extensions. Applications and drivers must set
>>>   	the array to zero.
>>>   
>>> +.. _v4l2-subdev-mbus-code-flags:
>>> +
>>> +
>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{7.7cm}|
>>> +
>>> +.. flat-table:: flags in struct v4l2_subdev_mbus_code_enum
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +    :widths:       1 1 2
>>> +
>>> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
>>> +      - 0x00000001
>>> +      - The driver supports setting the ycbcr encoding by the application
>>> +	when calling the VIDIOC_SUBDEV_S_FMT ioctl. see :ref:`v4l2-mbus-format`
>>
>> see -> See
>>
>>> +	on how to do this.
>>> +    * - V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION
>>> +      - 0x00000002
>>> +      - The driver supports setting the quantization by the application
>>> +	when calling the VIDIOC_SUBDEV_S_FMT ioctl. see :ref:`v4l2-mbus-format`
>>
>> see -> See
>>
>>> +	on how to do this.
>>>   
>>>   Return Value
>>>   ============
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index b2ef8e60ea7d..3c7ffb6b15cb 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1029,6 +1029,15 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
>>>   		fmt->fmt.pix_mp.num_planes = min_t(u32, fmt->fmt.pix_mp.num_planes,
>>>   					       VIDEO_MAX_PLANES);
>>>   
>>> +	if (fmt->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>>> +		if (fmt->fmt.pix_mp.flags & V4L2_PIX_FMT_FLAG_HAS_CSC)
>>> +			return;
>>> +		fmt->fmt.pix_mp.colorspace = V4L2_COLORSPACE_DEFAULT;
>>> +		fmt->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>> +		fmt->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
>>> +		fmt->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>> +	}
>>> +
>>>   	/*
>>>   	 * The v4l2_pix_format structure has been extended with fields that were
>>>   	 * not previously required to be set to zero by applications. The priv
>>> @@ -1043,8 +1052,16 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
>>>   	    fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>>>   		return;
>>>   
>>> -	if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC)
>>> +	if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC) {
>>> +		if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>>> +		    fmt->fmt.pix.flags & V4L2_PIX_FMT_FLAG_HAS_CSC)
>>> +			return;
>>> +		fmt->fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT;
>>> +		fmt->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>> +		fmt->fmt.pix.quantization = V4L2_QUANTIZATION_DEFAULT;
>>> +		fmt->fmt.pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>   		return;
>>> +	}
>>>   
>>>   	fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
>>>   
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index a376b351135f..51e009936aad 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -477,6 +477,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>   	case VIDIOC_SUBDEV_S_FMT: {
>>>   		struct v4l2_subdev_format *format = arg;
>>>   
>>> +		if (!(format->format.flags & V4L2_MBUS_FRAMEFMT_HAS_CSC)) {
>>> +			format->format.colorspace = V4L2_COLORSPACE_DEFAULT;
>>> +			format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>> +			format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>> +			format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
>>> +		}
>>> +
>>>   		memset(format->reserved, 0, sizeof(format->reserved));
>>>   		memset(format->format.reserved, 0, sizeof(format->format.reserved));
>>>   		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
>>> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
>>> index 123a231001a8..89ff0ad6a631 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_HAS_CSC	0x0001
>>> +
>>>   /**
>>>    * struct v4l2_mbus_framefmt - frame format on the media bus
>>>    * @width:	image width
>>> @@ -36,7 +38,8 @@ struct v4l2_mbus_framefmt {
>>>   	__u16			ycbcr_enc;
>>>   	__u16			quantization;
>>>   	__u16			xfer_func;
>>> -	__u16			reserved[11];
>>> +	__u16			flags;
>>> +	__u16			reserved[10];
>>>   };
>>>   
>>>   #ifndef __KERNEL__
>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>> index 03970ce30741..4410b26a7158 100644
>>> --- a/include/uapi/linux/v4l2-subdev.h
>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>> @@ -65,6 +65,8 @@ struct v4l2_subdev_crop {
>>>   	__u32 reserved[8];
>>>   };
>>>   
>>> +#define V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC	0x00000001
>>
>> I think it is better to add an HSV_ENC alias here as well.
>>
>> It's for readability only, since talking about YCBCR_ENC when the mediabus format
>> is a HSV format is just weird.
> 
> I see that in 'v4l2_pix_format' the {ycbcr_enc, hsv_enc} are a union
> but in 'v4l2_mbus_framefmt' there is only the field ycbcr_enc.
> I wonder why is that, since I see there is one HSV media bus format
> V4L2_MBUS_FROM_MEDIA_BUS_FMT(AHSV8888_1X32).
> 
> So I guess this is why I didn't add a V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC define.
> I also didn't add this flag to the docs. Should I?

I think this can be done in a separate (final) patch where a union is added for
v4l2_mbus_framefmt together with a V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC define.

These are effectively aliases to make code that works with HSV easier to read,
so they do not change functionality.

Regards,

	Hans

> 
> Thanks,
> Dafna
> 
>>
>>> +#define V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION	0x00000002
>>>   /**
>>>    * struct v4l2_subdev_mbus_code_enum - Media bus format enumeration
>>>    * @pad: pad number, as reported by the media API
>>> @@ -77,7 +79,8 @@ struct v4l2_subdev_mbus_code_enum {
>>>   	__u32 index;
>>>   	__u32 code;
>>>   	__u32 which;
>>> -	__u32 reserved[8];
>>> +	__u32 flags;
>>> +	__u32 reserved[7];
>>>   };
>>>   
>>>   /**
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 9817b7e2c968..adc9dd1080b8 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -772,6 +772,7 @@ struct v4l2_pix_format {
>>>   
>>>   /* Flags */
>>>   #define V4L2_PIX_FMT_FLAG_PREMUL_ALPHA	0x00000001
>>> +#define V4L2_PIX_FMT_FLAG_HAS_CSC	0x00000002
>>>   
>>>   /*
>>>    *	F O R M A T   E N U M E R A T I O N
>>> @@ -789,6 +790,9 @@ struct v4l2_fmtdesc {
>>>   #define V4L2_FMT_FLAG_EMULATED			0x0002
>>>   #define V4L2_FMT_FLAG_CONTINUOUS_BYTESTREAM	0x0004
>>>   #define V4L2_FMT_FLAG_DYN_RESOLUTION		0x0008
>>> +#define V4L2_FMT_FLAG_CSC_YCBCR_ENC		0x0010
>>> +#define V4L2_FMT_FLAG_CSC_HSV_ENC		0x0010
>>> +#define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0020
>>>   
>>>   	/* Frame Size and frame rate enumeration */
>>>   /*
>>>
>>
>> I think this is quite good, it's mainly doc improvements and renaming HAS_CSC to REQUEST_CSC.
>>
>> That said, I do want to see this supported in at least vivid (vimc would be nice
>> too!). And v4l2-ctl needs a patch to support this.
>>
>> A compliance test for v4l2-compliance would be nice too, but it is not a prerequisite.
>> I'm not entirely sure what would be the best way to test this. Perhaps a vivid-specific
>> test might be easiest (i.e., don't test it for all drivers, but just for vivid).
>>
>> Regards,
>>
>> 	Hans
>>