[PATCH v8 0/2] Enabling content-type setting for HDMI displays.

[PATCH v8 0/2] Enabling content-type setting for HDMI displays.

[StanLis]

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      | 76 +++++++++++++++++++++++++++-
 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, 137 insertions(+), 1 deletion(-)

-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[StanLis]

From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Added content_type property to drm_connector_state
in order to properly handle external HDMI TV content-type setting.

v2:
 * Moved helper function which attaches content type property
   to the drm core, as was suggested.
   Removed redundant connector state initialization.

v3:
 * Removed caps in drm_content_type_enum_list.
   After some discussion it turned out that HDMI Spec 1.4
   was wrongly assuming that IT Content(itc) bit doesn't affect
   Content type states, however itc bit needs to be manupulated
   as well. In order to not expose additional property for itc,
   for sake of simplicity it was decided to bind those together
   in same "content type" property.

v4:
 * Added it_content checking in intel_digital_connector_atomic_check.
   Fixed documentation for new content type enum.

v5:
 * Moved patch revision's description to commit messages.

v6:
 * Minor naming fix for the content type enumeration string.

v7:
 * Fix parameter name for documentation and parameter alignment
   in order not to get warning. Added Content Type description to
   new HDMI connector properties section.

v8:
 * Thrown away unneeded numbers from HDMI content-type property
   description. Switch to strings desription instead of plain
   definitions.

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      | 76 +++++++++++++++++++++++++++-
 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, 131 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 1dffd1ac4cd4..e233c2626bd0 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -517,6 +517,12 @@ Standard Connector Properties
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
    :doc: standard connector properties
 
+HDMI Specific Connector Properties
+-----------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_connector.c
+   :doc: HDMI connector properties
+
 Plane Composition Properties
 ----------------------------
 
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
index 07ed22ea3bd6..bfde04eddd14 100644
--- a/Documentation/gpu/kms-properties.csv
+++ b/Documentation/gpu/kms-properties.csv
@@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property Name,Type,Property Values,Object attached,De
 ,Virtual GPU,“suggested X”,RANGE,"Min=0, Max=0xffffffff",Connector,property to suggest an X offset for a connector
 ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property to suggest an Y offset for a connector
 ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9"" }",Connector,TDB
+,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"", ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
 i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"", ""Limited 16:235"" }",Connector,"When this property is set to Limited 16:235 and CTM is set, the hardware will be programmed with the result of the multiplication of CTM by the limited range matrix to ensure the pixels normaly in the range 0..1.0 are remapped to the range 16/255..235/255."
 ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on"" }",Connector,TBD
 ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"", ""PAL_B"" } etc.",Connector,TBD
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3d9ae057a6cd..046fc5249859 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1270,6 +1270,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) {
@@ -1355,6 +1364,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..49941d2bd0fe 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -720,6 +720,14 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
 	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
 };
 
+static const struct drm_prop_enum_list drm_content_type_enum_list[] = {
+	{ DRM_MODE_CONTENT_TYPE_NO_DATA, "No Data" },
+	{ DRM_MODE_CONTENT_TYPE_GRAPHICS, "Graphics" },
+	{ DRM_MODE_CONTENT_TYPE_PHOTO, "Photo" },
+	{ DRM_MODE_CONTENT_TYPE_CINEMA, "Cinema" },
+	{ DRM_MODE_CONTENT_TYPE_GAME, "Game" },
+};
+
 static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
 	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
 	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
@@ -996,6 +1004,45 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
 
