Re: [v15 4/4] drm/i915: Attach colorspace property and enable modeset

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Uma Shankar <uma.shankar@intel.com>
Cc: intel-gfx@lists.freedesktop.org, ville.syrjala@intel.com,
	maarten.lankhorst@intel.com, dri-devel@lists.freedesktop.org
Subject: Re: [v15 4/4] drm/i915: Attach colorspace property and enable modeset
Date: Tue, 12 Feb 2019 19:02:00 +0200	[thread overview]
Message-ID: <20190212170200.GB20097@intel.com> (raw)
In-Reply-To: <1549650296-28648-5-git-send-email-uma.shankar@intel.com>

On Fri, Feb 08, 2019 at 11:54: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: 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.
> 
> v8: Moved colorspace AVI Infoframe programming to drm core and
> removed from driver as per Ville's suggestion.
> 
> v9: Added a check to allow only RGB colorspaces to be set in
> infoframe through the colorspace property. Since there is no output
> csc property to control planar formats and it will be added later.
> Changes for RGB->YUV conversion inside driver without userspace
> knowledge is still supported. This is as per Ville's suggestion.
> 
> v10: Fixed an error in if check for rgb colorspace.
> 
> 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    | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_connector.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c      | 13 +++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 7cf9290..ba60d51 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -102,6 +102,20 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
>  	return -EINVAL;
>  }
>  
> +static inline bool check_colorspace_is_rgb(u32 colorspace)
> +{
> +	if (colorspace | (DRM_MODE_COLORIMETRY_OPRGB |
> +			   DRM_MODE_COLORIMETRY_BT2020_RGB |
> +			   DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65 |
> +			   DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER |
> +			   DRM_MODE_DP_COLORIMETRY_SRGB |
> +			   DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT |
> +			   DRM_MODE_DP_COLORIMETRY_SCRGB))
> +		return true;

That's the same as

bool func(...)
{
	return true;
}

> +
> +	return false;
> +}
> +
>  int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  					 struct drm_connector_state *new_state)
>  {
> @@ -118,6 +132,15 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  	if (!new_state->crtc)
>  		return 0;
>  
> +	/*
> +	 * Reject any planar formats, as currently not enabled.
> +	 * ToDo: Add a output CSC property interface to control planar
> +	 * formats.
> +	 */
> +	if ((new_conn_state->base.colorspace != DRM_MODE_COLORIMETRY_DEFAULT) ||
> +	    !check_colorspace_is_rgb(new_conn_state->base.colorspace))
> +		return 0;

Not sure what this stuff has to do with planar vs. not.

Either way, the code doesn't make sense.

> +
>  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
>  
>  	/*
> @@ -126,6 +149,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 5eb0b66..4af574f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1803,6 +1803,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 f125a62..765718b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -498,6 +498,8 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  
> +	drm_hdmi_avi_infoframe_colorspace(&frame.avi, conn_state);
> +
>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>  					   conn_state->connector,
>  					   adjusted_mode,
> @@ -2143,10 +2145,21 @@ 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
> +	 * ToDo: This needs to be extended for LSPCON implementation
> +	 * as well. Will be implemented separately.
> +	 */
> +	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx