* [PATCH v7 0/2] Enabling content-type setting for HDMI displays.
@ 2018-04-23 7:34 StanLis
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: StanLis @ 2018-04-23 7:34 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Added content type setting property to drm_connector(part 1)
and enabled transmitting it with HDMI AVI infoframes
for i915(part 2).
Stanislav Lisovskiy (2):
drm: content-type property for HDMI connector
i915: content-type property for HDMI connector
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 +
drivers/gpu/drm/i915/intel_atomic.c | 2 +
drivers/gpu/drm/i915/intel_hdmi.c | 4 ++
include/drm/drm_connector.h | 18 +++++++
include/drm/drm_mode_config.h | 5 ++
include/uapi/drm/drm_mode.h | 7 +++
10 files changed, 136 insertions(+)
--
2.17.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
@ 2018-04-23 7:34 ` StanLis
2018-04-23 13:45 ` Lisovskiy, Stanislav
2018-04-27 19:40 ` Ville Syrjälä
2018-04-23 7:34 ` [PATCH v7 2/2] i915: " StanLis
` (4 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: StanLis @ 2018-04-23 7:34 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
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.
+ */
+
+/**
+ * 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_NO_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_list,
+ ARRAY_SIZE(drm_content_type_enum_list));
+
+ if (dev->mode_config.content_type_property == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_content_type_property);
+
/**
* 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..b1b7f9683e34 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4867,6 +4867,8 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
}
frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
+ frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
+ frame->itc = 0;
/*
* Populate picture aspect ratio from either
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 675cc3f8cf85..1fedab750f09 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -418,6 +418,22 @@ 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 lowest 2 bits much
+ * match the values for &enum hdmi_content_type
+ */
+ enum hdmi_content_type content_type;
+
+ /**
+ * @it_content: Connector property to control the
+ * HDMI infoframe it content setting(used with content type).
+ * The %DRM_MODE_CONTENT_TYPE_\* values bit 2
+ * match the values of it_content
+ */
+ bool it_content;
+
/**
* @scaling_mode: Connector property to control the
* upscaling, mostly used for built-in panels.
@@ -1089,11 +1105,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_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..3c234bfa80b9 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
+/* HDMI content type and itc bit bound together for simplicity */
+#define DRM_MODE_CONTENT_TYPE_NO_DATA 0
+#define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
+#define DRM_MODE_CONTENT_TYPE_PHOTO 5
+#define DRM_MODE_CONTENT_TYPE_CINEMA 6
+#define DRM_MODE_CONTENT_TYPE_GAME 7
+
/* 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v7 2/2] i915: content-type property for HDMI connector
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
@ 2018-04-23 7:34 ` StanLis
2018-04-23 7:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enabling content-type setting for HDMI displays. (rev6) Patchwork
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: StanLis @ 2018-04-23 7:34 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Added encoding of drm content_type property from drm_connector_state
within AVI infoframe in order to properly handle external HDMI TV
content-type setting.
This requires also manipulationg ITC bit, as stated in
HDMI spec.
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>
---
drivers/gpu/drm/i915/intel_atomic.c | 2 ++
drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 40285d1b91b7..96a42eb457c5 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -124,6 +124,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
if (new_conn_state->force_audio != old_conn_state->force_audio ||
new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb ||
new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
+ new_conn_state->base.content_type != old_conn_state->base.content_type ||
+ new_conn_state->base.it_content != old_conn_state->base.it_content ||
new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode)
crtc_state->mode_changed = true;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee929f31f7db..078200908b7a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -491,6 +491,9 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
intel_hdmi->rgb_quant_range_selectable,
is_hdmi2_sink);
+ frame.avi.content_type = connector->state->content_type;
+ frame.avi.itc = connector->state->it_content;
+
/* TODO: handle pixel repetition for YCBCR420 outputs */
intel_write_infoframe(encoder, crtc_state, &frame);
}
@@ -2065,6 +2068,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
intel_attach_force_audio_property(connector);
intel_attach_broadcast_rgb_property(connector);
intel_attach_aspect_ratio_property(connector);
+ drm_connector_attach_content_type_property(connector);
connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
}
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for Enabling content-type setting for HDMI displays. (rev6)
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
2018-04-23 7:34 ` [PATCH v7 2/2] i915: " StanLis
@ 2018-04-23 7:50 ` Patchwork
2018-04-23 8:04 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-04-23 7:50 UTC (permalink / raw)
To: StanLis; +Cc: intel-gfx
== Series Details ==
Series: Enabling content-type setting for HDMI displays. (rev6)
URL : https://patchwork.freedesktop.org/series/41876/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
f891f01659af drm: content-type property for HDMI connector
-:127: CHECK:LINE_SPACING: Please don't use multiple blank lines
#127: FILE: drivers/gpu/drm/drm_connector.c:1007:
+
-:193: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "!dev->mode_config.content_type_property"
#193: FILE: drivers/gpu/drm/drm_connector.c:1330:
+ if (dev->mode_config.content_type_property == NULL)
total: 0 errors, 0 warnings, 2 checks, 207 lines checked
723a0fb27d74 i915: content-type property for HDMI connector
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✓ Fi.CI.BAT: success for Enabling content-type setting for HDMI displays. (rev6)
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
` (2 preceding siblings ...)
2018-04-23 7:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enabling content-type setting for HDMI displays. (rev6) Patchwork
@ 2018-04-23 8:04 ` Patchwork
2018-04-23 9:33 ` ✓ Fi.CI.IGT: " Patchwork
2018-04-25 8:02 ` [PATCH v7 0/2] Enabling content-type setting for HDMI displays Lisovskiy, Stanislav
5 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-04-23 8:04 UTC (permalink / raw)
To: StanLis; +Cc: intel-gfx
== Series Details ==
Series: Enabling content-type setting for HDMI displays. (rev6)
URL : https://patchwork.freedesktop.org/series/41876/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4075 -> Patchwork_8770 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/41876/revisions/6/mbox/
== Known issues ==
Here are the changes found in Patchwork_8770 that come from known issues:
=== IGT changes ===
==== Possible fixes ====
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
fi-snb-2520m: INCOMPLETE (fdo#103713) -> PASS
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: FAIL (fdo#104008) -> PASS
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
== Participating hosts (34 -> 32) ==
Missing (2): fi-ilk-m540 fi-skl-6700hq
== Build changes ==
* Linux: CI_DRM_4075 -> Patchwork_8770
CI_DRM_4075: 75c0982514374248e1fc0866c53180d2be590310 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4443: 8168bb65d5e64d4df4e5d847d448bab2d2825d73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8770: 723a0fb27d74183077a9aa38ccdc1c92520c6d9e @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4443: a2f486679f467cd6e82578384f56d4aabaa8cf2e @ git://anongit.freedesktop.org/piglit
== Linux commits ==
723a0fb27d74 i915: content-type property for HDMI connector
f891f01659af drm: content-type property for HDMI connector
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8770/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✓ Fi.CI.IGT: success for Enabling content-type setting for HDMI displays. (rev6)
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
` (3 preceding siblings ...)
2018-04-23 8:04 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-04-23 9:33 ` Patchwork
2018-04-25 8:02 ` [PATCH v7 0/2] Enabling content-type setting for HDMI displays Lisovskiy, Stanislav
5 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-04-23 9:33 UTC (permalink / raw)
To: StanLis; +Cc: intel-gfx
== Series Details ==
Series: Enabling content-type setting for HDMI displays. (rev6)
URL : https://patchwork.freedesktop.org/series/41876/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4075_full -> Patchwork_8770_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_8770_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_8770_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/41876/revisions/6/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_8770_full:
=== IGT changes ===
==== Warnings ====
igt@gem_exec_schedule@deep-bsd2:
shard-kbl: PASS -> SKIP +1
igt@kms_cursor_crc@cursor-256x256-random:
shard-glk: PASS -> SKIP +63
igt@kms_vblank@pipe-c-wait-forked-busy:
shard-glk: SKIP -> PASS +107
== Known issues ==
Here are the changes found in Patchwork_8770_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_pwrite_pread@display-pwrite-blt-gtt_mmap-performance:
shard-kbl: PASS -> DMESG-WARN (fdo#103558, fdo#105602)
igt@kms_flip@2x-flip-vs-expired-vblank:
shard-hsw: PASS -> FAIL (fdo#102887)
igt@kms_flip@2x-plain-flip-ts-check:
shard-hsw: PASS -> FAIL (fdo#103928)
igt@kms_flip@wf_vblank-ts-check-interruptible:
shard-glk: SKIP -> FAIL (fdo#100368)
igt@kms_setmode@basic:
shard-glk: PASS -> FAIL (fdo#99912)
shard-kbl: PASS -> FAIL (fdo#99912)
==== Possible fixes ====
igt@kms_flip@2x-plain-flip-ts-check-interruptible:
shard-hsw: FAIL (fdo#100368) -> PASS +1
igt@kms_flip@absolute-wf_vblank-interruptible:
shard-glk: FAIL (fdo#106087) -> PASS
igt@kms_flip@flip-vs-absolute-wf_vblank-interruptible:
shard-glk: FAIL (fdo#100368) -> PASS
igt@kms_flip@modeset-vs-vblank-race:
shard-hsw: FAIL (fdo#103060) -> PASS
igt@kms_sysfs_edid_timing:
shard-apl: WARN (fdo#100047) -> PASS
igt@kms_vblank@pipe-c-ts-continuation-suspend:
shard-kbl: DMESG-WARN (fdo#103558, fdo#105602) -> PASS +1
==== Warnings ====
igt@kms_sysfs_edid_timing:
shard-glk: WARN (fdo#100047) -> FAIL (fdo#100047)
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (6 -> 5) ==
Missing (1): shard-glkb
== Build changes ==
* Linux: CI_DRM_4075 -> Patchwork_8770
CI_DRM_4075: 75c0982514374248e1fc0866c53180d2be590310 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4443: 8168bb65d5e64d4df4e5d847d448bab2d2825d73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8770: 723a0fb27d74183077a9aa38ccdc1c92520c6d9e @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4443: a2f486679f467cd6e82578384f56d4aabaa8cf2e @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8770/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
@ 2018-04-23 13:45 ` Lisovskiy, Stanislav
2018-04-27 10:42 ` [Intel-gfx] " Lisovskiy, Stanislav
2018-04-27 19:40 ` Ville Syrjälä
1 sibling, 1 reply; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-04-23 13:45 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Ping
________________________________________
From: Intel-gfx [intel-gfx-bounces@lists.freedesktop.org] on behalf of StanLis [stanislav.lisovskiy@intel.com]
Sent: Monday, April 23, 2018 10:34 AM
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v7 1/2] drm: content-type property for HDMI connector
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.
+ */
+
+/**
+ * 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_NO_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_list,
+ ARRAY_SIZE(drm_content_type_enum_list));
+
+ if (dev->mode_config.content_type_property == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_content_type_property);
+
/**
* 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..b1b7f9683e34 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4867,6 +4867,8 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
}
frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
+ frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
+ frame->itc = 0;
/*
* Populate picture aspect ratio from either
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 675cc3f8cf85..1fedab750f09 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -418,6 +418,22 @@ 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 lowest 2 bits much
+ * match the values for &enum hdmi_content_type
+ */
+ enum hdmi_content_type content_type;
+
+ /**
+ * @it_content: Connector property to control the
+ * HDMI infoframe it content setting(used with content type).
+ * The %DRM_MODE_CONTENT_TYPE_\* values bit 2
+ * match the values of it_content
+ */
+ bool it_content;
+
/**
* @scaling_mode: Connector property to control the
* upscaling, mostly used for built-in panels.
@@ -1089,11 +1105,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_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..3c234bfa80b9 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
+/* HDMI content type and itc bit bound together for simplicity */
+#define DRM_MODE_CONTENT_TYPE_NO_DATA 0
+#define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
+#define DRM_MODE_CONTENT_TYPE_PHOTO 5
+#define DRM_MODE_CONTENT_TYPE_CINEMA 6
+#define DRM_MODE_CONTENT_TYPE_GAME 7
+
/* 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v7 0/2] Enabling content-type setting for HDMI displays.
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
` (4 preceding siblings ...)
2018-04-23 9:33 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-04-25 8:02 ` Lisovskiy, Stanislav
5 siblings, 0 replies; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-04-25 8:02 UTC (permalink / raw)
To: Daniel Vetter, dri-devel; +Cc: intel-gfx
Hi Daniel,
Could you please check changes in the documentation,
I've added a new HDMI connector properties section, based on our discussion.
I think now other HDMI specific properties can be transferred there as well.
Best Regards,
Lisovskiy Stanislav
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
________________________________________
From: dri-devel [dri-devel-bounces@lists.freedesktop.org] on behalf of StanLis [stanislav.lisovskiy@intel.com]
Sent: Monday, April 23, 2018 10:34 AM
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Subject: [PATCH v7 0/2] Enabling content-type setting for HDMI displays.
From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Added content type setting property to drm_connector(part 1)
and enabled transmitting it with HDMI AVI infoframes
for i915(part 2).
Stanislav Lisovskiy (2):
drm: content-type property for HDMI connector
i915: content-type property for HDMI connector
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 +
drivers/gpu/drm/i915/intel_atomic.c | 2 +
drivers/gpu/drm/i915/intel_hdmi.c | 4 ++
include/drm/drm_connector.h | 18 +++++++
include/drm/drm_mode_config.h | 5 ++
include/uapi/drm/drm_mode.h | 7 +++
10 files changed, 136 insertions(+)
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Intel-gfx] [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-23 13:45 ` Lisovskiy, Stanislav
@ 2018-04-27 10:42 ` Lisovskiy, Stanislav
0 siblings, 0 replies; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-04-27 10:42 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Ping.
On Mon, 2018-04-23 at 13:45 +0000, Lisovskiy, Stanislav wrote:
> Ping
> ________________________________________
> From: Intel-gfx [intel-gfx-bounces@lists.freedesktop.org] on behalf
> of StanLis [stanislav.lisovskiy@intel.com]
> Sent: Monday, April 23, 2018 10:34 AM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v7 1/2] drm: content-type property for
> HDMI connector
>
> 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.
> + */
> +
> +/**
> + * 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_NO_D
> ATA);
> + 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_list,
> + ARRAY_SIZE(drm_content_type_
> enum_list));
> +
> + if (dev->mode_config.content_type_property == NULL)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_content_type_property);
> +
> /**
> * 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..b1b7f9683e34 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4867,6 +4867,8 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> hdmi_avi_infoframe *frame,
> }
>
> frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> + frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
> + frame->itc = 0;
>
> /*
> * Populate picture aspect ratio from either
> diff --git a/include/drm/drm_connector.h
> b/include/drm/drm_connector.h
> index 675cc3f8cf85..1fedab750f09 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -418,6 +418,22 @@ 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 lowest 2 bits much
> + * match the values for &enum hdmi_content_type
> + */
> + enum hdmi_content_type content_type;
> +
> + /**
> + * @it_content: Connector property to control the
> + * HDMI infoframe it content setting(used with content type).
> + * The %DRM_MODE_CONTENT_TYPE_\* values bit 2
> + * match the values of it_content
> + */
> + bool it_content;
> +
> /**
> * @scaling_mode: Connector property to control the
> * upscaling, mostly used for built-in panels.
> @@ -1089,11 +1105,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_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..3c234bfa80b9 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
>
> +/* HDMI content type and itc bit bound together for simplicity */
> +#define DRM_MODE_CONTENT_TYPE_NO_DATA 0
> +#define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
> +#define DRM_MODE_CONTENT_TYPE_PHOTO 5
> +#define DRM_MODE_CONTENT_TYPE_CINEMA 6
> +#define DRM_MODE_CONTENT_TYPE_GAME 7
> +
> /* 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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
2018-04-23 13:45 ` Lisovskiy, Stanislav
@ 2018-04-27 19:40 ` Ville Syrjälä
2018-04-30 6:28 ` Lisovskiy, Stanislav
2018-04-30 15:00 ` Daniel Vetter
1 sibling, 2 replies; 17+ messages in thread
From: Ville Syrjälä @ 2018-04-27 19:40 UTC (permalink / raw)
To: StanLis; +Cc: Daniel Vetter, intel-gfx, dri-devel
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?
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-27 19:40 ` Ville Syrjälä
@ 2018-04-30 6:28 ` Lisovskiy, Stanislav
2018-04-30 15:00 ` Daniel Vetter
1 sibling, 0 replies; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-04-30 6:28 UTC (permalink / raw)
To: ville.syrjala; +Cc: daniel.vetter, intel-gfx, dri-devel
On Fri, 2018-04-27 at 22:40 +0300, Ville Syrjälä wrote:
> > +
> > +/**
> > + * 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?
>
I can just throw away the numbers from that doc, but add a bit of more
clarification in the definition itself. If this is fine with everybody.
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-27 19:40 ` Ville Syrjälä
2018-04-30 6:28 ` Lisovskiy, Stanislav
@ 2018-04-30 15:00 ` Daniel Vetter
2018-05-02 8:09 ` Lisovskiy, Stanislav
1 sibling, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2018-04-30 15:00 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: StanLis, Daniel Vetter, intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-04-30 15:00 ` Daniel Vetter
@ 2018-05-02 8:09 ` Lisovskiy, Stanislav
2018-05-02 8:15 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-05-02 8:09 UTC (permalink / raw)
To: daniel, ville.syrjala; +Cc: daniel.vetter, intel-gfx, dri-devel
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@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
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
/* HDMI content type and itc bit bound together for simplicity */
#define DRM_MODE_CONTENT_TYPE_NO_DATA 0
#define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
#define DRM_MODE_CONTENT_TYPE_PHOTO 5
#define DRM_MODE_CONTENT_TYPE_CINEMA 6
#define DRM_MODE_CONTENT_TYPE_GAME 7
Should I just throw out numbers from kernel doc for drm_connector.c? Or
may be add the enum string values? The thing is that other properties
documentation seems to be done in different manner, for example
"content protection" documents the defined enum values, while "scaling
mode" documents the strings themself.
So I would be happy to implement it correctly, however just need some
clue or hint here :)
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-05-02 8:09 ` Lisovskiy, Stanislav
@ 2018-05-02 8:15 ` Daniel Vetter
2018-05-02 9:08 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2018-05-02 8:15 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: daniel.vetter, dri-devel, intel-gfx
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@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
>
> 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.
>
> /* HDMI content type and itc bit bound together for simplicity */
> #define DRM_MODE_CONTENT_TYPE_NO_DATA 0
> #define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
> #define DRM_MODE_CONTENT_TYPE_PHOTO 5
> #define DRM_MODE_CONTENT_TYPE_CINEMA 6
> #define DRM_MODE_CONTENT_TYPE_GAME 7
These hare are for properties, which means they're not really required
since we have a getproperties ioctl to figure them out from the string.
> Should I just throw out numbers from kernel doc for drm_connector.c? Or
> may be add the enum string values? The thing is that other properties
> documentation seems to be done in different manner, for example
> "content protection" documents the defined enum values, while "scaling
> mode" documents the strings themself.
Yeah that's what Ville pointed out, we already screwed up a bit with the
content protection prop. For props, the string should be the uapi (but
we'll probably have to deal with userspace hardcoding stuff in some cases
no matter what).
> So I would be happy to implement it correctly, however just need some
> clue or hint here :)
I hope the above helps in clarifying things - aspect ratio ain't a
property.
-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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-05-02 8:15 ` Daniel Vetter
@ 2018-05-02 9:08 ` Lisovskiy, Stanislav
2018-05-02 11:08 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-05-02 9:08 UTC (permalink / raw)
To: daniel; +Cc: daniel.vetter, intel-gfx, dri-devel
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@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, "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.
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-05-02 9:08 ` Lisovskiy, Stanislav
@ 2018-05-02 11:08 ` Daniel Vetter
2018-05-02 11:19 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2018-05-02 11:08 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: daniel.vetter, dri-devel, intel-gfx
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@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, "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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v7 1/2] drm: content-type property for HDMI connector
2018-05-02 11:08 ` Daniel Vetter
@ 2018-05-02 11:19 ` Lisovskiy, Stanislav
0 siblings, 0 replies; 17+ messages in thread
From: Lisovskiy, Stanislav @ 2018-05-02 11:19 UTC (permalink / raw)
To: daniel; +Cc: daniel.vetter, intel-gfx, dri-devel
On Wed, 2018-05-02 at 13:08 +0200, Daniel Vetter wrote:
> 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@intel.com>
> > > > > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * 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_lis
> > t));
> >
> > 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
Ok, but anyway good to know how it should be at least.
I guess anyway I need to throw away numbers and move to strings
instead of enum definitions.
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-05-02 11:19 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-23 7:34 [PATCH v7 0/2] Enabling content-type setting for HDMI displays StanLis
2018-04-23 7:34 ` [PATCH v7 1/2] drm: content-type property for HDMI connector StanLis
2018-04-23 13:45 ` Lisovskiy, Stanislav
2018-04-27 10:42 ` [Intel-gfx] " Lisovskiy, Stanislav
2018-04-27 19:40 ` Ville Syrjälä
2018-04-30 6:28 ` Lisovskiy, Stanislav
2018-04-30 15:00 ` Daniel Vetter
2018-05-02 8:09 ` Lisovskiy, Stanislav
2018-05-02 8:15 ` Daniel Vetter
2018-05-02 9:08 ` Lisovskiy, Stanislav
2018-05-02 11:08 ` Daniel Vetter
2018-05-02 11:19 ` Lisovskiy, Stanislav
2018-04-23 7:34 ` [PATCH v7 2/2] i915: " StanLis
2018-04-23 7:50 ` ✗ Fi.CI.CHECKPATCH: warning for Enabling content-type setting for HDMI displays. (rev6) Patchwork
2018-04-23 8:04 ` ✓ Fi.CI.BAT: success " Patchwork
2018-04-23 9:33 ` ✓ Fi.CI.IGT: " Patchwork
2018-04-25 8:02 ` [PATCH v7 0/2] Enabling content-type setting for HDMI displays Lisovskiy, Stanislav
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.