+
+/**
+ * DOC: HDMI connector properties
+ *
+ * content type (HDMI specific):
+ *	Indicates content type setting to be used in HDMI infoframes to indicate
+ *	content type for the external device, so that it adjusts it's display
+ *	settings accordingly.
+ *
+ *	The value of this property can be one of the following:
+ *
+ *	No Data:
+ *		Content type is unknown, it content(itc) bit is cleared.
+ *	Graphics:
+ *		Content type is graphics, it content(itc) bit is set.
+ *	Photo:
+ *		Content type is photo, itc bit is set.
+ *	Cinema:
+ *		Content type is cinema, itc bit is set.
+ *	Game:
+ *		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
@@ -1150,7 +1197,7 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
  *
  * Returns:
  * Zero on success, negative errno on failure.
- */
+ */ 
 int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
 					       u32 scaling_mode_mask)
 {
@@ -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

[PATCH v8 2/2] i915: content-type property for HDMI connector

[StanLis]

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.

v8:
 * Thrown away unneeded numbers from HDMI content-type property
   description. Switch to strings desription instead of plain
   definitions.

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Enabling content-type setting for HDMI displays. (rev7)

[Patchwork]

== Series Details ==

Series: Enabling content-type setting for HDMI displays. (rev7)
URL   : https://patchwork.freedesktop.org/series/41876/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
1aec9a9e9965 drm: content-type property for HDMI connector
-:132: CHECK:LINE_SPACING: Please don't use multiple blank lines
#132: FILE: drivers/gpu/drm/drm_connector.c:1007:
 
+

-:179: ERROR:TRAILING_WHITESPACE: trailing whitespace
#179: FILE: drivers/gpu/drm/drm_connector.c:1200:
+ */ $

-:207: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "!dev->mode_config.content_type_property"
#207: FILE: drivers/gpu/drm/drm_connector.c:1330:
+	if (dev->mode_config.content_type_property == NULL)

total: 1 errors, 0 warnings, 2 checks, 215 lines checked
556d79c08efb i915: content-type property for HDMI connector

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Enabling content-type setting for HDMI displays. (rev7)

[Patchwork]

== Series Details ==

Series: Enabling content-type setting for HDMI displays. (rev7)
URL   : https://patchwork.freedesktop.org/series/41876/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4121 -> Patchwork_8874 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8874 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8874, 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/7/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8874:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8874 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> FAIL (fdo#103841)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841


== Participating hosts (40 -> 37) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8874

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8874: 556d79c08efbf303f4ce027c6af344761c3eb678 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

556d79c08efb i915: content-type property for HDMI connector
1aec9a9e9965 drm: content-type property for HDMI connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8874/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Enabling content-type setting for HDMI displays. (rev7)

[Patchwork]

== Series Details ==

Series: Enabling content-type setting for HDMI displays. (rev7)
URL   : https://patchwork.freedesktop.org/series/41876/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4121_full -> Patchwork_8874_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8874_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8874_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/7/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8874_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          SKIP -> PASS

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP +2
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_8874_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#102887)

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@dpms-vs-vblank-race:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          PASS -> FAIL (fdo#105707)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#103928)

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-render:
      shard-hsw:          PASS -> DMESG-FAIL (fdo#103167)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@modeset-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@plain-flip-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-cpu:
      shard-apl:          FAIL (fdo#103167) -> PASS

    igt@kms_sysfs_edid_timing:
      shard-apl:          WARN (fdo#100047) -> PASS

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105707 https://bugs.freedesktop.org/show_bug.cgi?id=105707
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (7 -> 7) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4121 -> Patchwork_8874

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8874: 556d79c08efbf303f4ce027c6af344761c3eb678 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8874/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Wed, 02 May 2018, StanLis <stanislav.lisovskiy@intel.com> wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>
> Added content_type property to drm_connector_state
> in order to properly handle external HDMI TV content-type setting.
>
> v2:
>  * Moved helper function which attaches content type property
>    to the drm core, as was suggested.
>    Removed redundant connector state initialization.
>
> v3:
>  * Removed caps in drm_content_type_enum_list.
>    After some discussion it turned out that HDMI Spec 1.4
>    was wrongly assuming that IT Content(itc) bit doesn't affect
>    Content type states, however itc bit needs to be manupulated
>    as well. In order to not expose additional property for itc,
>    for sake of simplicity it was decided to bind those together
>    in same "content type" property.
>
> v4:
>  * Added it_content checking in intel_digital_connector_atomic_check.
>    Fixed documentation for new content type enum.
>
> v5:
>  * Moved patch revision's description to commit messages.
>
> v6:
>  * Minor naming fix for the content type enumeration string.
>
> v7:
>  * Fix parameter name for documentation and parameter alignment
>    in order not to get warning. Added Content Type description to
>    new HDMI connector properties section.
>
> v8:
>  * Thrown away unneeded numbers from HDMI content-type property
>    description. Switch to strings desription instead of plain
>    definitions.
>
> 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      | 76 +++++++++++++++++++++++++++-
>  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, 131 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 1dffd1ac4cd4..e233c2626bd0 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -517,6 +517,12 @@ Standard Connector Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
>     :doc: standard connector properties
>  
> +HDMI Specific Connector Properties
> +-----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> +   :doc: HDMI connector properties
> +
>  Plane Composition Properties
>  ----------------------------
>  
> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
> index 07ed22ea3bd6..bfde04eddd14 100644
> --- a/Documentation/gpu/kms-properties.csv
> +++ b/Documentation/gpu/kms-properties.csv
> @@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property Name,Type,Property Values,Object attached,De
>  ,Virtual GPU,“suggested X”,RANGE,"Min=0, Max=0xffffffff",Connector,property to suggest an X offset for a connector
>  ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property to suggest an Y offset for a connector
>  ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9"" }",Connector,TDB
> +,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"", ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
>  i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"", ""Limited 16:235"" }",Connector,"When this property is set to Limited 16:235 and CTM is set, the hardware will be programmed with the result of the multiplication of CTM by the limited range matrix to ensure the pixels normaly in the range 0..1.0 are remapped to the range 16/255..235/255."
>  ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on"" }",Connector,TBD
>  ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"", ""PAL_B"" } etc.",Connector,TBD
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3d9ae057a6cd..046fc5249859 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1270,6 +1270,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.

Who decided? I was trying to look through the history, but couldn't find
the review discussion on this part.

This approach encodes meaning to bits in the property value, while no
such promise is actually made in the property interface. Do you expect
to support graphics, photo, cinema, game with and without itc?

Is the intention to be able to extend this later on? If yes, why is itc
in bit 2 instead of e.g. bit 0, so the content-type could actually grow?

> +		 */
> +		state->content_type = val & 3;
> +		state->it_content = val >> 2;

I'm mostly asking because this is really confusing considering the
property value definitions, and should IMO be written in terms of the
macros, respecting SPOT, instead of adding magic numbers here.

Also, later on you set frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS,
which is inconsistent with the masking above.

BR,
Jani.


PS. Please wait for the discussion to settle on this before sending
another version. Accumulating tons of revisions of the patch does not
make it move any faster.

>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
>  	} else if (property == connector->content_protection_property) {
> @@ -1355,6 +1364,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..49941d2bd0fe 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -720,6 +720,14 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
>  	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
>  };
>  
> +static const struct drm_prop_enum_list drm_content_type_enum_list[] = {
> +	{ DRM_MODE_CONTENT_TYPE_NO_DATA, "No Data" },
> +	{ DRM_MODE_CONTENT_TYPE_GRAPHICS, "Graphics" },
> +	{ DRM_MODE_CONTENT_TYPE_PHOTO, "Photo" },
> +	{ DRM_MODE_CONTENT_TYPE_CINEMA, "Cinema" },
> +	{ DRM_MODE_CONTENT_TYPE_GAME, "Game" },
> +};
> +
>  static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
>  	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
>  	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
> @@ -996,6 +1004,45 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
>  
> +
> +/**
> + * DOC: HDMI connector properties
> + *
> + * content type (HDMI specific):
> + *	Indicates content type setting to be used in HDMI infoframes to indicate
> + *	content type for the external device, so that it adjusts it's display
> + *	settings accordingly.
> + *
> + *	The value of this property can be one of the following:
> + *
> + *	No Data:
> + *		Content type is unknown, it content(itc) bit is cleared.
> + *	Graphics:
> + *		Content type is graphics, it content(itc) bit is set.
> + *	Photo:
> + *		Content type is photo, itc bit is set.
> + *	Cinema:
> + *		Content type is cinema, itc bit is set.
> + *	Game:
> + *		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
> @@ -1150,7 +1197,7 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>   *
>   * Returns:
>   * Zero on success, negative errno on failure.
> - */
> + */ 
>  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  					       u32 scaling_mode_mask)
>  {
> @@ -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 \

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lisovskiy, Stanislav]

On Thu, 2018-05-03 at 13:18 +0300, Jani Nikula wrote:
> On Wed, 02 May 2018, StanLis <stanislav.lisovskiy@intel.com> wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv
> > index 07ed22ea3bd6..bfde04eddd14 100644
> > --- a/Documentation/gpu/kms-properties.csv
> > +++ b/Documentation/gpu/kms-properties.csv
> > @@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property
> > Name,Type,Property Values,Object attached,De
> >  ,Virtual GPU,“suggested X”,RANGE,"Min=0,
> > Max=0xffffffff",Connector,property to suggest an X offset for a
> > connector
> >  ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property
> > to suggest an Y offset for a connector
> >  ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9""
> > }",Connector,TDB
> > +,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"",
> > ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
> >  i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"",
> > ""Limited 16:235"" }",Connector,"When this property is set to
> > Limited 16:235 and CTM is set, the hardware will be programmed with
> > the result of the multiplication of CTM by the limited range matrix
> > to ensure the pixels normaly in the range 0..1.0 are remapped to
> > the range 16/255..235/255."
> >  ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on""
> > }",Connector,TBD
> >  ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"",
> > ""PAL_B"" } etc.",Connector,TBD
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > index 3d9ae057a6cd..046fc5249859 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1270,6 +1270,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.
> 
> Who decided? I was trying to look through the history, but couldn't
> find
> the review discussion on this part.
> 
> This approach encodes meaning to bits in the property value, while no
> such promise is actually made in the property interface. Do you
> expect
> to support graphics, photo, cinema, game with and without itc?
> 
> Is the intention to be able to extend this later on? If yes, why is
> itc
> in bit 2 instead of e.g. bit 0, so the content-type could actually
> grow?

There was a discussion here with Hans Verkuil, somewhat 2-3 weeks ago,
he indicated that HDMI 1.4 spec was wrong in assuming that itc and
content-type bits can be used separately. Then it was decided offline
with Ville Sirjala that we're not going to create a separate property
for itc bit for a sake of simplicity. 
In short, the content type bits CN1..CN0 are not valid without itc bit.

> 
> > +		 */
> > +		state->content_type = val & 3;
> > +		state->it_content = val >> 2;
> 
> I'm mostly asking because this is really confusing considering the
> property value definitions, and should IMO be written in terms of the
> macros, respecting SPOT, instead of adding magic numbers here.
> 
> Also, later on you set frame->content_type =
> HDMI_CONTENT_TYPE_GRAPHICS,
> which is inconsistent with the masking above.

If you mean those lines, where it initializes the avi infoframe
structure, it sets itc bit at the same time with content_type and from
the frame
encoding point of view those are still separate, so there is no
violation:

+       frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
+       frame->itc = 0;

So we have a single property setting both at the same time, as
there is no point to have content-type without itc bit set.
Everytime property is set, those state bits are changed and then
encoded into frame.

+       frame.avi.content_type = connector->state->content_type;
+       frame.avi.itc = connector->state->it_content;

> BR,
> Jani.
> 
> 
> PS. Please wait for the discussion to settle on this before sending
> another version. Accumulating tons of revisions of the patch does not
> make it move any faster.

Well, there was plenty of discussion already, Ville, Hans and Daniel
gave recommendations and acknowleged those changes earlier, so I
thought that everything is clear here at least. Any comments are still
welcome of course, however I think I need to fix the stuff otherwise
that simple patch will hang here forever imho.

Version 8 just fixes the latest comment from Ville, that docs should
use string values in property description. Everytime I get any
comments, I fix them.

> 
-- 
Best Regards,

Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lisovskiy, Stanislav]

