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.lisovs...@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.lisovs...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
