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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 27, 2018 at 10:40:00PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 23, 2018 at 10:34:41AM +0300, StanLis wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > 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.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  Documentation/gpu/drm-kms.rst        |  6 +++
> >  Documentation/gpu/kms-properties.csv |  1 +
> >  drivers/gpu/drm/drm_atomic.c         | 17 +++++++
> >  drivers/gpu/drm/drm_connector.c      | 74 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_edid.c           |  2 +
> >  include/drm/drm_connector.h          | 18 +++++++
> >  include/drm/drm_mode_config.h        |  5 ++
> >  include/uapi/drm/drm_mode.h          |  7 +++
> >  8 files changed, 130 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 6b28b014cb7d..3567c986bd7d 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 7d25c42f22db..479499f5848e 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1266,6 +1266,15 @@ 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) {
> > +		/*
> > +		 * Lowest two bits of content_type property control
> > +		 * content_type, bit 2 controls itc bit.
> > +		 * It was decided to have a single property called
> > +		 * content_type, instead of content_type and itc.
> > +		 */
> > +		state->content_type = val & 3;
> > +		state->it_content = val >> 2;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		state->scaling_mode = val;
> >  	} else if (property == connector->content_protection_property) {
> > @@ -1351,6 +1360,14 @@ 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) {
> > +		/*
> > +		 * Lowest two bits of content_type property control
> > +		 * content_type, bit 2 controls itc bit.
> > +		 * It was decided to have a single property called
> > +		 * content_type, instead of content_type and itc.
> > +		 */
> > +		*val = state->content_type | (state->it_content << 2);
> >  	} 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..4f89602ebaf0 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:
> > + *
> > + *	- DRM_MODE_CONTENT_TYPE_NO_DATA = 0
> > + *		Content type is unknown, it content(itc) bit is cleared.
> > + *	- DRM_MODE_CONTENT_TYPE_GRAPHICS = 4
> > + *		Content type is graphics, it content(itc) bit is set.
> > + *	- DRM_MODE_CONTENT_TYPE_PHOTO = 5
> > + *		Content type is photo, itc bit is set.
> > + *	- DRM_MODE_CONTENT_TYPE_CINEMA = 6
> > + *		Content type is cinema, itc bit is set.
> > + *	- DRM_MODE_CONTENT_TYPE_GAME = 7
> > + *		Content type is game, itc bit is set.
> 
> I wonder if we're trying to document the uabi or the internals here.
> If we are interested in the uabi, then we should document the enum
> value strings here. If on the other hand we're trying to document the
> internal details then we should keep the DRM_CONTENT_TYPE_* stuff.
> Maybe we want both? The raw numbers I think we should just throw out.
> While they do have some specific meaning in the case (matching the bits
> in the infoframe), I'm not sure that's important enough to include in
> the docs. We could add a comment next to the DRM_MODE_CONTENT_TYPE_*
> definitions.
> 
> Looks like the content protection prop is documenting the internals only
> as well. Hmm. Actually it looks like those things are defined in the uapi
> header. Oh and the scaling mode prop also does that. This is all setting
> a rather bad example for userspace. Or maybe we've given up on the "the
> string is the uabi" notion entirely?

Wrt documenting uapi: That should imo also be in there, but I realize it
makes it a bit a mess. The kerneldoc should definitely align with other
property docs to make sure it all looks consistent (i.e. enumeration list
with the same indentation as all the other properties).

I guess it'd be good if we can aim for "the string is the uabi", but in
practice people will hardcode. For cases where this is likely I think
having the defines in the uapi header is probably better.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel