diff mbox

[v1,2/2] i915: content-type property for HDMI connector

Message ID 20180416231659.9333-3-stanislav.lisovskiy@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stanislav Lisovskiy April 16, 2018, 11:16 p.m. UTC
From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Added encoding of drm content_type property from
drm_connector_state within AVI infoframe in order to properly handle
external HDMI TV content-type setting.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c |  1 +
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c   |  4 ++++
 drivers/gpu/drm/i915/intel_modes.c  | 10 ++++++++++
 4 files changed, 16 insertions(+)

Comments

Sean Paul April 16, 2018, 4:23 p.m. UTC | #1
On Tue, Apr 17, 2018 at 02:16:59AM +0300, StanLis wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> Added encoding of drm content_type property from
> drm_connector_state within AVI infoframe in order to properly handle
> external HDMI TV content-type setting.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Hi Stanislav,
Thank you for your patch.

> ---
>  drivers/gpu/drm/i915/intel_atomic.c |  1 +
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c   |  4 ++++
>  drivers/gpu/drm/i915/intel_modes.c  | 10 ++++++++++
>  4 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 40285d1b91b7..61ddb5871d8a 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.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)
>  		crtc_state->mode_changed = true;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5bd2263407b2..07fd7ba21f38 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1818,6 +1818,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>  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_content_type_property(struct drm_connector *connector);
>  
>  
>  /* intel_overlay.c */
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ee929f31f7db..cd484276e9b0 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  					   intel_hdmi->rgb_quant_range_selectable,
>  					   is_hdmi2_sink);
>  
> +	frame.avi.content_type = connector->state->content_type;
> +
>  	/* TODO: handle pixel repetition for YCBCR420 outputs */
>  	intel_write_infoframe(encoder, crtc_state, &frame);
>  }
> @@ -2065,7 +2067,9 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>  	intel_attach_force_audio_property(connector);
>  	intel_attach_broadcast_rgb_property(connector);
>  	intel_attach_aspect_ratio_property(connector);
> +	intel_attach_content_type_property(connector);
>  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +	connector->state->content_type = HDMI_CONTENT_TYPE_GRAPHICS;

This is redudant, the attach function already sets this.

>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> index b39846613e3c..232811ab71a3 100644
> --- a/drivers/gpu/drm/i915/intel_modes.c
> +++ b/drivers/gpu/drm/i915/intel_modes.c
> @@ -133,3 +133,13 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
>  			connector->dev->mode_config.aspect_ratio_property,
>  			DRM_MODE_PICTURE_ASPECT_NONE);
>  }
> +
> +void
> +intel_attach_content_type_property(struct drm_connector *connector)
> +{
> +	if (!drm_mode_create_content_type_property(connector->dev))
> +		drm_object_attach_property(&connector->base,
> +			connector->dev->mode_config.content_type_property,
> +			DRM_MODE_CONTENT_TYPE_GRAPHICS);
> +}

I think the "in" thing to do is to add this helper to the core, since this is a
core property.

Sean

> +
> -- 
> 2.17.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Stanislav Lisovskiy April 17, 2018, 9:21 a.m. UTC | #2
Hi Sean,

Thank you for comments! 
Could you please clarify a bit more here, as I've just
started recently working on drm side, so I took an
aspect ratio property as an example.

> @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  					   intel_hdmi->rgb_quant_range_selectable,
>  					   is_hdmi2_sink);
>  
> +	frame.avi.content_type = connector->state->content_type;
> +
>  	/* TODO: handle pixel repetition for YCBCR420 outputs */
>  	intel_write_infoframe(encoder, crtc_state, &frame);  } @@ -2065,7 
> +2067,9 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>  	intel_attach_force_audio_property(connector);
>  	intel_attach_broadcast_rgb_property(connector);
>  	intel_attach_aspect_ratio_property(connector);
> +	intel_attach_content_type_property(connector);
>  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +	connector->state->content_type = HDMI_CONTENT_TYPE_GRAPHICS;

>This is redudant, the attach function already sets this.

As you can see aspect ratio is set exactly same way,
which is also an HDMI avi info frame property. 

Also there are actually two different enums: HDMI_CONTENT_TYPE_*
and DRM_MODE_CONTENT_TYPE_* i.e:

there are one in drm_connector.c:

static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
       { DRM_MODE_PICTURE_ASPECT_NONE, "Automatic" },
       { DRM_MODE_PICTURE_ASPECT_4_3, "4:3" },
       { DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
};

so I added 

static const struct drm_prop_enum_list drm_content_type_enum_list[] = {
       { DRM_MODE_CONTENT_TYPE_GRAPHICS, "GRAPHICS" },
       { DRM_MODE_CONTENT_TYPE_PHOTO, "PHOTO" },
       { DRM_MODE_CONTENT_TYPE_CINEMA, "CINEMA" },
       { DRM_MODE_CONTENT_TYPE_GAME, "GAME" },
};

and the one in linux/hdmi.h:

enum hdmi_picture_aspect {
         HDMI_PICTURE_ASPECT_NONE,
         HDMI_PICTURE_ASPECT_4_3,
         HDMI_PICTURE_ASPECT_16_9,
         HDMI_PICTURE_ASPECT_64_27,
         HDMI_PICTURE_ASPECT_256_135,
         HDMI_PICTURE_ASPECT_RESERVED,
};

enum hdmi_content_type {
         HDMI_CONTENT_TYPE_GRAPHICS,
         HDMI_CONTENT_TYPE_PHOTO,
         HDMI_CONTENT_TYPE_CINEMA,
         HDMI_CONTENT_TYPE_GAME,
};

For some reason the latter enums are used in drm_connector_state, but not
the drm_content_type_enum_list(those are actually defined values which simply match):

From drm_connector.c:

/**
* @picture_aspect_ratio: Connector property to control the
* HDMI infoframe aspect ratio setting.
*
* The %DRM_MODE_PICTURE_ASPECT_\* values much match the
* values for &enum hdmi_picture_aspect
*/
enum hdmi_picture_aspect picture_aspect_ratio;

/**
* @content_type: Connector property to control the
* HDMI infoframe content type setting.
*
* The %DRM_MODE_CONTENT_TYPE_\* values much match the
* values for &enum hdmi_content_type
*/
enum hdmi_content_type   content_type;

That's why I did it exactly as it is done with aspect ratio. Just want to clarify,
as I was assuming this was done for reason. 

>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_modes.c 
> b/drivers/gpu/drm/i915/intel_modes.c
> index b39846613e3c..232811ab71a3 100644
> --- a/drivers/gpu/drm/i915/intel_modes.c
> +++ b/drivers/gpu/drm/i915/intel_modes.c
> @@ -133,3 +133,13 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
>  			connector->dev->mode_config.aspect_ratio_property,
>  			DRM_MODE_PICTURE_ASPECT_NONE);
>  }
> +
> +void
> +intel_attach_content_type_property(struct drm_connector *connector) {
> +	if (!drm_mode_create_content_type_property(connector->dev))
> +		drm_object_attach_property(&connector->base,
> +			connector->dev->mode_config.content_type_property,
> +			DRM_MODE_CONTENT_TYPE_GRAPHICS);
> +}

>I think the "in" thing to do is to add this helper to the core, since this is a core property.

Could you please explain a bit more, what do you mean by core here?

I just thought it is one of HDMI infoframe properties, as stated in spec:

https://www.hdmi.org/manufacturer/hdmi_1_4/content_type.aspx 

Best Regards,

Lisovskiy Stanislav
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 40285d1b91b7..61ddb5871d8a 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.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)
 		crtc_state->mode_changed = true;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5bd2263407b2..07fd7ba21f38 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1818,6 +1818,7 @@  int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 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_content_type_property(struct drm_connector *connector);
 
 
 /* intel_overlay.c */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee929f31f7db..cd484276e9b0 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -491,6 +491,8 @@  static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 					   intel_hdmi->rgb_quant_range_selectable,
 					   is_hdmi2_sink);
 
+	frame.avi.content_type = connector->state->content_type;
+
 	/* TODO: handle pixel repetition for YCBCR420 outputs */
 	intel_write_infoframe(encoder, crtc_state, &frame);
 }
@@ -2065,7 +2067,9 @@  intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
 	intel_attach_aspect_ratio_property(connector);
+	intel_attach_content_type_property(connector);
 	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+	connector->state->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index b39846613e3c..232811ab71a3 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -133,3 +133,13 @@  intel_attach_aspect_ratio_property(struct drm_connector *connector)
 			connector->dev->mode_config.aspect_ratio_property,
 			DRM_MODE_PICTURE_ASPECT_NONE);
 }
+
+void
+intel_attach_content_type_property(struct drm_connector *connector)
+{
+	if (!drm_mode_create_content_type_property(connector->dev))
+		drm_object_attach_property(&connector->base,
+			connector->dev->mode_config.content_type_property,
+			DRM_MODE_CONTENT_TYPE_GRAPHICS);
+}
+