Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 08, 2019 at 03:03:34PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 8, 2019 8:08 PM
> >To: Sharma, Shashank <shashank.sharma@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Syrjala, Ville <ville.syrjala@intel.com>; dri-devel@lists.freedesktop.org; Lankhorst,
> >Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace property
> >
> >On Fri, Feb 08, 2019 at 07:36:24PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >> On 2/8/2019 7:00 PM, Ville Syrjälä wrote:
> >> > On Fri, Feb 08, 2019 at 06:36:39PM +0530, Sharma, Shashank wrote:
> >> >> Regards
> >> >>
> >> >> Shashank
> >> >>
> >> >> On 2/8/2019 6:22 PM, Ville Syrjälä wrote:
> >> >>> On Fri, Feb 08, 2019 at 12:36:25PM +0000, Sharma, Shashank wrote:
> >> >>>> Regards
> >> >>>> Shashank
> >> >>>>
> >> >>>>> -----Original Message-----
> >> >>>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org]
> >> >>>>> On Behalf Of Shankar, Uma
> >> >>>>> Sent: Friday, February 8, 2019 5:45 PM
> >> >>>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >>>>> Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville
> >> >>>>> <ville.syrjala@intel.com>; dri- devel@lists.freedesktop.org;
> >> >>>>> Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >> >>>>> Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace
> >> >>>>> property
> >> >>>>>
> >> >>>>>>>>>> -----Original Message-----
> >> >>>>>>>>>> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >>>>>>>>>> Sent: Tuesday, February 5, 2019 10:02 PM
> >> >>>>>>>>>> To: Shankar, Uma <uma.shankar@intel.com>
> >> >>>>>>>>>> Cc: intel-gfx@lists.freedesktop.org;
> >> >>>>>>>>>> dri-devel@lists.freedesktop.org; Syrjala, Ville
> >> >>>>>>>>>> <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >>>>>>>>>> <maarten.lankhorst@intel.com>
> >> >>>>>>>>>> Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace
> >> >>>>>>>>>> property
> >> >>>>>>>>>>
> >> >>>>>>>>>> On Tue, Feb 05, 2019 at 09:33:34PM +0530, Uma Shankar wrote:
> >> >>>>>>>>>>> Create a new connector property to program colorspace to sink
> >devices.
> >> >>>>>>>>>>> Modern sink devices support more than 1 type of colorspace
> >> >>>>>>>>>>> like 601, 709, BT2020 etc. This helps to switch based on
> >> >>>>>>>>>>> content type which is to be displayed. The decision lies
> >> >>>>>>>>>>> with compositors as to in which scenarios, a particular colorspace will
> >be picked.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> This will be helpful mostly to switch to higher gamut
> >> >>>>>>>>>>> colorspaces like
> >> >>>>>>>>>>> BT2020 when the media content is encoded as BT2020.
> >> >>>>>>>>>>> Thereby giving a good visual experience to users.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> The expectation from userspace is that it should parse the
> >> >>>>>>>>>>> EDID and get supported colorspaces. Use this property and
> >> >>>>>>>>>>> switch to the one supported. Sink supported colorspaces
> >> >>>>>>>>>>> should be retrieved by userspace from EDID and driver will
> >> >>>>>>>>>>> not explicitly expose
> >> >>>>>> them.
> >> >>>>>>>>>>> Basically the expectation from userspace is:
> >> >>>>>>>>>>>    - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
> >> >>>>>>>>>>>      colorspace
> >> >>>>>>>>>>>    - Set this new property to let the sink know what it
> >> >>>>>>>>>>>      converted the CRTC output to.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v2: Addressed Maarten and Ville's review comments.
> >> >>>>>>>>>>> Enhanced the colorspace enum to incorporate both HDMI and
> >> >>>>>>>>>>> DP supported
> >> >>>>>> colorspaces.
> >> >>>>>>>>>>> Also, added a default option for colorspace.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v3: Removed Adobe references from enum definitions as per
> >> >>>>>>>>>>> Ville, Hans Verkuil and Jonas Karlman suggestions. Changed
> >> >>>>>>>>>>> Default to an unset state where driver will assign the
> >> >>>>>>>>>>> colorspace is not chosen by user, suggested by Ville and
> >> >>>>>>>>>>> Maarten. Addressed other misc review comments from Maarten.
> >> >>>>>>>>>>> Split the changes to have separate colorspace property for DP and
> >HDMI.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v4: Addressed Chris and Ville's review comments, and
> >> >>>>>>>>>>> created a common colorspace property for DP and HDMI,
> >> >>>>>>>>>>> filtered the list based on the colorspaces supported by the respective
> >protocol standard.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v5: Made the property creation helper accept enum list
> >> >>>>>>>>>>> based on platform capabilties as suggested by Shashank.
> >> >>>>>>>>>>> Consolidated HDMI and DP property creation in the common helper.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v6: Addressed Shashank's review comments.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v7: Added defines instead of enum in uapi as per Brian
> >> >>>>>>>>>>> Starkey's suggestion in order to go with string matching at userspace.
> >> >>>>>>>>>>> Updated the commit message to add more details as well kernel docs.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v8: Addressed Maarten's review comments.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v9: Removed macro defines from uapi as per Brian Starkey
> >> >>>>>>>>>>> and Daniel Stone's comments and moved to drm include file.
> >> >>>>>>>>>>> Moved back to older design with exposing all HDMI
> >> >>>>>>>>>>> colorspaces to userspace since infoframe capability is
> >> >>>>>>>>>>> there even on legacy platforms, as per Ville's review comments.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v10: Fixed sparse warnings, updated the RB from Maarten and Jani's
> >ack.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> v11: Addressed Ville's review comments. Updated the Macro
> >> >>>>>>>>>>> naming and added DCI-P3 colorspace as well defined in CEA 861.G
> >spec.
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> >>>>>>>>>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
> >> >>>>>>>>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> >> >>>>>>>>>>> Reviewed-by: Maarten Lankhorst
> >> >>>>>>>>>>> <maarten.lankhorst@linux.intel.com>
> >> >>>>>>>>>>> ---
> >> >>>>>>>>>>>    drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
> >> >>>>>>>>>>>    drivers/gpu/drm/drm_connector.c   | 78
> >> >>>>>>>>>> +++++++++++++++++++++++++++++++++++++++
> >> >>>>>>>>>>>    include/drm/drm_connector.h       | 50
> >+++++++++++++++++++++++++
> >> >>>>>>>>>>>    3 files changed, 132 insertions(+)
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> >>>>>>>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> >>>>>>>>>>> index 9a1f41a..9b5d44f 100644
> >> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> >>>>>>>>>>> @@ -746,6 +746,8 @@ static int
> >> >>>>>>>>>>> drm_atomic_connector_set_property(struct
> >> >>>>>>>>>> drm_connector *connector,
> >> >>>>>>>>>>>    			return -EINVAL;
> >> >>>>>>>>>>>    		}
> >> >>>>>>>>>>>    		state->content_protection = val;
> >> >>>>>>>>>>> +	} else if (property == connector->colorspace_property) {
> >> >>>>>>>>>>> +		state->colorspace = val;
> >> >>>>>>>>>>>    	} else if (property == config->writeback_fb_id_property) {
> >> >>>>>>>>>>>    		struct drm_framebuffer *fb =
> >> >>>>>> drm_framebuffer_lookup(dev,
> >> >>>>>>>>>> NULL, val);
> >> >>>>>>>>>>>    		int ret =
> >> >>>>>> drm_atomic_set_writeback_fb_for_connector(state,
> >> >>>>>>>>>> fb); @@
> >> >>>>>>>>>>> -814,6 +816,8 @@ static int
> >> >>>>>>>>>>> drm_atomic_connector_set_property(struct
> >> >>>>>>>>>> drm_connector *connector,
> >> >>>>>>>>>>>    		*val = state->picture_aspect_ratio;
> >> >>>>>>>>>>>    	} else if (property == config->content_type_property) {
> >> >>>>>>>>>>>    		*val = state->content_type;
> >> >>>>>>>>>>> +	} else if (property == connector->colorspace_property) {
> >> >>>>>>>>>>> +		*val = state->colorspace;
> >> >>>>>>>>>>>    	} else if (property == connector->scaling_mode_property) {
> >> >>>>>>>>>>>    		*val = state->scaling_mode;
> >> >>>>>>>>>>>    	} else if (property ==
> >> >>>>>>>>>>> connector->content_protection_property)
> >> >>>>>>>>>>> { diff --git a/drivers/gpu/drm/drm_connector.c
> >> >>>>>>>>>>> b/drivers/gpu/drm/drm_connector.c index 8475396..4309873
> >> >>>>>>>>>>> 100644
> >> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_connector.c
> >> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
> >> >>>>>>>>>>> @@ -826,6 +826,33 @@ int
> >> >>>>>>>>>>> drm_display_info_set_bus_formats(struct
> >> >>>>>>>>>>> drm_display_info *info,  };
> >> >>>>>>>>>>> DRM_ENUM_NAME_FN(drm_get_content_protection_name,
> >> >>>>>>>>>> drm_cp_enum_list)
> >> >>>>>>>>>>> +static const struct drm_prop_enum_list hdmi_colorspaces[] = {
> >> >>>>>>>>>>> +	/* For Default case, driver will set the colorspace */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> >> >>>>>>>>>>> +	/* Standard Definition Colorimetry based on CEA 861 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_SMPTE_170M,
> >"SMPTE_170M" },
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_BT709, "BT709" },
> >> >>>>>>>>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4
> >*/
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> >> >>>>>>>>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> >> >>>>>>>>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> >> >>>>>>>>>>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> >> >>>>>>>>>>> +	/* Colorimetry based on IEC 61966-2-5 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> >> >>>>>>>>>>> +	/* Colorimetry based on ITU-R BT.2020 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC"
> >},
> >> >>>>>>>>>>> +	/* Colorimetry based on ITU-R BT.2020 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB"
> >},
> >> >>>>>>>>>>> +	/* Colorimetry based on ITU-R BT.2020 */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_BT2020_CYCC,
> >"BT2020_CYCC" },
> >> >>>>>>>>>>> +	/* Added as part of Additional Colorimetry Extension in
> >861.G */
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-
> >> >>>>>> P3_RGB_D65" },
> >> >>>>>>>>>>> +	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-
> >> >>>>>>>>>> P3_RGB_Theater" },
> >> >>>>>>>>>>> +};
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>>    /**
> >> >>>>>>>>>>>     * DOC: standard connector properties
> >> >>>>>>>>>>>     *
> >> >>>>>>>>>>> @@ -1548,6 +1575,57 @@ int
> >> >>>>>>>>>>> drm_mode_create_aspect_ratio_property(struct drm_device
> >> >>>>>>>>>>> *dev)
> >> >>>>>>>>>>> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> >> >>>>>>>>>>>
> >> >>>>>>>>>>>    /**
> >> >>>>>>>>>>> + * DOC: standard connector properties
> >> >>>>>>>>>>> + *
> >> >>>>>>>>>>> + * Colorspace:
> >> >>>>>>>>>>> + *     drm_mode_create_colorspace_property - create colorspace
> >> >>>>>> property
> >> >>>>>>>>>>> + *     This property helps select a suitable colorspace based on the
> >sink
> >> >>>>>>>>>>> + *     capability. Modern sink devices support wider gamut like
> >BT2020.
> >> >>>>>>>>>>> + *     This helps switch to BT2020 mode if the BT2020 encoded video
> >> >>>>>> stream
> >> >>>>>>>>>>> + *     is being played by the user, same for any other colorspace.
> >Thereby
> >> >>>>>>>>>>> + *     giving a good visual experience to users.
> >> >>>>>>>>>>> + *
> >> >>>>>>>>>>> + *     The expectation from userspace is that it should parse the EDID
> >> >>>>>>>>>>> + *     and get supported colorspaces. Use this property and switch to
> >the
> >> >>>>>>>>>>> + *     one supported. Sink supported colorspaces should be retrieved
> >by
> >> >>>>>>>>>>> + *     userspace from EDID and driver will not explicitly expose them.
> >> >>>>>>>>>>> + *
> >> >>>>>>>>>>> + *     Basically the expectation from userspace is:
> >> >>>>>>>>>>> + *      - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some
> >sink
> >> >>>>>>>>>>> + *        colorspace
> >> >>>>>>>>>>> + *      - Set this new property to let the sink know what it
> >> >>>>>>>>>>> + *        converted the CRTC output to.
> >> >>>>>>>>>>> + *      - This property is just to inform sink what colorspace
> >> >>>>>>>>>>> + *        source is trying to drive.
> >> >>>>>>>>>>> + *
> >> >>>>>>>>>>> + * Called by a driver the first time it's needed, must be
> >> >>>>>>>>>>> +attached to desired
> >> >>>>>>>>>>> + * connectors.
> >> >>>>>>>>>>> + */
> >> >>>>>>>>>>> +int drm_mode_create_colorspace_property(struct
> >> >>>>>>>>>>> +drm_connector
> >> >>>>>>>>>>> +*connector) {
> >> >>>>>>>>>>> +	struct drm_device *dev = connector->dev;
> >> >>>>>>>>>>> +	struct drm_property *prop;
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>> +	if (connector->connector_type ==
> >> >>>>>> DRM_MODE_CONNECTOR_HDMIA ||
> >> >>>>>>>>>>> +	    connector->connector_type ==
> >> >>>>>> DRM_MODE_CONNECTOR_HDMIB) {
> >> >>>>>>>>>>> +		prop = drm_property_create_enum(dev,
> >> >>>>>>>>>> DRM_MODE_PROP_ENUM,
> >> >>>>>>>>>>> +						"Colorspace",
> >> >>>>>>>>>>> +						hdmi_colorspaces,
> >> >>>>>>>>>>> +
> >> >>>>>>>>>> 	ARRAY_SIZE(hdmi_colorspaces));
> >> >>>>>>>>>>> +		if (!prop)
> >> >>>>>>>>>>> +			return -ENOMEM;
> >> >>>>>>>>>>> +	} else {
> >> >>>>>>>>>>> +		DRM_DEBUG_KMS("Colorspace property not
> >> >>>>>> supported\n");
> >> >>>>>>>>>>> +		return 0;
> >> >>>>>>>>>>> +	}
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>> +	connector->colorspace_property = prop;
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>> +	return 0;
> >> >>>>>>>>>>> +}
> >> >>>>>>>>>>> +EXPORT_SYMBOL(drm_mode_create_colorspace_property);
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>> +/**
> >> >>>>>>>>>>>     * drm_mode_create_content_type_property - create
> >> >>>>>>>>>>> content type
> >> >>>>>> property
> >> >>>>>>>>>>>     * @dev: DRM device
> >> >>>>>>>>>>>     *
> >> >>>>>>>>>>> diff --git a/include/drm/drm_connector.h
> >> >>>>>>>>>>> b/include/drm/drm_connector.h index 9941613..58db66e
> >> >>>>>>>>>>> 100644
> >> >>>>>>>>>>> --- a/include/drm/drm_connector.h
> >> >>>>>>>>>>> +++ b/include/drm/drm_connector.h
> >> >>>>>>>>>>> @@ -253,6 +253,42 @@ enum drm_panel_orientation {
> >> >>>>>>>>>>>    	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >> >>>>>>>>>>>    };
> >> >>>>>>>>>>>
> >> >>>>>>>>>>> +/*
> >> >>>>>>>>>>> + * This is a consolidated colorimetry list supported by
> >> >>>>>>>>>>> +HDMI and
> >> >>>>>>>>>>> + * DP protocol standard. The respective connectors will
> >> >>>>>>>>>>> +register
> >> >>>>>>>>>>> + * a property with the subset of this list (supported by
> >> >>>>>>>>>>> +that
> >> >>>>>>>>>>> + * respective protocol). Userspace will set the
> >> >>>>>>>>>>> +colorspace through
> >> >>>>>>>>>>> + * a colorspace property which will be created and
> >> >>>>>>>>>>> +exposed to
> >> >>>>>>>>>>> + * userspace.
> >> >>>>>>>>>>> + */
> >> >>>>>>>>>>> +
> >> >>>>>>>>>>> +/* For Default case, driver will set the colorspace */
> >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_DEFAULT			0
> >> >>>>>>>>>>> +/* CEA 861 Normal Colorimetry options */
> >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_NO_DATA			0
> >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_SMPTE_170M
> >> >>>>>>>> 	1
> >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_BT709			2
> >> >>>>>>>>>> Still missing the YCbCr information in these two.
> >> >>>>>>>>> As per CTA 861.G spec, we have these as only SMPTE_170M and
> >> >>>>>>>>> ITU-R BT709, with no specific mention of RGB or YCbCr.
> >> >>>>>>>>> Hence, kept it as per spec. We seem to have a common field for both as
> >per CTA spec.
> >> >>>>>>>>> Correct me if my understanding is wrong.
> >> >>>>>>>> Check
> >> >>>>>>>> "Table 14 Picture Colorimetry Indicated by the RGB or YC B C
> >> >>>>>>>> R (Y), Colorimetry
> >> >>>>>>>> (C) and Extended Colorimetry (EC) Field Settings"
> >> >>>>>>> These  Y2 Y1 Y0 values should be programmed separately and not
> >> >>>>>>> through the colorspace as they are data formats isn't it. I
> >> >>>>>>> feel this should get controlled separately independent of
> >> >>>>>>> colorimetry, or should we add all the YCbCr and RGB versions
> >> >>>>>>> and program them as part of this property itself
> >> >>>>>> ?
> >> >>>>>>
> >> >>>>>> I don't think we can separate them. The values of the
> >> >>>>>> colorimetry bits doesn't mean anything without the Y bits. It
> >> >>>>>> would make the uapi kinda crazy if we allow the user to specify
> >> >>>>>> BT.2020 RGB but then we actually signal BT.2020 YCbCr in the
> >> >>>>>> infoframe, or vice versa. Or we just signal a totally invalid value for all the
> >other cases.
> >> >>>>> I agree we need data format also to be send along with
> >> >>>>> colorspace, but they are 2 different things. To handle YCbCr and
> >> >>>>> variants (YUV 444, 420 or 422) and RGB I feel we should expose a
> >> >>>>> different API instead of using this one.  We can create an output csc property
> >which does that job for us.
> >> >>>>>
> >> >>>>> So from a user perspective if we wants to set a colorspace we
> >> >>>>> should use the one in this series and set the data format
> >> >>>>> separately using the other interface. Both will be received in the state
> >variables and later Infoframe packet will be created accordingly.
> >> >>>>> Clubbing both in one may lead to lot of possible combinations
> >> >>>>> exposed as enum which may not look too clean.
> >> >>>>>
> >> >>>>> What do you say about handling that in the output csc property.
> >> >>>>> I will go with what you recommend .
> >> >>>> Programming Y2Y1Y0 is already taken care of, when we added support for
> >YCBCR outputs (4:2:0 implementation).
> >> >>>> In intel_hdmi.c:intel_hdmi_set_avi_infoframe():
> >> >>>>
> >> >>>> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> >> >>>> 	frame.avi.colorspace = HDMI_COLORSPACE_YUV420; else if
> >> >>>> (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
> >> >>>> 	frame.avi.colorspace = HDMI_COLORSPACE_YUV444; else
> >> >>>> 	frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >> >>>>
> >> >>>> IMO This looks like a better way to handle this, ie while adding support for a
> >new format, add support for corresponding AVI IF fileds. This will also make scope of
> >the property under discussion less complicated.
> >> >>> That's an exceptional case. We're programming the CSC in that case
> >> >>> to do the RGB->YCbCr conversion without telling userspace. So we
> >> >>> must also configure the Y bits automagically.
> >> >>>
> >> >>> What we want is a check like so:
> >> >>>
> >> >>> if (prop.colorspace != default && output_format != RGB)
> >> >>>       return -INVAL;
> >> >>>
> >> >>> because we can't set up the CSC to make a mess of the user's
> >> >>> carefully crafted pixels. The pixels might not even contain RGB
> >> >>> data in that case.
> >> >>>
> >> >>> We'll need to extend that a little bit once we add the explicit
> >> >>> control of the output CSC via another prop. But the same principle
> >> >>> should hold.
> >> >> In fact, I remember the reason why we created infrastructure like
> >> >> this, so that, going forward, we could just add a output_format
> >> >> property, for which, we already have the backed and the state
> >> >> variables ready. The consume of this backend can be this
> >> >> drm_property (like output_format), or internal modeset like
> >> >> YCBCR4:2:0 only modes in EDID. It will also be inline with your suggestion of CSC
> >property.
> >> >>
> >> >> So we can have two properties colorspace (BT2020/SRGB/REC709) and
> >> >> color_model/color_format(RGB/YCBCR444/420), and a combination of
> >> >> both should program the output AVI info-frames. Does it appeal you ?
> >> > I'm not sure it's feasible to split it like that. The combinations
> >> > supported by the infoframe are rather specific so trying to split it
> >> > up leads to either super limited uapi, or one that exposes a lot of
> >> > illegal combinations. Also DP has slightly different set of things
> >> > it supports which adds more complications.
> >> >
> >> > So I think the best option is still to include the Y=yes/no
> >> > information in the prop value alognside the C/EC/ACE bits.
> >>
> >> Yes, you are right, this could be a simpler way of doing things, but
> >> with a limited set of combination.
> >>
> >> Consider this example:
> >>
> >> - If a HDMI monitor supports BT2020, but doesn't support YCBCR
> >> formatting, how to handle this option ? This means we have to
> >> selectively show BT2020_RGB enum value only not BT2020_YCBCR_444/420,
> >>
> >> - This also means we have to probe the EDID first, and then create
> >> this property, which means we will be dependent on the hot-plug /
> >> detect to create this property, instead of doing this in hdmi_init.
> >
> >IMO we just ignore this problem and expose all the legal options the spec has.
> >
> >If userspace wants to know what's actually supported it will have to parse the EDID.
> >We could think about parsing that in the kernel too and exposing an immutable prop
> >to expose that infromation. But I'm not sure this is such a good idea since we'd
> >basically drown in properties if we try to expose all the information from the EDID. A
> >better long term plan would be a common EDID parsing library for everyone.
> >
> >>
> >> Even more complex scenario would be when the output support depends on
> >> monitor + platform both. This means we have to provide this
> >> combination upfront while creating this property.
> >>
> >> On the other hand, the invalid combination of two property, it would
> >> be easy to detect at runtime at atomic_check() and fail the commit.
> >>
> >>
> >> This is just a thought, honestly I am also not sure what would be the
> >> most appropriate solution for this, but seems its simpler to create
> >> property with two properties, might be difficult to manage at runtime.
> >>
> >> With one property, its very difficult to create, but once created, it
> >> would work sure shot.
> >
> >I don't think it's that difficult. Just expose all the things in the
> >CTA-861 big table. Which means including the Y bit too.
> 
> Hi Ville,
> Ok So will try to do this way then:
> 
> Have a separate enum option like 
> DRM_MODE_COLORIMETRY_BT709_YUV444
> DRM_MODE_COLORIMETRY_BT709_YUV420
> DRM_MODE_COLORIMETRY_BT709_YUV422

Hmm. I'm not quite convinced that we want the subsampling information
included here. That will make it difficult to do the output csc
property in a way that leaves the driver free to select the subsampling
automagically, which we proably still want because of those annoying
HDMI 2.0 monitors.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel