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

["Sharma, Shashank" <shashank.sharma@intel.com>]

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@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      | 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.
> +		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.
> +
>   	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 ?
>   	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@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx