Re: [Intel-gfx] [PATCH v9 1/2] drm: content-type property for HDMI connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 08, 2018 at 07:18:20AM +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2018-05-07 at 16:05 +0200, Daniel Vetter wrote:
> > On Mon, May 07, 2018 at 04:16:40PM +0300, StanLis wrote:
> > > From: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > 
> > > Added content_type property to drm_connector_state
> > > in order to properly handle external HDMI TV content-type setting.
> > > 
> > > v2:
> > >  * Moved helper function which attaches content type property
> > >    to the drm core, as was suggested.
> > >    Removed redundant connector state initialization.
> > > 
> > > v3:
> > >  * Removed caps in drm_content_type_enum_list.
> > >    After some discussion it turned out that HDMI Spec 1.4
> > >    was wrongly assuming that IT Content(itc) bit doesn't affect
> > >    Content type states, however itc bit needs to be manupulated
> > >    as well. In order to not expose additional property for itc,
> > >    for sake of simplicity it was decided to bind those together
> > >    in same "content type" property.
> > > 
> > > v4:
> > >  * Added it_content checking in
> > > intel_digital_connector_atomic_check.
> > >    Fixed documentation for new content type enum.
> > > 
> > > v5:
> > >  * Moved patch revision's description to commit messages.
> > > 
> > > v6:
> > >  * Minor naming fix for the content type enumeration string.
> > > 
> > > v7:
> > >  * Fix parameter name for documentation and parameter alignment
> > >    in order not to get warning. Added Content Type description to
> > >    new HDMI connector properties section.
> > > 
> > > v8:
> > >  * Thrown away unneeded numbers from HDMI content-type property
> > >    description. Switch to strings desription instead of plain
> > >    definitions.
> > > 
> > > v9:
> > >  * Moved away hdmi specific content-type enum from
> > >    drm_connector_state. Content type property should probably not
> > >    be bound to any specific connector interface in
> > >    drm_connector_state.
> > >    Same probably should be done to hdmi_picture_aspect_ration enum
> > >    which is also contained in drm_connector_state. Added special
> > >    helper function to get derive hdmi specific relevant infoframe
> > >    fields.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > ---
> > >  Documentation/gpu/drm-kms.rst        |  6 +++
> > >  Documentation/gpu/kms-properties.csv |  1 +
> > >  drivers/gpu/drm/drm_atomic.c         |  4 ++
> > >  drivers/gpu/drm/drm_connector.c      | 74
> > > ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_edid.c           | 55 +++++++++++++++++++++
> > >  include/drm/drm_connector.h          | 12 +++++
> > >  include/drm/drm_edid.h               |  6 +++
> > >  include/drm/drm_mode_config.h        |  5 ++
> > >  include/uapi/drm/drm_mode.h          |  7 +++
> > >  9 files changed, 170 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-
> > > kms.rst
> > > index 1dffd1ac4cd4..e233c2626bd0 100644
> > > --- a/Documentation/gpu/drm-kms.rst
> > > +++ b/Documentation/gpu/drm-kms.rst
> > > @@ -517,6 +517,12 @@ Standard Connector Properties
> > >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > >     :doc: standard connector properties
> > >  
> > > +HDMI Specific Connector Properties
> > > +-----------------------------
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > +   :doc: HDMI connector properties
> > > +
> > >  Plane Composition Properties
> > >  ----------------------------
> > >  
> > > diff --git a/Documentation/gpu/kms-properties.csv
> > > b/Documentation/gpu/kms-properties.csv
> > > index 07ed22ea3bd6..bfde04eddd14 100644
> > > --- a/Documentation/gpu/kms-properties.csv
> > > +++ b/Documentation/gpu/kms-properties.csv
> > > @@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property
> > > Name,Type,Property Values,Object attached,De
> > >  ,Virtual GPU,“suggested X”,RANGE,"Min=0,
> > > Max=0xffffffff",Connector,property to suggest an X offset for a
> > > connector
> > >  ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property
> > > to suggest an Y offset for a connector
> > >  ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9""
> > > }",Connector,TDB
> > > +,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"",
> > > ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
> > >  i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"",
> > > ""Limited 16:235"" }",Connector,"When this property is set to
> > > Limited 16:235 and CTM is set, the hardware will be programmed with
> > > the result of the multiplication of CTM by the limited range matrix
> > > to ensure the pixels normaly in the range 0..1.0 are remapped to
> > > the range 16/255..235/255."
> > >  ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on""
> > > }",Connector,TBD
> > >  ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"",
> > > ""PAL_B"" } etc.",Connector,TBD
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > b/drivers/gpu/drm/drm_atomic.c
> > > index 3d9ae057a6cd..6c1e1e774517 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1270,6 +1270,8 @@ static int
> > > drm_atomic_connector_set_property(struct drm_connector *connector,
> > >  			state->link_status = val;
> > >  	} else if (property == config->aspect_ratio_property) {
> > >  		state->picture_aspect_ratio = val;
> > > +	} else if (property == config->content_type_property) {
> > > +		state->content_type = val;
> > >  	} else if (property == connector->scaling_mode_property) {
> > >  		state->scaling_mode = val;
> > >  	} else if (property == connector-
> > > >content_protection_property) {
> > > @@ -1355,6 +1357,8 @@ drm_atomic_connector_get_property(struct
> > > drm_connector *connector,
> > >  		*val = state->link_status;
> > >  	} else if (property == config->aspect_ratio_property) {
> > >  		*val = state->picture_aspect_ratio;
> > > +	} else if (property == config->content_type_property) {
> > > +		*val = state->content_type;
> > >  	} else if (property == connector->scaling_mode_property) {
> > >  		*val = state->scaling_mode;
> > >  	} else if (property == connector-
> > > >content_protection_property) {
> > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > b/drivers/gpu/drm/drm_connector.c
> > > index b3cde897cd80..fe63395f159d 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -720,6 +720,14 @@ static const struct drm_prop_enum_list
> > > drm_aspect_ratio_enum_list[] = {
> > >  	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
> > >  };
> > >  
> > > +static const struct drm_prop_enum_list
> > > drm_content_type_enum_list[] = {
> > > +	{ DRM_MODE_CONTENT_TYPE_NO_DATA, "No Data" },
> > > +	{ DRM_MODE_CONTENT_TYPE_GRAPHICS, "Graphics" },
> > > +	{ DRM_MODE_CONTENT_TYPE_PHOTO, "Photo" },
> > > +	{ DRM_MODE_CONTENT_TYPE_CINEMA, "Cinema" },
> > > +	{ DRM_MODE_CONTENT_TYPE_GAME, "Game" },
> > > +};
> > > +
> > >  static const struct drm_prop_enum_list
> > > drm_panel_orientation_enum_list[] = {
> > >  	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	
> > > },
> > >  	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside
> > > Down"	},
> > > @@ -996,6 +1004,45 @@ int drm_mode_create_dvi_i_properties(struct
> > > drm_device *dev)
> > >  }
> > >  EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
> > >  
> > > +
> > > +/**
> > > + * DOC: HDMI connector properties
> > > + *
> > > + * content type (HDMI specific):
> > > + *	Indicates content type setting to be used in HDMI
> > > infoframes to indicate
> > > + *	content type for the external device, so that it adjusts
> > > it's display
> > > + *	settings accordingly.
> > > + *
> > > + *	The value of this property can be one of the following:
> > > + *
> > > + *	No Data:
> > > + *		Content type is unknown
> > > + *	Graphics:
> > > + *		Content type is graphics
> > > + *	Photo:
> > > + *		Content type is photo
> > > + *	Cinema:
> > > + *		Content type is cinema
> > > + *	Game:
> > > + *		Content type is game
> > 
> > Links to the function that set up/use the property would be good
> > here.
> > I.e.
> > 
> > 	"Drivers can set up this property by calling
> > 	drm_connector_attach_content_type_property(). Decoding to
> > 	infoframe values is done through
> > 	drm_hdmi_get_content_type_from_property() and
> > 	drm_hdmi_get_itc_bit_from_property()."
> > 
> > Aside: A simple helper that computes the correct infoframes from the
> > drm_connector_state and drm_display_info, without forcing drivers to
> > call
> > a gazillion individual helper functions would be real nice. But
> > that's for
> > someone else I think - all the verbosity here simply annoys me a bit.
> > 
> > With the above kerneldoc polish:
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > -Daniel
> 
> Yep, I suggested that, adding some single function could be more proper
> approach - there is even already a function called
> drm_hdmi_avi_infoframe_from_display_mode, I initially suggested that
> it would be convenient to add it there, however as I understood from
> discussion with Ville, not all drivers support atomic properties,
> otherwise certainly it would be much easier to have it in a single
> function.

Yes not everything is atomic, but we can pretty much require atomic for
new drivers. We'd need to make a new function though which takes
drm_connector_state into account (so that non-atomic drivers can keep
calling drm_hdmi_avi_infoframe_from_display_mode). For that overall
function we'd probably want:

drm_hdmi_avi_infoframe_from_state(struct hdmi_avi_infoframe *frame,
				  struct drm_connector_state *conn_state,
				  struct drm_crtc_state *crtc_state);

display_info we can get at through conn_state->connector.display_info, and
mode we can reach through crtc_state->adjusted_mode.

But yeah probably better to do that with the next little hdmi infoframe
thing we're going to support, or as a follow-up.
-Daniel

> 
> > > + */
> > > +
> > > +/**
> > > + * drm_connector_attach_content_type_property - attach content-
> > > type property
> > > + * @connector: connector to attach content type property on.
> > > + *
> > > + * Called by a driver the first time a HDMI connector is made.
> > > + */
> > > +int drm_connector_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_N
> > > O_DATA);
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_attach_content_type_property);
> > > +
> > >  /**
> > >   * drm_create_tv_properties - create TV specific connector
> > > properties
> > >   * @dev: DRM device
> > > @@ -1260,6 +1307,33 @@ int
> > > drm_mode_create_aspect_ratio_property(struct drm_device *dev)
> > >  }
> > >  EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> > >  
> > > +/**
> > > + * drm_mode_create_content_type_property - create content type
> > > property
> > > + * @dev: DRM device
> > > + *
> > > + * Called by a driver the first time it's needed, must be attached
> > > to desired
> > > + * connectors.
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative errno on failure.
> > > + */
> > > +int drm_mode_create_content_type_property(struct drm_device *dev)
> > > +{
> > > +	if (dev->mode_config.content_type_property)
> > > +		return 0;
> > > +
> > > +	dev->mode_config.content_type_property =
> > > +		drm_property_create_enum(dev, 0, "content type",
> > > +					 drm_content_type_enum_lis
> > > t,
> > > +					 ARRAY_SIZE(drm_content_ty
> > > pe_enum_list));
> > > +
> > > +	if (dev->mode_config.content_type_property == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_mode_create_content_type_property);
> > 
> > Do we really need to export this? Drivers should only call
> > drm_connector_attach_content_type_property() I think ...
> 
> All of those property creation functions are exported, I was 
> just following those as example:
> 
> EXPORT_SYMBOL(drm_mode_create_tv_properties);
> EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
> 
> and so on. 
> 
> > 
> > > +
> > >  /**
> > >   * drm_mode_create_suggested_offset_properties - create suggests
> > > offset properties
> > >   * @dev: DRM device
> > > diff --git a/drivers/gpu/drm/drm_edid.c
> > > b/drivers/gpu/drm/drm_edid.c
> > > index 134069f36482..9ecda0b2a4d8 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4868,6 +4868,14 @@
> > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >  
> > >  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > >  
> > > +	/*
> > > +	 * As some drivers don't support atomic, we can't use
> > > connector state.
> > > +	 * So just initialize the frame with default values, just
> > > the same way
> > > +	 * as it's done with other properties here.
> > > +	 */
> > > +	frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
> > > +	frame->itc = 0;
> > > +
> > >  	/*
> > >  	 * Populate picture aspect ratio from either
> > >  	 * user input (if specified) or from the CEA mode list.
> > > @@ -4886,6 +4894,53 @@
> > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >  }
> > >  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
> > >  
> > > +/**
> > > + * drm_hdmi_get_itc_bit_from_property() - get the HDMI IT content
> > > bit
> > > + *                                        from content type
> > > property.
> > > + * content_type: Content type DRM property value
> > > + *
> > > + */
> > > +bool
> > > +drm_hdmi_get_itc_bit_from_property(unsigned int content_type)
> > > +{
> > > +	/* Whenever something else than "No Data", IT Content bit
> > > must be set */
> > > +	return content_type != DRM_MODE_CONTENT_TYPE_NO_DATA ?
> > > true : false;
> > > +}
> > > +EXPORT_SYMBOL(drm_hdmi_get_itc_bit_from_property);
> > > +
> > > +/**
> > > + * drm_hdmi_get_content_type_from_property() - get the HDMI
> > > content type bits
> > > + *                                             from content type
> > > property.
> > > + * content_type: Content type DRM property value
> > > + *
> > > + */
> > > +enum hdmi_content_type
> > > +drm_hdmi_get_content_type_from_property(unsigned int content_type)
> > > +{
> > > +	enum hdmi_content_type ret;
> > > +
> > > +	switch (content_type) {
> > > +	case DRM_MODE_CONTENT_TYPE_GRAPHICS:
> > > +		ret = HDMI_CONTENT_TYPE_GRAPHICS;
> > > +		break;
> > > +	case DRM_MODE_CONTENT_TYPE_CINEMA:
> > > +		ret = HDMI_CONTENT_TYPE_CINEMA;
> > > +		break;
> > > +	case DRM_MODE_CONTENT_TYPE_GAME:
> > > +		ret = HDMI_CONTENT_TYPE_GAME;
> > > +		break;
> > > +	case DRM_MODE_CONTENT_TYPE_PHOTO:
> > > +		ret = HDMI_CONTENT_TYPE_PHOTO;
> > > +		break;
> > > +	default:
> > > +		/* Graphics is the default(0) */
> > > +		ret = HDMI_CONTENT_TYPE_GRAPHICS;
> > > +	}
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_hdmi_get_content_type_from_property);
> > > +
> > > +
> > >  /**
> > >   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI
> > > infoframe
> > >   *                                        quantization range
> > > information
> > > diff --git a/include/drm/drm_connector.h
> > > b/include/drm/drm_connector.h
> > > index 675cc3f8cf85..178fa3f3f5f7 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -418,6 +418,16 @@ struct drm_connector_state {
> > >  	 */
> > >  	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.
> > > +	 */
> > > +	unsigned int content_type;
> > > +
> > > +
> > >  	/**
> > >  	 * @scaling_mode: Connector property to control the
> > >  	 * upscaling, mostly used for built-in panels.
> > > @@ -1089,11 +1099,13 @@ int drm_mode_create_tv_properties(struct
> > > drm_device *dev,
> > >  				  unsigned int num_modes,
> > >  				  const char * const modes[]);
> > >  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> > > +int drm_connector_attach_content_type_property(struct
> > > drm_connector *dev);
> > >  int drm_connector_attach_scaling_mode_property(struct
> > > drm_connector *connector,
> > >  					       u32
> > > scaling_mode_mask);
> > >  int drm_connector_attach_content_protection_property(
> > >  		struct drm_connector *connector);
> > >  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> > > +int drm_mode_create_content_type_property(struct drm_device *dev);
> > >  int drm_mode_create_suggested_offset_properties(struct drm_device
> > > *dev);
> > >  
> > >  int drm_mode_connector_set_path_property(struct drm_connector
> > > *connector,
> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > > index 8d89a9c3748d..4e264c4d8a9b 100644
> > > --- a/include/drm/drm_edid.h
> > > +++ b/include/drm/drm_edid.h
> > > @@ -350,6 +350,12 @@ drm_load_edid_firmware(struct drm_connector
> > > *connector)
> > >  }
> > >  #endif
> > >  
> > > +bool
> > > +drm_hdmi_get_itc_bit_from_property(unsigned int content_type);
> > > +
> > > +enum hdmi_content_type
> > > +drm_hdmi_get_content_type_from_property(unsigned int
> > > content_type);
> > > +
> > >  int
> > >  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >  					 const struct
> > > drm_display_mode *mode,
> > > diff --git a/include/drm/drm_mode_config.h
> > > b/include/drm/drm_mode_config.h
> > > index 33b3a96d66d0..fb45839179dd 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -726,6 +726,11 @@ struct drm_mode_config {
> > >  	 * HDMI infoframe aspect ratio setting.
> > >  	 */
> > >  	struct drm_property *aspect_ratio_property;
> > > +	/**
> > > +	 * @content_type_property: Optional connector property to
> > > control the
> > > +	 * HDMI infoframe content type setting.
> > > +	 */
> > > +	struct drm_property *content_type_property;
> > >  	/**
> > >  	 * @degamma_lut_property: Optional CRTC property to set
> > > the LUT used to
> > >  	 * convert the framebuffer's colors to linear gamma.
> > > diff --git a/include/uapi/drm/drm_mode.h
> > > b/include/uapi/drm/drm_mode.h
> > > index 50bcf4214ff9..cad9e09ffaee 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -94,6 +94,13 @@ extern "C" {
> > >  #define DRM_MODE_PICTURE_ASPECT_4_3		1
> > >  #define DRM_MODE_PICTURE_ASPECT_16_9		2
> > >  
> > > +/* Content type options */
> > > +#define DRM_MODE_CONTENT_TYPE_NO_DATA		0
> > > +#define DRM_MODE_CONTENT_TYPE_GRAPHICS		1
> > > +#define DRM_MODE_CONTENT_TYPE_PHOTO		2
> > > +#define DRM_MODE_CONTENT_TYPE_CINEMA		3
> > > +#define DRM_MODE_CONTENT_TYPE_GAME		4
> > > +
> > >  /* Aspect ratio flag bitmask (4 bits 22:19) */
> > >  #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> > >  #define  DRM_MODE_FLAG_PIC_AR_NONE \
> > > -- 
> > > 2.17.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> -- 
> Best Regards,
> 
> Lisovskiy Stanislav

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux