[v8 2/2] drm/i915: Attach colorspace property and enable modeset

Sharma, Shashank shashank.sharma at linux.intel.com
Wed Jan 30 07:57:33 UTC 2019


   Hello Ville,

On 1/29/2019 9:33 PM, Ville Syrjälä wrote:
> On Tue, Jan 29, 2019 at 03:57:29PM +0000, Shankar, Uma wrote:
>>
>>> -----Original Message-----
>>> From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>>> Ville Syrjälä
>>> Sent: Tuesday, January 29, 2019 9:14 PM
>>> To: Shankar, Uma <uma.shankar at intel.com>
>>> Cc: emil.l.velikov at gmail.com; intel-gfx at lists.freedesktop.org; Syrjala, Ville
>>> <ville.syrjala at intel.com>; Lankhorst, Maarten <maarten.lankhorst at intel.com>;
>>> dri-devel at lists.freedesktop.org
>>> Subject: Re: [v8 2/2] drm/i915: Attach colorspace property and enable modeset
>>>
>>> On Tue, Jan 29, 2019 at 09:22:56PM +0530, Uma Shankar wrote:
>>>> This patch attaches the colorspace connector property to the hdmi
>>>> connector. Based on colorspace change, modeset will be triggered to
>>>> switch to new colorspace.
>>>>
>>>> Based on colorspace property value create an infoframe with
>>>> appropriate colorspace. This can be used to send an infoframe packet
>>>> with proper colorspace value set which will help to enable wider color
>>>> gamut like BT2020 on sink.
>>>>
>>>> This patch attaches and enables HDMI colorspace, DP will be taken care
>>>> separately.
>>>>
>>>> v2: Merged the changes of creating infoframe as well to this patch as
>>>> per Maarten's suggestion.
>>>>
>>>> v3: Addressed review comments from Shashank. Separated HDMI and DP
>>>> colorspaces as suggested by Ville and Maarten.
>>>>
>>>> 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. Handle the
>>>> default case properly.
>>>>
>>>> v5: Added Platform specific colorspace enums and called the property
>>>> creation helper using the same.
>>>>
>>>> v6: Addressed Shashank's review comments.
>>>>
>>>> v7: Rebase
>>>>
>>>> v8: Addressed Maarten's review comments.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>>> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_atomic.c    |  1 +
>>>>   drivers/gpu/drm/i915/intel_connector.c | 63
>>> ++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/intel_drv.h       |  1 +
>>>>   drivers/gpu/drm/i915/intel_hdmi.c      | 24 +++++++++++++
>>>>   4 files changed, 89 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 16263ad..76b7114 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -124,6 +124,7 @@ int intel_digital_connector_atomic_check(struct
>>> drm_connector *conn,
>>>>   	 */
>>>>   	if (new_conn_state->force_audio != old_conn_state->force_audio ||
>>>>   	    new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb
>>>> ||
>>>> +	    new_state->colorspace != old_state->colorspace ||
>>>>   	    new_conn_state->base.picture_aspect_ratio != old_conn_state-
>>>> base.picture_aspect_ratio ||
>>>>   	    new_conn_state->base.content_type != old_conn_state-
>>>> base.content_type ||
>>>>   	    new_conn_state->base.scaling_mode !=
>>>> old_conn_state->base.scaling_mode)
>>>> diff --git a/drivers/gpu/drm/i915/intel_connector.c
>>>> b/drivers/gpu/drm/i915/intel_connector.c
>>>> index ee16758..9a12d5e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_connector.c
>>>> +++ b/drivers/gpu/drm/i915/intel_connector.c
>>>> @@ -30,6 +30,48 @@
>>>>   #include "intel_drv.h"
>>>>   #include "i915_drv.h"
>>>>
>>>> +static const struct drm_prop_enum_list gen10_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_ITU_601, "ITU_601" },
>>>> +	{ DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>>> +	{ DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_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_RGB, "BT2020_RGB" },
>>>> +	/* 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_CYCC, "BT2020_CYCC" }, };
>>>> +
>>>> +static const struct drm_prop_enum_list legacy_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_ITU_601, "ITU_601" },
>>>> +	{ DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>>> +	{ DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_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" }, };
>>> Why are we duplicating these in the driver code? And why do we have different
>>> sets for gen10 and older?
>> This is to expose colorspaces which a particular platforms supports instead of giving
>> full HDMI protocol supported list as some platforms may not support all of it.
>>
>> Here, BT2020 support is added for Gen10 onwards, while older platforms support
>> all  the rest apart from BT2020.
> This is just about the infoframe. I don't think we have any platform
> limitations there.

I am not sure about this, reasons:

- this property not only prepares infoframe, but indicates userspace 
that the platforms has support for following colorspaces. Many 
userspaces might be taking a call of BT2020/HDR support based on this 
property.

- Also, can we send CTA-861-G infoframes using a HDMI-1.4b devices ? 
Even if we can, is that the right thing to do ?


- Shashank

>> For sink supported colorspaces, userspace will retrieve it from EDID and set the colorspace
>> which is supported by both platform as well as sink.
>>
>> Regards,
>> Uma Shankar
>>
>>>> +
>>>>   int intel_connector_init(struct intel_connector *connector)  {
>>>>   	struct intel_digital_connector_state *conn_state; @@ -265,3 +307,24
>>>> @@ int intel_ddc_get_modes(struct drm_connector *connector,
>>>>   			connector->dev->mode_config.aspect_ratio_property,
>>>>   			DRM_MODE_PICTURE_ASPECT_NONE);
>>>>   }
>>>> +
>>>> +void
>>>> +intel_attach_colorspace_property(struct drm_connector *connector) {
>>>> +	struct drm_device *dev = connector->dev;
>>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +
>>>> +	if (INTEL_GEN(dev_priv) >= 10) {
>>>> +		if (!drm_mode_create_colorspace_property(connector,
>>>> +			gen10_hdmi_colorspaces,
>>>> +			ARRAY_SIZE(gen10_hdmi_colorspaces)))
>>>> +			drm_object_attach_property(&connector->base,
>>>> +				connector->colorspace_property, 0);
>>>> +	} else {
>>>> +		if (!drm_mode_create_colorspace_property(connector,
>>>> +			legacy_hdmi_colorspaces,
>>>> +			ARRAY_SIZE(legacy_hdmi_colorspaces)))
>>>> +			drm_object_attach_property(&connector->base,
>>>> +				connector->colorspace_property, 0);
>>>> +	}
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 85b913e..5178a9a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1783,6 +1783,7 @@ int intel_connector_update_modes(struct
>>>> drm_connector *connector,  void
>>>> intel_attach_force_audio_property(struct drm_connector *connector);
>>>> void intel_attach_broadcast_rgb_property(struct drm_connector
>>>> *connector);  void intel_attach_aspect_ratio_property(struct
>>>> drm_connector *connector);
>>>> +void intel_attach_colorspace_property(struct drm_connector
>>>> +*connector);
>>>>
>>>>   /* intel_csr.c */
>>>>   void intel_csr_ucode_init(struct drm_i915_private *); diff --git
>>>> a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index 97a98e1..9ed00e3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -498,6 +498,24 @@ static void intel_hdmi_set_avi_infoframe(struct
>>> intel_encoder *encoder,
>>>>   	else
>>>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>
>>>> +	if (conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT) {
>>>> +		/* Set ITU 709 as default for HDMI */
>>>> +		frame.avi.colorimetry = DRM_MODE_COLORIMETRY_ITU_709;
>>>> +	} else if (conn_state->colorspace <
>>> DRM_MODE_COLORIMETRY_XV_YCC_601) {
>>>> +		frame.avi.colorimetry = conn_state->colorspace;
>>>> +	} else {
>>>> +		frame.avi.colorimetry = HDMI_COLORIMETRY_EXTENDED;
>>>> +		/*
>>>> +		 * Starting from extended list where COLORIMETRY_XV_YCC_601
>>>> +		 * is the first extended mode and its value is 0 as per HDMI
>>>> +		 * specification.
>>>> +		 * TODO: This needs to be extended for LSPCON implementation
>>>> +		 * as well. Will be implemented separately.
>>>> +		 */
>>>> +		frame.avi.extended_colorimetry = conn_state->colorspace -
>>>> +			DRM_MODE_COLORIMETRY_XV_YCC_601;
>>>> +	}
>>>> +
>>>>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>>   					   conn_state->connector,
>>>>   					   adjusted_mode,
>>>> @@ -2143,10 +2161,16 @@ static void intel_hdmi_destroy(struct
>>>> drm_connector *connector)  intel_hdmi_add_properties(struct intel_hdmi
>>>> *intel_hdmi, struct drm_connector *connector)  {
>>>>   	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>>> +	struct intel_digital_port *intel_dig_port =
>>>> +				hdmi_to_dig_port(intel_hdmi);
>>>>
>>>>   	intel_attach_force_audio_property(connector);
>>>>   	intel_attach_broadcast_rgb_property(connector);
>>>>   	intel_attach_aspect_ratio_property(connector);
>>>> +	/* Attach Colorspace property for Non LSPCON based device */
>>>> +	if (!intel_dig_port->lspcon.active)
>>>> +		intel_attach_colorspace_property(connector);
>>>> +
>>>>   	drm_connector_attach_content_type_property(connector);
>>>>   	connector->state->picture_aspect_ratio =
>>> HDMI_PICTURE_ASPECT_NONE;
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> --
>>> Ville Syrjälä
>>> Intel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


More information about the dri-devel mailing list