On Thu, 2018-05-03 at 10:46 +0000, Lisovskiy, Stanislav wrote:
> On Thu, 2018-05-03 at 13:18 +0300, Jani Nikula wrote:
> > On Wed, 02 May 2018, StanLis <stanislav.lisovskiy@intel.com> wrote:
> > > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > 
> > > 
> > > diff --git a/Documentation/gpu/kms-properties.csv
> > > b/Documentation/gpu/kms-properties.csv
> > > index 07ed22ea3bd6..bfde04eddd14 100644
> > > --- a/Documentation/gpu/kms-properties.csv
> > > +++ b/Documentation/gpu/kms-properties.csv
> > > @@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property
> > > Name,Type,Property Values,Object attached,De
> > >  ,Virtual GPU,“suggested X”,RANGE,"Min=0,
> > > Max=0xffffffff",Connector,property to suggest an X offset for a
> > > connector
> > >  ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property
> > > to suggest an Y offset for a connector
> > >  ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9""
> > > }",Connector,TDB
> > > +,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"",
> > > ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
> > >  i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"",
> > > ""Full"",
> > > ""Limited 16:235"" }",Connector,"When this property is set to
> > > Limited 16:235 and CTM is set, the hardware will be programmed
> > > with
> > > the result of the multiplication of CTM by the limited range
> > > matrix
> > > to ensure the pixels normaly in the range 0..1.0 are remapped to
> > > the range 16/255..235/255."
> > >  ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on""
> > > }",Connector,TBD
> > >  ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"",
> > > ""PAL_B"" } etc.",Connector,TBD
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > b/drivers/gpu/drm/drm_atomic.c
> > > index 3d9ae057a6cd..046fc5249859 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1270,6 +1270,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.
> > 
> > Who decided? I was trying to look through the history, but couldn't
> > find
> > the review discussion on this part.
> > 
> > This approach encodes meaning to bits in the property value, while
> > no
> > such promise is actually made in the property interface. Do you
> > expect
> > to support graphics, photo, cinema, game with and without itc?
> > 
> > Is the intention to be able to extend this later on? If yes, why is
> > itc
> > in bit 2 instead of e.g. bit 0, so the content-type could actually
> > grow?
> 
> There was a discussion here with Hans Verkuil, somewhat 2-3 weeks
> ago,
> he indicated that HDMI 1.4 spec was wrong in assuming that itc and
> content-type bits can be used separately. Then it was decided offline
> with Ville Sirjala that we're not going to create a separate property
> for itc bit for a sake of simplicity. 
> In short, the content type bits CN1..CN0 are not valid without itc
> bit.
> 
> > 
> > > +		 */
> > > +		state->content_type = val & 3;
> > > +		state->it_content = val >> 2;
> > 
> > I'm mostly asking because this is really confusing considering the
> > property value definitions, and should IMO be written in terms of
> > the
> > macros, respecting SPOT, instead of adding magic numbers here.
> > 
> > Also, later on you set frame->content_type =
> > HDMI_CONTENT_TYPE_GRAPHICS,
> > which is inconsistent with the masking above.
> 
> If you mean those lines, where it initializes the avi infoframe
> structure, it sets itc bit at the same time with content_type and
> from
> the frame
> encoding point of view those are still separate, so there is no
> violation:
> 
> +       frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
> +       frame->itc = 0;
> 
> So we have a single property setting both at the same time, as
> there is no point to have content-type without itc bit set.
> Everytime property is set, those state bits are changed and then
> encoded into frame.
> 
> +       frame.avi.content_type = connector->state->content_type;
> +       frame.avi.itc = connector->state->it_content;
> 
> > BR,
> > Jani.
> > 
> > 
> > PS. Please wait for the discussion to settle on this before sending
> > another version. Accumulating tons of revisions of the patch does
> > not
> > make it move any faster.
> 
> Well, there was plenty of discussion already, Ville, Hans and Daniel
> gave recommendations and acknowleged those changes earlier, so I
> thought that everything is clear here at least. Any comments are
> still
> welcome of course, however I think I need to fix the stuff otherwise
> that simple patch will hang here forever imho.
> 
> Version 8 just fixes the latest comment from Ville, that docs should
> use string values in property description. Everytime I get any
> comments, I fix them.
> 

So settling here a discussion also. Just to have it in writing as we
spoke offline, the problem is that we basically have to manipulate with
two different AVI inforame fields(itc bit and content type bits) using
the same DRM property.  
This obviously implies some coding/decoding logic already, my initial
approach was just to have those bits in place( bit 2 controls IT
content bit, bits  1..0 control content-type cn1..cn0 bits) in order
not to have any other decoding logic in set/get property calls(also I
saw there are other non-contigious bit masks and sets in uapi header).

> 
> > +/* 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

If those "magic" numbers defined here are not OK, then I need to add
some additional decoding layer. Should I just add a macros or helper
functions then, which would transform contigious
DRM_MODE_CONTENT_TYPE_* values into correspondent AVI infoframe itc and
content-type fields?

> > +
> >  /* Aspect ratio flag bitmask (4 bits 22:19) */
> >  #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> >  #define  DRM_MODE_FLAG_PIC_AR_NONE \
> 
> 
-- 
Best Regards,

Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx