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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 11:02 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
>dri-devel@xxxxxxxxxxxxxxxxxxxxx
>Cc: hansverk@xxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst,
>Maarten <maarten.lankhorst@xxxxxxxxx>; jonas@xxxxxxxxx
>Subject: Re: [v5 2/2] drm/i915: Attach colorspace property and enable modeset
>
>Regards
>
>Shashank
>
>
>On 12/11/2018 11:44 PM, 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>>   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      | 18 ++++++++++
>>   4 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index a5a2c8f..35ef70a 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -125,6 +125,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 18e370f..59fa420 100644
>> --- a/drivers/gpu/drm/i915/intel_connector.c
>> +++ b/drivers/gpu/drm/i915/intel_connector.c
>> @@ -31,6 +31,48 @@
>>   #include "intel_drv.h"
>>   #include "i915_drv.h"
>>
>> +static const struct drm_prop_enum_list gen10_hdmi_colorspace[] = {
>> +	/* For Default case, driver will set the colorspace */
>> +	{ COLORIMETRY_DEFAULT, "Default" },
>> +	/* Standard Definition Colorimetry based on CEA 861 */
>> +	{ COLORIMETRY_ITU_601, "ITU_601" },
>> +	{ COLORIMETRY_ITU_709, "ITU_709" },
>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> +	{ COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>> +	{ COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> +	{ COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
>> +	{ COLORIMETRY_OPYCC_601, "opYCC_601" },
>> +	/* Colorimetry based on IEC 61966-2-5 */
>> +	{ COLORIMETRY_OPRGB, "opRGB" },
>> +	/* Colorimetry based on ITU-R BT.2020 */
>> +	{ COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> +	/* Colorimetry based on ITU-R BT.2020 */
>> +	{ COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> +	/* Colorimetry based on ITU-R BT.2020 */
>> +	{ COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, };
>> +
>> +static const struct drm_prop_enum_list legacy_hdmi_colorspace[] = {
>> +	/* For Default case, driver will set the colorspace */
>> +	{ COLORIMETRY_DEFAULT, "Default" },
>> +	/* Standard Definition Colorimetry based on CEA 861 */
>> +	{ COLORIMETRY_ITU_601, "ITU_601" },
>> +	{ COLORIMETRY_ITU_709, "ITU_709" },
>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> +	{ COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>> +	{ COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> +	{ COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
>> +	{ COLORIMETRY_OPYCC_601, "opYCC_601" },
>> +	/* Colorimetry based on IEC 61966-2-5 */
>> +	{ COLORIMETRY_OPRGB, "opRGB" },
>> +};
>> +
>>   int intel_connector_init(struct intel_connector *connector)
>>   {
>>   	struct intel_digital_connector_state *conn_state; @@ -262,3 +304,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) {
>I am slightly unsure on if we should expose BT2020 colorspace support for
>CNL/GEN10 too, even though on paper it supports it, but do we have a proper
>color pipeline which can handle input degamma and more precisely the output
>gamma lut precision, which might be required post blending to restore BT2020
>non-linearity. Can you please confirm on this, and until, we should just keep it >
>10, instead of >= 10.

Gen10 does have capability to handle this. So we can enable this safely here. 

>> +		if (!drm_mode_create_colorspace_property(connector,
>> +			gen10_hdmi_colorspace,
>> +			ARRAY_SIZE(gen10_hdmi_colorspace)))
>> +			drm_object_attach_property(&connector->base,
>> +				connector->colorspace_property, 0);
>> +	} else {
>> +		if (!drm_mode_create_colorspace_property(connector,
>> +			legacy_hdmi_colorspace,
>> +			ARRAY_SIZE(legacy_hdmi_colorspace)))
>> +			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 a62d77b..891bc82 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1762,6 +1762,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 e2c6a2b..7ddc723 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -486,6 +486,22 @@ static void intel_hdmi_set_avi_infoframe(struct
>intel_encoder *encoder,
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>
>> +	if (conn_state->colorspace == COLORIMETRY_DEFAULT) {
>> +		/* Set ITU 709 as default for HDMI */
>> +		frame.avi.colorimetry = COLORIMETRY_ITU_709;
>> +	} else if (conn_state->colorspace < 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.
>> +		 */
>> +		frame.avi.extended_colorimetry = conn_state->colorspace -
>> +
>	COLORIMETRY_XV_YCC_601;
>> +	}
>We will need these changes for lspcon_set_infoframes() function too, as there
>are talks about needing HDR suppot using LSPCON. If you want to defer this to
>next patch, I would like to see a TODO: note.

Sure, I will add a TODO to track this.

>> +
>>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>>   					   crtc_state->limited_color_range ?
>>
>HDMI_QUANTIZATION_RANGE_LIMITED :
>> @@ -2135,6 +2151,8 @@ static void intel_hdmi_destroy(struct drm_connector
>*connector)
>>   	intel_attach_force_audio_property(connector);
>>   	intel_attach_broadcast_rgb_property(connector);
>>   	intel_attach_aspect_ratio_property(connector);
>> +	intel_attach_colorspace_property(connector);
>> +
>seems like extra blank line ?

Will fix this.

>>   	drm_connector_attach_content_type_property(connector);
>>   	connector->state->picture_aspect_ratio =
>HDMI_PICTURE_ASPECT_NONE;
>>
>With the above minor comments fixed, patch looks good to me, feel free to use:
>Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>

Thanks Shashank for the review.

Regards,
Uma Shankar
>_______________________________________________
>dri-devel mailing list
>dri-devel@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux