diff mbox series

[v10,3/3] drm/i915: Attach colorspace property and enable modeset

Message ID 1548852866-32349-4-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add Colorspace connector property interface | expand

Commit Message

Shankar, Uma Jan. 30, 2019, 12:54 p.m. UTC
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: Merged the DP handling along with platform colorspace
handling as per Shashank's comments.

v6: Reverted to old design of exposing all colorspaces to
userspace as per Ville's review comment

v7: Fixed a checkpatch complaint, Addressed  Maarten' review
comment, updated the RB from Maarten and Jani's ack.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c    |  1 +
 drivers/gpu/drm/i915/intel_connector.c |  8 ++++++++
 drivers/gpu/drm/i915/intel_drv.h       |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c      | 25 +++++++++++++++++++++++++
 4 files changed, 35 insertions(+)

Comments

Ville Syrjala Feb. 1, 2019, 7:01 p.m. UTC | #1
On Wed, Jan 30, 2019 at 06:24:26PM +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: Merged the DP handling along with platform colorspace
> handling as per Shashank's comments.
> 
> v6: Reverted to old design of exposing all colorspaces to
> userspace as per Ville's review comment
> 
> v7: Fixed a checkpatch complaint, Addressed  Maarten' review
> comment, updated the RB from Maarten and Jani's ack.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c    |  1 +
>  drivers/gpu/drm/i915/intel_connector.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c      | 25 +++++++++++++++++++++++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 16263ad..a4bb906 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_conn_state->base.colorspace != old_conn_state->base.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..8352d0b 100644
> --- a/drivers/gpu/drm/i915/intel_connector.c
> +++ b/drivers/gpu/drm/i915/intel_connector.c
> @@ -265,3 +265,11 @@ 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)
> +{
> +	if (!drm_mode_create_colorspace_property(connector))
> +		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..5c5009d 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;

Default should map to NONE.

> +	} 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;
> +	}

This code seems like something that should be in the core.

Like said earlier maybe we could just directly map the
values to these bits, and then this should just become

if.colorimetry = val & 0x3;
if.extended_colorimetry = (val >> 2) & 0x7;
if.ace = (val >> 5) & 0xf;

Hmm, looks like hdmi.h doesn't even know about the ACE bits yet :(

> +
>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>  					   conn_state->connector,
>  					   adjusted_mode,
> @@ -2143,10 +2161,17 @@ 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Shankar, Uma Feb. 4, 2019, 5:18 p.m. UTC | #2
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>Ville Syrjälä
>Sent: Saturday, February 2, 2019 12:31 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>; dri-
>devel@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [v10 3/3] drm/i915: Attach colorspace property and
>enable modeset
>
>On Wed, Jan 30, 2019 at 06:24:26PM +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: Merged the DP handling along with platform colorspace handling as
>> per Shashank's comments.
>>
>> v6: Reverted to old design of exposing all colorspaces to userspace as
>> per Ville's review comment
>>
>> v7: Fixed a checkpatch complaint, Addressed  Maarten' review comment,
>> updated the RB from Maarten and Jani's ack.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c    |  1 +
>>  drivers/gpu/drm/i915/intel_connector.c |  8 ++++++++
>>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_hdmi.c      | 25 +++++++++++++++++++++++++
>>  4 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 16263ad..a4bb906 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_conn_state->base.colorspace !=
>> +old_conn_state->base.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..8352d0b 100644
>> --- a/drivers/gpu/drm/i915/intel_connector.c
>> +++ b/drivers/gpu/drm/i915/intel_connector.c
>> @@ -265,3 +265,11 @@ 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) {
>> +	if (!drm_mode_create_colorspace_property(connector))
>> +		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..5c5009d 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;
>
>Default should map to NONE.

Ok, I will make it NO DATA  i.e. 0 for DEFAULT.

>
>> +	} 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;
>> +	}
>
>This code seems like something that should be in the core.
>
>Like said earlier maybe we could just directly map the values to these bits, and
>then this should just become
>
>if.colorimetry = val & 0x3;
>if.extended_colorimetry = (val >> 2) & 0x7; if.ace = (val >> 5) & 0xf;

Ok, will modify along these lines.

>Hmm, looks like hdmi.h doesn't even know about the ACE bits yet :(

:)

Thanks Ville for all your comments and suggestion.

Regards,
Uma Shankar
>
>> +
>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>  					   conn_state->connector,
>>  					   adjusted_mode,
>> @@ -2143,10 +2161,17 @@ 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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shankar, Uma Feb. 5, 2019, 3:28 p.m. UTC | #3
>-----Original Message-----
>From: Shankar, Uma
>Sent: Monday, February 4, 2019 10:49 PM
>To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>; dri-
>devel@lists.freedesktop.org
>Subject: RE: [Intel-gfx] [v10 3/3] drm/i915: Attach colorspace property and
>enable modeset
>
>
>
>>-----Original Message-----
>>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
>>Behalf Of Ville Syrjälä
>>Sent: Saturday, February 2, 2019 12:31 AM
>>To: Shankar, Uma <uma.shankar@intel.com>
>>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville
>><ville.syrjala@intel.com>; Lankhorst, Maarten
>><maarten.lankhorst@intel.com>; dri- devel@lists.freedesktop.org
>>Subject: Re: [Intel-gfx] [v10 3/3] drm/i915: Attach colorspace property
>>and enable modeset
>>
>>On Wed, Jan 30, 2019 at 06:24:26PM +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: Merged the DP handling along with platform colorspace handling as
>>> per Shashank's comments.
>>>
>>> v6: Reverted to old design of exposing all colorspaces to userspace
>>> as per Ville's review comment
>>>
>>> v7: Fixed a checkpatch complaint, Addressed  Maarten' review comment,
>>> updated the RB from Maarten and Jani's ack.
>>>
>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_atomic.c    |  1 +
>>>  drivers/gpu/drm/i915/intel_connector.c |  8 ++++++++
>>>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>>>  drivers/gpu/drm/i915/intel_hdmi.c      | 25 +++++++++++++++++++++++++
>>>  4 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>> index 16263ad..a4bb906 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_conn_state->base.colorspace !=
>>> +old_conn_state->base.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..8352d0b 100644
>>> --- a/drivers/gpu/drm/i915/intel_connector.c
>>> +++ b/drivers/gpu/drm/i915/intel_connector.c
>>> @@ -265,3 +265,11 @@ 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) {
>>> +	if (!drm_mode_create_colorspace_property(connector))
>>> +		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..5c5009d 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;
>>
>>Default should map to NONE.
>
>Ok, I will make it NO DATA  i.e. 0 for DEFAULT.
>
>>
>>> +	} 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;
>>> +	}
>>
>>This code seems like something that should be in the core.
>>
>>Like said earlier maybe we could just directly map the values to these
>>bits, and then this should just become
>>
>>if.colorimetry = val & 0x3;
>>if.extended_colorimetry = (val >> 2) & 0x7; if.ace = (val >> 5) & 0xf;
>
>Ok, will modify along these lines.

On looking further, it feels to have the enum defined sequentially for userspace
and then respective encoders taking care as per spec looks better. In the case here,
both DP and HDMI share some colorspaces so we can't have a common value for both
as far as infoframe programming is concerned. Also currently with ACE, total bits go to
9 for HDMI. Hence we need to have enum value going randomly from 0 to 512 which will
look more like magic numbers to userspace.

Currently I have moved this to drm layer as you suggested, will send the next version with all
your comments addressed. Planning to send separate series to handle ACE stuff in linux/hdmi
and then extend this as well as part of that series. Hope this is ok.

Regards,
Uma Shankar

>>Hmm, looks like hdmi.h doesn't even know about the ACE bits yet :(
>
>:)
>
>Thanks Ville for all your comments and suggestion.
>
>Regards,
>Uma Shankar
>>
>>> +
>>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>  					   conn_state->connector,
>>>  					   adjusted_mode,
>>> @@ -2143,10 +2161,17 @@ 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
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>--
>>Ville Syrjälä
>>Intel
>>_______________________________________________
>>dri-devel mailing list
>>dri-devel@lists.freedesktop.org
>>https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 16263ad..a4bb906 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_conn_state->base.colorspace != old_conn_state->base.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..8352d0b 100644
--- a/drivers/gpu/drm/i915/intel_connector.c
+++ b/drivers/gpu/drm/i915/intel_connector.c
@@ -265,3 +265,11 @@  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)
+{
+	if (!drm_mode_create_colorspace_property(connector))
+		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..5c5009d 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,17 @@  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;