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

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@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.

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@intel.com

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@ffwll.ch -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.

...

• */

+/**

• • 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx