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

[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, &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