RE: [Intel-gfx] [PATCH v1 2/2] i915: content-type property for HDMI connector

2018-04-17 Thread Lisovskiy, Stanislav 
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, );  } @@ -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  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  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(>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



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v1 2/2] i915: content-type property for HDMI connector

2018-04-16 Thread Sean Paul 
On Tue, Apr 17, 2018 at 02:16:59AM +0300, StanLis wrote:
> From: Stanislav Lisovskiy 
> 
> 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 

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, );
>  }
> @@ -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(>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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel