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

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

 



On Wed, May 02, 2018 at 09:08:11AM +0000, Lisovskiy, Stanislav wrote:
> On Wed, 2018-05-02 at 10:15 +0200, Daniel Vetter wrote:
> > > 
> > On Wed, May 02, 2018 at 08:09:24AM +0000, Lisovskiy, Stanislav wrote:
> > > On Mon, 2018-04-30 at 17:00 +0200, Daniel Vetter wrote:
> > > > 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@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.
> > > > > > 
> > > > > > 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,	"Upsi
> > > > > > de
> > > > > > 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
> > > 
> > > So how should we proceed then? In fact those
> > > definitions(DRM_MODE_CONTENT_TYPE_*) are already in 
> > > the uapi header(I've added them in the first patch), done
> > > same way as aspect ratio:
> > > 
> > > * Picture aspect ratio options */
> > > #define DRM_MODE_PICTURE_ASPECT_NONE            0
> > > #define DRM_MODE_PICTURE_ASPECT_4_3             1
> > > #define DRM_MODE_PICTURE_ASPECT_16_9            2
> > 
> > This aren't for a property, but for flags in the drm_mode_modeinfo
> > structure. That means it's required to have them as #defines.
> 
> Ok, honestly I thought aspect ratio is also a property, because it is
> created along with other drm connector properties 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" },
> };
> 
> /**
>  * drm_mode_create_aspect_ratio_property - create aspect ratio 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_aspect_ratio_property(struct drm_device *dev)
> {
>        if (dev->mode_config.aspect_ratio_property)
>                return 0;
> 
>        dev->mode_config.aspect_ratio_property =
>                 drm_property_create_enum(dev, 0, "aspect ratio",
>                                drm_aspect_ratio_enum_list,
>                                ARRAY_SIZE(drm_aspect_ratio_enum_list));
> 
>         if (dev->mode_config.aspect_ratio_property == NULL)
>               return -ENOMEM;
> 
>         return 0;
> }
> 
> And basically as I see drm_atomic_connector_set_property can set it
> either. 

Oh, oops, I totally forgot that we have this. I guess we've been bad at
this a few times already. Other properties with hardcoded values are DPMS,
scaling and probably a few more.

So not sure what to do here. I guess you're current code is ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux