[PATCH v2 1/3] drm: Add SCDC helpers

[PATCH v2 1/3] drm: Add SCDC helpers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

SCDC is a mechanism defined in the HDMI 2.0 specification that allows
the source and sink devices to communicate.

This commit introduces helpers to access the SCDC and provides the
symbolic names for the various registers defined in the specification.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- indent register field definitions for readability (Ville Syrjälä)
- use GFP_TEMPORARY for temporary buffer allocation (Jani Nikula)

 Documentation/gpu/drm-kms-helpers.rst |  12 ++++
 drivers/gpu/drm/Kconfig               |   4 ++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
 include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
 5 files changed, 260 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
 create mode 100644 include/drm/drm_scdc_helper.h

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 03040aa14fe8..759275629fcf 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -217,6 +217,18 @@ EDID Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_edid.c
    :export:
 
+SCDC Helper Functions Reference
+===============================
+
+.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
+   :doc: scdc helpers
+
+.. kernel-doc:: include/drm/drm_scdc_helper.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
+   :export:
+
 Rectangle Utilities Reference
 =============================
 
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 95fc0410e129..d0031fe45bab 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -84,6 +84,10 @@ config DRM_FBDEV_EMULATION
 
 	  If in doubt, say "Y".
 
+config DRM_SCDC
+	bool
+	depends on DRM_KMS_HELPER
+
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
 	depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 883f3e75cfbc..71c38b6dd546 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
 		drm_simple_kms_helper.o drm_modeset_helper.o
 
+drm_kms_helper-$(CONFIG_DRM_SCDC) += drm_scdc_helper.o
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
new file mode 100644
index 000000000000..fe0e1211e873
--- /dev/null
+++ b/drivers/gpu/drm/drm_scdc_helper.c
@@ -0,0 +1,111 @@
+/*
+ * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/slab.h>
+
+#include <drm/drm_scdc_helper.h>
+
+/**
+ * DOC: scdc helpers
+ *
+ * Status and Control Data Channel (SCDC) is a mechanism introduced by the
+ * HDMI 2.0 specification. It is a point-to-point protocol that allows the
+ * HDMI source and HDMI sink to exchange data. The same I2C interface that
+ * is used to access EDID serves as the transport mechanism for SCDC.
+ */
+
+#define SCDC_I2C_SLAVE_ADDRESS 0x54
+
+/**
+ * drm_scdc_read - read a block of data from SCDC
+ * @adapter: I2C controller
+ * @offset: start offset of block to read
+ * @buffer: return location for the block to read
+ * @size: size of the block to read
+ *
+ * Reads a block of data from SCDC, starting at a given offset.
+ *
+ * Returns:
+ * The number of bytes read from SCDC or a negative error code on failure.
+ */
+ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		      size_t size)
+{
+	struct i2c_msg msgs[2] = {
+		{
+			.addr = SCDC_I2C_SLAVE_ADDRESS,
+			.flags = 0,
+			.len = 1,
+			.buf = &offset,
+		}, {
+			.addr = SCDC_I2C_SLAVE_ADDRESS,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buffer,
+		}
+	};
+
+	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+}
+EXPORT_SYMBOL(drm_scdc_read);
+
+/**
+ * drm_scdc_write - write a block of data to SCDC
+ * @adapter: I2C controller
+ * @offset: start offset of block to write
+ * @buffer: block of data to write
+ * @size: size of the block to write
+ *
+ * Writes a block of data to SCDC, starting at a given offset.
+ *
+ * Returns:
+ * The number of bytes written to SCDC or a negative error code on failure.
+ */
+ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
+		       const void *buffer, size_t size)
+{
+	struct i2c_msg msg = {
+		.addr = SCDC_I2C_SLAVE_ADDRESS,
+		.flags = 0,
+		.len = 1 + size,
+		.buf = NULL,
+	};
+	void *data;
+	int err;
+
+	data = kmalloc(1 + size, GFP_TEMPORARY);
+	if (!data)
+		return -ENOMEM;
+
+	msg.buf = data;
+
+	memcpy(data, &offset, sizeof(offset));
+	memcpy(data + 1, buffer, size);
+
+	err = i2c_transfer(adapter, &msg, 1);
+
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL(drm_scdc_write);
diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
new file mode 100644
index 000000000000..93b07bcf0291
--- /dev/null
+++ b/include/drm/drm_scdc_helper.h
@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef DRM_SCDC_HELPER_H
+#define DRM_SCDC_HELPER_H
+
+#include <linux/i2c.h>
+#include <linux/types.h>
+
+#define SCDC_SINK_VERSION 0x01
+
+#define SCDC_SOURCE_VERSION 0x02
+
+#define SCDC_UPDATE_0 0x10
+#define  SCDC_READ_REQUEST_TEST (1 << 2)
+#define  SCDC_CED_UPDATE (1 << 1)
+#define  SCDC_STATUS_UPDATE (1 << 0)
+
+#define SCDC_UPDATE_1 0x11
+
+#define SCDC_TMDS_CONFIG 0x20
+#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1)
+#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
+#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
+
+#define SCDC_SCRAMBLER_STATUS 0x21
+#define  SCDC_SCRAMBLING_STATUS (1 << 0)
+
+#define SCDC_CONFIG_0 0x30
+#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
+
+#define SCDC_STATUS_FLAGS_0 0x40
+#define  SCDC_CH2_LOCK (1 < 3)
+#define  SCDC_CH1_LOCK (1 < 2)
+#define  SCDC_CH0_LOCK (1 < 1)
+#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK)
+#define  SCDC_CLOCK_DETECT (1 << 0)
+
+#define SCDC_STATUS_FLAGS_1 0x41
+
+#define SCDC_ERR_DET_0_L 0x50
+#define SCDC_ERR_DET_0_H 0x51
+#define SCDC_ERR_DET_1_L 0x52
+#define SCDC_ERR_DET_1_H 0x53
+#define SCDC_ERR_DET_2_L 0x54
+#define SCDC_ERR_DET_2_H 0x55
+#define  SCDC_CHANNEL_VALID (1 << 7)
+
+#define SCDC_ERR_DET_CHECKSUM 0x56
+
+#define SCDC_TEST_CONFIG_0 0xc0
+#define  SCDC_TEST_READ_REQUEST (1 << 7)
+#define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
+
+#define SCDC_MANUFACTURER_IEEE_OUI 0xd0
+#define SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
+
+#define SCDC_DEVICE_ID 0xd3
+#define SCDC_DEVICE_ID_SIZE 8
+
+#define SCDC_DEVICE_HARDWARE_REVISION 0xdb
+#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf)
+#define  SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
+
+#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc
+#define SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
+
+#define SCDC_MANUFACTURER_SPECIFIC 0xde
+#define SCDC_MANUFACTURER_SPECIFIC_SIZE 34
+
+ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		      size_t size);
+ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
+		       const void *buffer, size_t size);
+
+/**
+ * drm_scdc_readb - read a single byte from SCDC
+ * @adapter: I2C adapter
+ * @offset: offset of register to read
+ * @value: return location for the register value
+ *
+ * Reads a single byte from SCDC. This is a convenience wrapper around the
+ * drm_scdc_read() function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
+				 u8 *value)
+{
+	return drm_scdc_read(adapter, offset, value, sizeof(*value));
+}
+
+/**
+ * drm_scdc_writeb - write a single byte to SCDC
+ * @adapter: I2C adapter
+ * @offset: offset of register to read
+ * @value: return location for the register value
+ *
+ * Writes a single byte to SCDC. This is a convenience wrapper around the
+ * drm_scdc_write() function.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
+				  u8 value)
+{
+	return drm_scdc_write(adapter, offset, &value, sizeof(value));
+}
+
+#endif
-- 
2.10.2

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

[PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Sinks compliant with the HDMI 2.0 specification may support SCDC, a
mechanism for the source and the sink to communicate. Sinks advertise
support for this feature in the HDMI Forum Vendor Specific Data Block
as defined in the HDMI 2.0 specification, section 10.4 "Status and
Control Data Channel". Implement a small helper that find the HF-VSDB
and parses it to check if the sink supports SCDC.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- rename internal function to cea_db_is_hdmi_forum_vsdb() for more
  clarity (Ville Syrjälä)

 drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 include/linux/hdmi.h       |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 336be31ff3de..369961597ee5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 	return hdmi_id == HDMI_IEEE_OUI;
 }
 
+static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
+{
+	unsigned int oui;
+
+	if (cea_db_tag(db) != VENDOR_BLOCK)
+		return false;
+
+	if (cea_db_payload_len(db) < 7)
+		return false;
+
+	oui = db[3] << 16 | db[2] << 8 | db[1];
+
+	return oui == HDMI_FORUM_IEEE_OUI;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
 	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
 
@@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
 EXPORT_SYMBOL(drm_detect_hdmi_monitor);
 
 /**
+ * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
+ * @edid: sink EDID information
+ *
+ * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
+ * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
+ * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
+ *
+ * Returns:
+ * True if the sink supports SCDC, false otherwise.
+ */
+bool drm_detect_hdmi_scdc(struct edid *edid)
+{
+	unsigned int start, end, i;
+	const u8 *cea;
+
+	cea = drm_find_cea_extension(edid);
+	if (!cea)
+		return false;
+
+	if (cea_db_offsets(cea, &start, &end))
+		return false;
+
+	for_each_cea_db(cea, i, start, end) {
+		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
+			if (cea[i + 6] & 0x80)
+				return true;
+		}
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_detect_hdmi_scdc);
+
+/**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 38eabf65f19d..7ea7e90846d8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
 bool drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_hdmi_scdc(struct edid *edid);
 bool drm_detect_monitor_audio(struct edid *edid);
 bool drm_rgb_quant_range_selectable(struct edid *edid);
 int drm_add_modes_noedid(struct drm_connector *connector,
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index edbb4fc674ed..d271ff23984f 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -35,6 +35,7 @@ enum hdmi_infoframe_type {
 };
 
 #define HDMI_IEEE_OUI 0x000c03
+#define HDMI_FORUM_IEEE_OUI 0xc45dd8
 #define HDMI_INFOFRAME_HEADER_SIZE  4
 #define HDMI_AVI_INFOFRAME_SIZE    13
 #define HDMI_SPD_INFOFRAME_SIZE    25
-- 
2.10.2

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

[PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Sinks that support SCDC can optionally have the capability to initiate
read requests, which are a mechanism by which a sink can notify its
source that it should read the Update Flags. If either the sink or the
source are not Read Request capable, polling of the Update Flags shall
be employed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- new patch

 drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 369961597ee5..8211cce3e09e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3736,6 +3736,42 @@ bool drm_detect_hdmi_scdc(struct edid *edid)
 EXPORT_SYMBOL(drm_detect_hdmi_scdc);
 
 /**
+ * drm_detect_hdmi_scdc_rr_capable - detect whether an HDMI sink is capable of
+ *    initiating an SCDC Read Request
+ * @edid: sink EDID information
+ *
+ * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
+ * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
+ * Block" and checks if the RR_Capable bit (bit 6 of byte 6) is set.
+ *
+ * Returns:
+ * True if the sink is capable of initiating an SCDC Read Request, false
+ * otherwise.
+ */
+bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid)
+{
+	unsigned int start, end, i;
+	const u8 *cea;
+
+	cea = drm_find_cea_extension(edid);
+	if (!cea)
+		return false;
+
+	if (cea_db_offsets(cea, &start, &end))
+		return false;
+
+	for_each_cea_db(cea, i, start, end) {
+		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
+			if (cea[i + 6] & 0x40)
+				return true;
+		}
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_detect_hdmi_scdc_rr_capable);
+
+/**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 7ea7e90846d8..d1c29586035e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -441,6 +441,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
 bool drm_detect_hdmi_monitor(struct edid *edid);
 bool drm_detect_hdmi_scdc(struct edid *edid);
+bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid);
 bool drm_detect_monitor_audio(struct edid *edid);
 bool drm_rgb_quant_range_selectable(struct edid *edid);
 int drm_add_modes_noedid(struct drm_connector *connector,
-- 
2.10.2

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

RE: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Sharma, Shashank]

Hi Thierry, 

If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
https://patchwork.kernel.org/patch/9452259/ 

Regards
Shashank
-----Original Message-----
From: Thierry Reding [mailto:thierry.reding@gmail.com] 
Sent: Saturday, December 3, 2016 12:54 AM
To: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; Jose Abreu <joabreu@synopsys.com>
Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

From: Thierry Reding <treding@nvidia.com>

Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism for the source and the sink to communicate. Sinks advertise support for this feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 specification, section 10.4 "Status and Control Data Channel". Implement a small helper that find the HF-VSDB and parses it to check if the sink supports SCDC.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- rename internal function to cea_db_is_hdmi_forum_vsdb() for more
  clarity (Ville Syrjälä)

 drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 include/linux/hdmi.h       |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 336be31ff3de..369961597ee5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 	return hdmi_id == HDMI_IEEE_OUI;
 }
 
+static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
+	unsigned int oui;
+
+	if (cea_db_tag(db) != VENDOR_BLOCK)
+		return false;
+
+	if (cea_db_payload_len(db) < 7)
+		return false;
+
+	oui = db[3] << 16 | db[2] << 8 | db[1];
+
+	return oui == HDMI_FORUM_IEEE_OUI;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
 	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
 
@@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
 
 /**
+ * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
+ * @edid: sink EDID information
+ *
+ * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
+ * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
+ * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
+ *
+ * Returns:
+ * True if the sink supports SCDC, false otherwise.
+ */
+bool drm_detect_hdmi_scdc(struct edid *edid) {
+	unsigned int start, end, i;
+	const u8 *cea;
+
+	cea = drm_find_cea_extension(edid);
+	if (!cea)
+		return false;
+
+	if (cea_db_offsets(cea, &start, &end))
+		return false;
+
+	for_each_cea_db(cea, i, start, end) {
+		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
+			if (cea[i + 6] & 0x80)
+				return true;
+		}
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_detect_hdmi_scdc);
+
+/**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 38eabf65f19d..7ea7e90846d8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_hdmi_scdc(struct edid *edid);
 bool drm_detect_monitor_audio(struct edid *edid);  bool drm_rgb_quant_range_selectable(struct edid *edid);  int drm_add_modes_noedid(struct drm_connector *connector, diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -35,6 +35,7 @@ enum hdmi_infoframe_type {  };
 
 #define HDMI_IEEE_OUI 0x000c03
+#define HDMI_FORUM_IEEE_OUI 0xc45dd8
 #define HDMI_INFOFRAME_HEADER_SIZE  4
 #define HDMI_AVI_INFOFRAME_SIZE    13
 #define HDMI_SPD_INFOFRAME_SIZE    25
--
2.10.2

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

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4898 bytes --]

On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> Hi Thierry, 
> 
> If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> https://patchwork.kernel.org/patch/9452259/ 

I think there had been pushback before about caching capabilities from
EDID, so from that point of view my patch is more inline with existing
EDID parsing API.

Other than that the patches are mostly equivalent, except yours parses
more information than just the SCDC bits.

Thierry

> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com] 
> Sent: Saturday, December 3, 2016 12:54 AM
> To: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; Jose Abreu <joabreu@synopsys.com>
> Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> 
> From: Thierry Reding <treding@nvidia.com>
> 
> Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism for the source and the sink to communicate. Sinks advertise support for this feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 specification, section 10.4 "Status and Control Data Channel". Implement a small helper that find the HF-VSDB and parses it to check if the sink supports SCDC.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
>   clarity (Ville Syrjälä)
> 
>  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  1 +
>  include/linux/hdmi.h       |  1 +
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 336be31ff3de..369961597ee5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> +	unsigned int oui;
> +
> +	if (cea_db_tag(db) != VENDOR_BLOCK)
> +		return false;
> +
> +	if (cea_db_payload_len(db) < 7)
> +		return false;
> +
> +	oui = db[3] << 16 | db[2] << 8 | db[1];
> +
> +	return oui == HDMI_FORUM_IEEE_OUI;
> +}
> +
>  #define for_each_cea_db(cea, i, start, end) \
>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>  
> @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  
>  /**
> + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> + * @edid: sink EDID information
> + *
> + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> + *
> + * Returns:
> + * True if the sink supports SCDC, false otherwise.
> + */
> +bool drm_detect_hdmi_scdc(struct edid *edid) {
> +	unsigned int start, end, i;
> +	const u8 *cea;
> +
> +	cea = drm_find_cea_extension(edid);
> +	if (!cea)
> +		return false;
> +
> +	if (cea_db_offsets(cea, &start, &end))
> +		return false;
> +
> +	for_each_cea_db(cea, i, start, end) {
> +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> +			if (cea[i + 6] & 0x80)
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> +
> +/**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
>   *
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 38eabf65f19d..7ea7e90846d8 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool drm_detect_hdmi_monitor(struct edid *edid);
> +bool drm_detect_hdmi_scdc(struct edid *edid);
>  bool drm_detect_monitor_audio(struct edid *edid);  bool drm_rgb_quant_range_selectable(struct edid *edid);  int drm_add_modes_noedid(struct drm_connector *connector, diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -35,6 +35,7 @@ enum hdmi_infoframe_type {  };
>  
>  #define HDMI_IEEE_OUI 0x000c03
> +#define HDMI_FORUM_IEEE_OUI 0xc45dd8
>  #define HDMI_INFOFRAME_HEADER_SIZE  4
>  #define HDMI_AVI_INFOFRAME_SIZE    13
>  #define HDMI_SPD_INFOFRAME_SIZE    25
> --
> 2.10.2
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Daniel Vetter]

On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > Hi Thierry, 
> > 
> > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > https://patchwork.kernel.org/patch/9452259/ 
> 
> I think there had been pushback before about caching capabilities from
> EDID, so from that point of view my patch is more inline with existing
> EDID parsing API.

Hm, where was that pushback? We do have a bit a mess between explicitly
parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
I think long-term stuffing it into drm_display_info is probably better,
since then we only have 1 interaction point between the probe code and the
atomic_check code. That should be useful for eventually fixing the lack of
locking between the two, if I ever get around to that ;-)

> Other than that the patches are mostly equivalent, except yours parses
> more information than just the SCDC bits.

So merge patch 1 from your series + Shashank's parsing patch? Everyone
agrees and can you pls cross-r-b stamp so I can apply it all?

Thanks, Daniel

> 
> Thierry
> 
> > -----Original Message-----
> > From: Thierry Reding [mailto:thierry.reding@gmail.com] 
> > Sent: Saturday, December 3, 2016 12:54 AM
> > To: dri-devel@lists.freedesktop.org
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; Jose Abreu <joabreu@synopsys.com>
> > Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> > 
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism for the source and the sink to communicate. Sinks advertise support for this feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 specification, section 10.4 "Status and Control Data Channel". Implement a small helper that find the HF-VSDB and parses it to check if the sink supports SCDC.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
> >   clarity (Ville Syrjälä)
> > 
> >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  1 +
> >  include/linux/hdmi.h       |  1 +
> >  3 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 336be31ff3de..369961597ee5 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >  	return hdmi_id == HDMI_IEEE_OUI;
> >  }
> >  
> > +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> > +	unsigned int oui;
> > +
> > +	if (cea_db_tag(db) != VENDOR_BLOCK)
> > +		return false;
> > +
> > +	if (cea_db_payload_len(db) < 7)
> > +		return false;
> > +
> > +	oui = db[3] << 16 | db[2] << 8 | db[1];
> > +
> > +	return oui == HDMI_FORUM_IEEE_OUI;
> > +}
> > +
> >  #define for_each_cea_db(cea, i, start, end) \
> >  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
> >  
> > @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >  
> >  /**
> > + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> > + * @edid: sink EDID information
> > + *
> > + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> > + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> > + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> > + *
> > + * Returns:
> > + * True if the sink supports SCDC, false otherwise.
> > + */
> > +bool drm_detect_hdmi_scdc(struct edid *edid) {
> > +	unsigned int start, end, i;
> > +	const u8 *cea;
> > +
> > +	cea = drm_find_cea_extension(edid);
> > +	if (!cea)
> > +		return false;
> > +
> > +	if (cea_db_offsets(cea, &start, &end))
> > +		return false;
> > +
> > +	for_each_cea_db(cea, i, start, end) {
> > +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> > +			if (cea[i + 6] & 0x80)
> > +				return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> > +
> > +/**
> >   * drm_detect_monitor_audio - check monitor audio capability
> >   * @edid: EDID block to scan
> >   *
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 38eabf65f19d..7ea7e90846d8 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> >  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool drm_detect_hdmi_monitor(struct edid *edid);
> > +bool drm_detect_hdmi_scdc(struct edid *edid);
> >  bool drm_detect_monitor_audio(struct edid *edid);  bool drm_rgb_quant_range_selectable(struct edid *edid);  int drm_add_modes_noedid(struct drm_connector *connector, diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 100644
> > --- a/include/linux/hdmi.h
> > +++ b/include/linux/hdmi.h
> > @@ -35,6 +35,7 @@ enum hdmi_infoframe_type {  };
> >  
> >  #define HDMI_IEEE_OUI 0x000c03
> > +#define HDMI_FORUM_IEEE_OUI 0xc45dd8
> >  #define HDMI_INFOFRAME_HEADER_SIZE  4
> >  #define HDMI_AVI_INFOFRAME_SIZE    13
> >  #define HDMI_SPD_INFOFRAME_SIZE    25
> > --
> > 2.10.2
> > 



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


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

Re: [PATCH v2 1/3] drm: Add SCDC helpers

[Jose Abreu]

Hi Thierry,


On 02-12-2016 19:24, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> SCDC is a mechanism defined in the HDMI 2.0 specification that allows
> the source and sink devices to communicate.
>
> This commit introduces helpers to access the SCDC and provides the
> symbolic names for the various registers defined in the specification.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - indent register field definitions for readability (Ville Syrjälä)
> - use GFP_TEMPORARY for temporary buffer allocation (Jani Nikula)
>
>  Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>  drivers/gpu/drm/Kconfig               |   4 ++
>  drivers/gpu/drm/Makefile              |   1 +
>  drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
>  include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
>  5 files changed, 260 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
>  create mode 100644 include/drm/drm_scdc_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 03040aa14fe8..759275629fcf 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -217,6 +217,18 @@ EDID Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_edid.c
>     :export:
>  
> +SCDC Helper Functions Reference
> +===============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :doc: scdc helpers
> +
> +.. kernel-doc:: include/drm/drm_scdc_helper.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :export:
> +
>  Rectangle Utilities Reference
>  =============================
>  
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 95fc0410e129..d0031fe45bab 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -84,6 +84,10 @@ config DRM_FBDEV_EMULATION
>  
>  	  If in doubt, say "Y".
>  
> +config DRM_SCDC
> +	bool
> +	depends on DRM_KMS_HELPER
> +
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
>  	depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 883f3e75cfbc..71c38b6dd546 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>  		drm_simple_kms_helper.o drm_modeset_helper.o
>  
> +drm_kms_helper-$(CONFIG_DRM_SCDC) += drm_scdc_helper.o
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
> new file mode 100644
> index 000000000000..fe0e1211e873
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/slab.h>
> +
> +#include <drm/drm_scdc_helper.h>
> +
> +/**
> + * DOC: scdc helpers
> + *
> + * Status and Control Data Channel (SCDC) is a mechanism introduced by the
> + * HDMI 2.0 specification. It is a point-to-point protocol that allows the
> + * HDMI source and HDMI sink to exchange data. The same I2C interface that
> + * is used to access EDID serves as the transport mechanism for SCDC.
> + */
> +
> +#define SCDC_I2C_SLAVE_ADDRESS 0x54
> +
> +/**
> + * drm_scdc_read - read a block of data from SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to read
> + * @buffer: return location for the block to read
> + * @size: size of the block to read
> + *
> + * Reads a block of data from SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes read from SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size)
> +{
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &offset,
> +		}, {
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = I2C_M_RD,
> +			.len = size,
> +			.buf = buffer,
> +		}
> +	};
> +
> +	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +}
> +EXPORT_SYMBOL(drm_scdc_read);
> +
> +/**
> + * drm_scdc_write - write a block of data to SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to write
> + * @buffer: block of data to write
> + * @size: size of the block to write
> + *
> + * Writes a block of data to SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes written to SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size)
> +{
> +	struct i2c_msg msg = {
> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> +		.flags = 0,
> +		.len = 1 + size,
> +		.buf = NULL,
> +	};
> +	void *data;
> +	int err;
> +
> +	data = kmalloc(1 + size, GFP_TEMPORARY);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	msg.buf = data;
> +
> +	memcpy(data, &offset, sizeof(offset));
> +	memcpy(data + 1, buffer, size);

Don't you agree it would be better if you use the same scheme as
drm_scdc_read()? Something like:

struct i2c_msg msgs[] = {
    {
        .addr = SCDC_I2C_SLAVE_ADDRESS,
        .flags = 0,
        .len = 1,
        .buf = &offset,
    }, {
        .addr = SCDC_I2C_SLAVE_ADDRESS,
        .flags = 0,
        .len = size,
        .buf = buffer,
    },
};


> +
> +	err = i2c_transfer(adapter, &msg, 1);
> +
> +	kfree(data);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_scdc_write);
> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
> new file mode 100644
> index 000000000000..93b07bcf0291
> --- /dev/null
> +++ b/include/drm/drm_scdc_helper.h
> @@ -0,0 +1,132 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef DRM_SCDC_HELPER_H
> +#define DRM_SCDC_HELPER_H
> +
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +
> +#define SCDC_SINK_VERSION 0x01
> +
> +#define SCDC_SOURCE_VERSION 0x02
> +
> +#define SCDC_UPDATE_0 0x10
> +#define  SCDC_READ_REQUEST_TEST (1 << 2)
> +#define  SCDC_CED_UPDATE (1 << 1)
> +#define  SCDC_STATUS_UPDATE (1 << 0)
> +
> +#define SCDC_UPDATE_1 0x11
> +
> +#define SCDC_TMDS_CONFIG 0x20
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1)
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
> +#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
> +
> +#define SCDC_SCRAMBLER_STATUS 0x21
> +#define  SCDC_SCRAMBLING_STATUS (1 << 0)
> +
> +#define SCDC_CONFIG_0 0x30
> +#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_0 0x40
> +#define  SCDC_CH2_LOCK (1 < 3)
> +#define  SCDC_CH1_LOCK (1 < 2)
> +#define  SCDC_CH0_LOCK (1 < 1)
> +#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK)
> +#define  SCDC_CLOCK_DETECT (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_1 0x41
> +
> +#define SCDC_ERR_DET_0_L 0x50
> +#define SCDC_ERR_DET_0_H 0x51
> +#define SCDC_ERR_DET_1_L 0x52
> +#define SCDC_ERR_DET_1_H 0x53
> +#define SCDC_ERR_DET_2_L 0x54
> +#define SCDC_ERR_DET_2_H 0x55
> +#define  SCDC_CHANNEL_VALID (1 << 7)
> +
> +#define SCDC_ERR_DET_CHECKSUM 0x56
> +
> +#define SCDC_TEST_CONFIG_0 0xc0
> +#define  SCDC_TEST_READ_REQUEST (1 << 7)
> +#define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> +
> +#define SCDC_MANUFACTURER_IEEE_OUI 0xd0
> +#define SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
> +
> +#define SCDC_DEVICE_ID 0xd3
> +#define SCDC_DEVICE_ID_SIZE 8
> +
> +#define SCDC_DEVICE_HARDWARE_REVISION 0xdb
> +#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf)
> +#define  SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
> +
> +#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc
> +#define SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
> +
> +#define SCDC_MANUFACTURER_SPECIFIC 0xde
> +#define SCDC_MANUFACTURER_SPECIFIC_SIZE 34
> +
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size);
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size);
> +
> +/**
> + * drm_scdc_readb - read a single byte from SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Reads a single byte from SCDC. This is a convenience wrapper around the
> + * drm_scdc_read() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
> +				 u8 *value)
> +{
> +	return drm_scdc_read(adapter, offset, value, sizeof(*value));
> +}
> +
> +/**
> + * drm_scdc_writeb - write a single byte to SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Writes a single byte to SCDC. This is a convenience wrapper around the
> + * drm_scdc_write() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
> +				  u8 value)
> +{
> +	return drm_scdc_write(adapter, offset, &value, sizeof(value));
> +}
> +
> +#endif

Best regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Jose Abreu]

Hi Thierry,


Do you think while you are at it you could implement a
set_scrambling() callback? It should be pretty straight forward:
you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
again.


I think this is an important feature that we should have.


Best regards,

Jose Miguel Abreu


On 02-12-2016 19:24, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Sinks that support SCDC can optionally have the capability to initiate
> read requests, which are a mechanism by which a sink can notify its
> source that it should read the Update Flags. If either the sink or the
> source are not Read Request capable, polling of the Update Flags shall
> be employed.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - new patch
>
>  drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 369961597ee5..8211cce3e09e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3736,6 +3736,42 @@ bool drm_detect_hdmi_scdc(struct edid *edid)
>  EXPORT_SYMBOL(drm_detect_hdmi_scdc);
>  
>  /**
> + * drm_detect_hdmi_scdc_rr_capable - detect whether an HDMI sink is capable of
> + *    initiating an SCDC Read Request
> + * @edid: sink EDID information
> + *
> + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> + * Block" and checks if the RR_Capable bit (bit 6 of byte 6) is set.
> + *
> + * Returns:
> + * True if the sink is capable of initiating an SCDC Read Request, false
> + * otherwise.
> + */
> +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid)
> +{
> +	unsigned int start, end, i;
> +	const u8 *cea;
> +
> +	cea = drm_find_cea_extension(edid);
> +	if (!cea)
> +		return false;
> +
> +	if (cea_db_offsets(cea, &start, &end))
> +		return false;
> +
> +	for_each_cea_db(cea, i, start, end) {
> +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> +			if (cea[i + 6] & 0x40)
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_detect_hdmi_scdc_rr_capable);
> +
> +/**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
>   *
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 7ea7e90846d8..d1c29586035e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -441,6 +441,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
>  bool drm_detect_hdmi_monitor(struct edid *edid);
>  bool drm_detect_hdmi_scdc(struct edid *edid);
> +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid);
>  bool drm_detect_monitor_audio(struct edid *edid);
>  bool drm_rgb_quant_range_selectable(struct edid *edid);
>  int drm_add_modes_noedid(struct drm_connector *connector,

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

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7451 bytes --]

On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > Hi Thierry, 
> > > 
> > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > https://patchwork.kernel.org/patch/9452259/ 
> > 
> > I think there had been pushback before about caching capabilities from
> > EDID, so from that point of view my patch is more inline with existing
> > EDID parsing API.
> 
> Hm, where was that pushback? We do have a bit a mess between explicitly
> parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.

You did object to a very similar patch some time ago that did a similar
thing for DPCD stuff. And also Villa had commented on an earlier patch
from Jose about concerns of bloating core structures:

	https://patchwork.freedesktop.org/patch/104806/

> I think long-term stuffing it into drm_display_info is probably better,
> since then we only have 1 interaction point between the probe code and the
> atomic_check code. That should be useful for eventually fixing the lack of
> locking between the two, if I ever get around to that ;-)

I don't really have objections to caching the results of parsing, it's
what I had proposed and what seemed most natural back when I was working
on the DPCD helpers. But if we now agree that this is the preferred way
to do things, then we should at least agree that it applies to all areas
for the sake of consistency.

Also, it might be worth looking into improving the structures, and maybe
adding new ones to order things more conveniently or at least group them
in some logical way. In my opinion some of our data structures are
becoming somewhat... unwieldy.

> > Other than that the patches are mostly equivalent, except yours parses
> > more information than just the SCDC bits.
> 
> So merge patch 1 from your series + Shashank's parsing patch? Everyone
> agrees and can you pls cross-r-b stamp so I can apply it all?

I think I spotted a mistake in Shashank's parsing patch. Let me take
another look.

If we can agree on a common way forward on how to deal with this kind of
static data, I have no objections to caching data for the duration of a
hotplug period.

Thierry

> > > -----Original Message-----
> > > From: Thierry Reding [mailto:thierry.reding@gmail.com] 
> > > Sent: Saturday, December 3, 2016 12:54 AM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; Jose Abreu <joabreu@synopsys.com>
> > > Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> > > 
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism for the source and the sink to communicate. Sinks advertise support for this feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 specification, section 10.4 "Status and Control Data Channel". Implement a small helper that find the HF-VSDB and parses it to check if the sink supports SCDC.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v2:
> > > - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
> > >   clarity (Ville Syrjälä)
> > > 
> > >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_edid.h     |  1 +
> > >  include/linux/hdmi.h       |  1 +
> > >  3 files changed, 51 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 336be31ff3de..369961597ee5 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> > >  	return hdmi_id == HDMI_IEEE_OUI;
> > >  }
> > >  
> > > +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> > > +	unsigned int oui;
> > > +
> > > +	if (cea_db_tag(db) != VENDOR_BLOCK)
> > > +		return false;
> > > +
> > > +	if (cea_db_payload_len(db) < 7)
> > > +		return false;
> > > +
> > > +	oui = db[3] << 16 | db[2] << 8 | db[1];
> > > +
> > > +	return oui == HDMI_FORUM_IEEE_OUI;
> > > +}
> > > +
> > >  #define for_each_cea_db(cea, i, start, end) \
> > >  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
> > >  
> > > @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > >  
> > >  /**
> > > + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> > > + * @edid: sink EDID information
> > > + *
> > > + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> > > + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> > > + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> > > + *
> > > + * Returns:
> > > + * True if the sink supports SCDC, false otherwise.
> > > + */
> > > +bool drm_detect_hdmi_scdc(struct edid *edid) {
> > > +	unsigned int start, end, i;
> > > +	const u8 *cea;
> > > +
> > > +	cea = drm_find_cea_extension(edid);
> > > +	if (!cea)
> > > +		return false;
> > > +
> > > +	if (cea_db_offsets(cea, &start, &end))
> > > +		return false;
> > > +
> > > +	for_each_cea_db(cea, i, start, end) {
> > > +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> > > +			if (cea[i + 6] & 0x80)
> > > +				return true;
> > > +		}
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> > > +
> > > +/**
> > >   * drm_detect_monitor_audio - check monitor audio capability
> > >   * @edid: EDID block to scan
> > >   *
> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 38eabf65f19d..7ea7e90846d8 100644
> > > --- a/include/drm/drm_edid.h
> > > +++ b/include/drm/drm_edid.h
> > > @@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> > >  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool drm_detect_hdmi_monitor(struct edid *edid);
> > > +bool drm_detect_hdmi_scdc(struct edid *edid);
> > >  bool drm_detect_monitor_audio(struct edid *edid);  bool drm_rgb_quant_range_selectable(struct edid *edid);  int drm_add_modes_noedid(struct drm_connector *connector, diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 100644
> > > --- a/include/linux/hdmi.h
> > > +++ b/include/linux/hdmi.h
> > > @@ -35,6 +35,7 @@ enum hdmi_infoframe_type {  };
> > >  
> > >  #define HDMI_IEEE_OUI 0x000c03
> > > +#define HDMI_FORUM_IEEE_OUI 0xc45dd8
> > >  #define HDMI_INFOFRAME_HEADER_SIZE  4
> > >  #define HDMI_AVI_INFOFRAME_SIZE    13
> > >  #define HDMI_SPD_INFOFRAME_SIZE    25
> > > --
> > > 2.10.2
> > > 
> 
> 
> 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3904 bytes --]

On Mon, Dec 05, 2016 at 11:06:15AM +0000, Jose Abreu wrote:
> Hi Thierry,
> 
> 
> Do you think while you are at it you could implement a
> set_scrambling() callback? It should be pretty straight forward:
> you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
> again.
> 
> 
> I think this is an important feature that we should have.

Yeah, agreed. I was actually thinking about going one step further and
provide more of the polling functionality as a helper. Even if we have
accessors that wrap the low-level functionality, most drivers would
still have to provide their own delayed workqueue to deal with sinks
(or sources) that don't support read requests. Having this in standard
helpers would help reduce the boilerplate a lot further.

Does your hardware by any chance support read requests on SCDC? It'd be
interesting to see how that works in practice. Unfortunately Tegra does
not seem to support it.

Thierry

> On 02-12-2016 19:24, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Sinks that support SCDC can optionally have the capability to initiate
> > read requests, which are a mechanism by which a sink can notify its
> > source that it should read the Update Flags. If either the sink or the
> > source are not Read Request capable, polling of the Update Flags shall
> > be employed.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - new patch
> >
> >  drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 369961597ee5..8211cce3e09e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3736,6 +3736,42 @@ bool drm_detect_hdmi_scdc(struct edid *edid)
> >  EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> >  
> >  /**
> > + * drm_detect_hdmi_scdc_rr_capable - detect whether an HDMI sink is capable of
> > + *    initiating an SCDC Read Request
> > + * @edid: sink EDID information
> > + *
> > + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> > + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> > + * Block" and checks if the RR_Capable bit (bit 6 of byte 6) is set.
> > + *
> > + * Returns:
> > + * True if the sink is capable of initiating an SCDC Read Request, false
> > + * otherwise.
> > + */
> > +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid)
> > +{
> > +	unsigned int start, end, i;
> > +	const u8 *cea;
> > +
> > +	cea = drm_find_cea_extension(edid);
> > +	if (!cea)
> > +		return false;
> > +
> > +	if (cea_db_offsets(cea, &start, &end))
> > +		return false;
> > +
> > +	for_each_cea_db(cea, i, start, end) {
> > +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> > +			if (cea[i + 6] & 0x40)
> > +				return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +EXPORT_SYMBOL(drm_detect_hdmi_scdc_rr_capable);
> > +
> > +/**
> >   * drm_detect_monitor_audio - check monitor audio capability
> >   * @edid: EDID block to scan
> >   *
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 7ea7e90846d8..d1c29586035e 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -441,6 +441,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
> >  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
> >  bool drm_detect_hdmi_monitor(struct edid *edid);
> >  bool drm_detect_hdmi_scdc(struct edid *edid);
> > +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid);
> >  bool drm_detect_monitor_audio(struct edid *edid);
> >  bool drm_rgb_quant_range_selectable(struct edid *edid);
> >  int drm_add_modes_noedid(struct drm_connector *connector,
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 1/3] drm: Add SCDC helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1762 bytes --]

On Mon, Dec 05, 2016 at 10:12:39AM +0000, Jose Abreu wrote:
> On 02-12-2016 19:24, Thierry Reding wrote:
[...]
> > +/**
> > + * drm_scdc_write - write a block of data to SCDC
> > + * @adapter: I2C controller
> > + * @offset: start offset of block to write
> > + * @buffer: block of data to write
> > + * @size: size of the block to write
> > + *
> > + * Writes a block of data to SCDC, starting at a given offset.
> > + *
> > + * Returns:
> > + * The number of bytes written to SCDC or a negative error code on failure.
> > + */
> > +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> > +		       const void *buffer, size_t size)
> > +{
> > +	struct i2c_msg msg = {
> > +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> > +		.flags = 0,
> > +		.len = 1 + size,
> > +		.buf = NULL,
> > +	};
> > +	void *data;
> > +	int err;
> > +
> > +	data = kmalloc(1 + size, GFP_TEMPORARY);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	msg.buf = data;
> > +
> > +	memcpy(data, &offset, sizeof(offset));
> > +	memcpy(data + 1, buffer, size);
> 
> Don't you agree it would be better if you use the same scheme as
> drm_scdc_read()? Something like:
> 
> struct i2c_msg msgs[] = {
>     {
>         .addr = SCDC_I2C_SLAVE_ADDRESS,
>         .flags = 0,
>         .len = 1,
>         .buf = &offset,
>     }, {
>         .addr = SCDC_I2C_SLAVE_ADDRESS,
>         .flags = 0,
>         .len = size,
>         .buf = buffer,
>     },
> };

Ville had a similar comment on a prior iteration. It looks as if the
above should work, but it's probably best to test it a little more
widely to make sure we're not running into cases where it breaks.

Have you by any chance verified that it works on your hardware?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 1/3] drm: Add SCDC helpers

[Ville Syrjälä]

On Mon, Dec 05, 2016 at 12:16:52PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 10:12:39AM +0000, Jose Abreu wrote:
> > On 02-12-2016 19:24, Thierry Reding wrote:
> [...]
> > > +/**
> > > + * drm_scdc_write - write a block of data to SCDC
> > > + * @adapter: I2C controller
> > > + * @offset: start offset of block to write
> > > + * @buffer: block of data to write
> > > + * @size: size of the block to write
> > > + *
> > > + * Writes a block of data to SCDC, starting at a given offset.
> > > + *
> > > + * Returns:
> > > + * The number of bytes written to SCDC or a negative error code on failure.
> > > + */
> > > +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> > > +		       const void *buffer, size_t size)
> > > +{
> > > +	struct i2c_msg msg = {
> > > +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> > > +		.flags = 0,
> > > +		.len = 1 + size,
> > > +		.buf = NULL,
> > > +	};
> > > +	void *data;
> > > +	int err;
> > > +
> > > +	data = kmalloc(1 + size, GFP_TEMPORARY);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > > +
> > > +	msg.buf = data;
> > > +
> > > +	memcpy(data, &offset, sizeof(offset));
> > > +	memcpy(data + 1, buffer, size);
> > 
> > Don't you agree it would be better if you use the same scheme as
> > drm_scdc_read()? Something like:
> > 
> > struct i2c_msg msgs[] = {
> >     {
> >         .addr = SCDC_I2C_SLAVE_ADDRESS,
> >         .flags = 0,
> >         .len = 1,
> >         .buf = &offset,
> >     }, {
> >         .addr = SCDC_I2C_SLAVE_ADDRESS,
> >         .flags = 0,
> >         .len = size,
> >         .buf = buffer,
> >     },
> > };
> 
> Ville had a similar comment on a prior iteration. It looks as if the
> above should work, but it's probably best to test it a little more
> widely to make sure we're not running into cases where it breaks.

AFAICS the spec says we shouldn't use a repeated start for writes.
I guess it might work for some devices, but going against the spec
sounds questionable to me.

> 
> Have you by any chance verified that it works on your hardware?
> 
> Thierry



-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Ville Syrjälä]

On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > Hi Thierry, 
> > > > 
> > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > https://patchwork.kernel.org/patch/9452259/ 
> > > 
> > > I think there had been pushback before about caching capabilities from
> > > EDID, so from that point of view my patch is more inline with existing
> > > EDID parsing API.
> > 
> > Hm, where was that pushback? We do have a bit a mess between explicitly
> > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> 
> You did object to a very similar patch some time ago that did a similar
> thing for DPCD stuff. And also Villa had commented on an earlier patch
> from Jose about concerns of bloating core structures:
> 
> 	https://patchwork.freedesktop.org/patch/104806/

Yeah, the same old "adding stuff all over without actual users" story.
It's near impossible to judge whether the added things are actually
useful (or if the implementation is the best) without seeing how it's
going to be used.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Add SCDC helpers

[Jose Abreu]

Hi,


On 05-12-2016 13:31, Ville Syrjälä wrote:
> On Mon, Dec 05, 2016 at 12:16:52PM +0100, Thierry Reding wrote:
>> On Mon, Dec 05, 2016 at 10:12:39AM +0000, Jose Abreu wrote:
>>> On 02-12-2016 19:24, Thierry Reding wrote:
>> [...]
>>>> +/**
>>>> + * drm_scdc_write - write a block of data to SCDC
>>>> + * @adapter: I2C controller
>>>> + * @offset: start offset of block to write
>>>> + * @buffer: block of data to write
>>>> + * @size: size of the block to write
>>>> + *
>>>> + * Writes a block of data to SCDC, starting at a given offset.
>>>> + *
>>>> + * Returns:
>>>> + * The number of bytes written to SCDC or a negative error code on failure.
>>>> + */
>>>> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>>> +		       const void *buffer, size_t size)
>>>> +{
>>>> +	struct i2c_msg msg = {
>>>> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
>>>> +		.flags = 0,
>>>> +		.len = 1 + size,
>>>> +		.buf = NULL,
>>>> +	};
>>>> +	void *data;
>>>> +	int err;
>>>> +
>>>> +	data = kmalloc(1 + size, GFP_TEMPORARY);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	msg.buf = data;
>>>> +
>>>> +	memcpy(data, &offset, sizeof(offset));
>>>> +	memcpy(data + 1, buffer, size);
>>> Don't you agree it would be better if you use the same scheme as
>>> drm_scdc_read()? Something like:
>>>
>>> struct i2c_msg msgs[] = {
>>>     {
>>>         .addr = SCDC_I2C_SLAVE_ADDRESS,
>>>         .flags = 0,
>>>         .len = 1,
>>>         .buf = &offset,
>>>     }, {
>>>         .addr = SCDC_I2C_SLAVE_ADDRESS,
>>>         .flags = 0,
>>>         .len = size,
>>>         .buf = buffer,
>>>     },
>>> };
>> Ville had a similar comment on a prior iteration. It looks as if the
>> above should work, but it's probably best to test it a little more
>> widely to make sure we're not running into cases where it breaks.
> AFAICS the spec says we shouldn't use a repeated start for writes.
> I guess it might work for some devices, but going against the spec
> sounds questionable to me.
>
>> Have you by any chance verified that it works on your hardware?
>>
>> Thierry

Actually, I do not do two writes. My I2C controller is accessible
through the HDMI controller and has direct mapping in the regbank
of the slave address, reg address and data. I am using a I2C
adapter driver but it was modified to when the slave address is
SCDC and we are doing the first write then store the data and
only send it in the next write. Something like this:
http://lxr.free-electrons.com/source/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/anx9805.c#L43
(but modified to SCDC).

Anyway, I was not remembering this so just disregard my comment.
We should follow the spec as Ville said.

>
>

Best regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Jose Abreu]

Hi Thierry,


On 05-12-2016 11:14, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 11:06:15AM +0000, Jose Abreu wrote:
>> Hi Thierry,
>>
>>
>> Do you think while you are at it you could implement a
>> set_scrambling() callback? It should be pretty straight forward:
>> you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
>> again.
>>
>>
>> I think this is an important feature that we should have.
> Yeah, agreed. I was actually thinking about going one step further and
> provide more of the polling functionality as a helper. Even if we have
> accessors that wrap the low-level functionality, most drivers would
> still have to provide their own delayed workqueue to deal with sinks
> (or sources) that don't support read requests. Having this in standard
> helpers would help reduce the boilerplate a lot further.
>
> Does your hardware by any chance support read requests on SCDC? It'd be
> interesting to see how that works in practice. Unfortunately Tegra does
> not seem to support it.
>
> Thierry

Yes, my HW supports it though I don't have the setup here to test
right now (but it was used before). In our controller it is
pretty straight forward:
    1) Enable interrupt for rr indication [source]
    2) Enable update read on a rr [source]
    3) Set rr flag in the sink [sink]

Point 2) makes it clear: Whenever we get a rr then the source
will automatically read the sink and dump the contents read in
the regbank. Then the SW gets an interrupt and it should read the
contents of the registers.

>
>> On 02-12-2016 19:24, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Sinks that support SCDC can optionally have the capability to initiate
>>> read requests, which are a mechanism by which a sink can notify its
>>> source that it should read the Update Flags. If either the sink or the
>>> source are not Read Request capable, polling of the Update Flags shall
>>> be employed.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> Changes in v2:
>>> - new patch
>>>
>>>  drivers/gpu/drm/drm_edid.c | 36 ++++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_edid.h     |  1 +
>>>  2 files changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 369961597ee5..8211cce3e09e 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -3736,6 +3736,42 @@ bool drm_detect_hdmi_scdc(struct edid *edid)
>>>  EXPORT_SYMBOL(drm_detect_hdmi_scdc);
>>>  
>>>  /**
>>> + * drm_detect_hdmi_scdc_rr_capable - detect whether an HDMI sink is capable of
>>> + *    initiating an SCDC Read Request
>>> + * @edid: sink EDID information
>>> + *
>>> + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
>>> + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
>>> + * Block" and checks if the RR_Capable bit (bit 6 of byte 6) is set.
>>> + *
>>> + * Returns:
>>> + * True if the sink is capable of initiating an SCDC Read Request, false
>>> + * otherwise.
>>> + */
>>> +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid)
>>> +{
>>> +	unsigned int start, end, i;
>>> +	const u8 *cea;
>>> +
>>> +	cea = drm_find_cea_extension(edid);
>>> +	if (!cea)
>>> +		return false;
>>> +
>>> +	if (cea_db_offsets(cea, &start, &end))
>>> +		return false;
>>> +
>>> +	for_each_cea_db(cea, i, start, end) {
>>> +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
>>> +			if (cea[i + 6] & 0x40)
>>> +				return true;
>>> +		}
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +EXPORT_SYMBOL(drm_detect_hdmi_scdc_rr_capable);
>>> +
>>> +/**
>>>   * drm_detect_monitor_audio - check monitor audio capability
>>>   * @edid: EDID block to scan
>>>   *
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index 7ea7e90846d8..d1c29586035e 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -441,6 +441,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>>>  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
>>>  bool drm_detect_hdmi_monitor(struct edid *edid);
>>>  bool drm_detect_hdmi_scdc(struct edid *edid);
>>> +bool drm_detect_hdmi_scdc_rr_capable(struct edid *edid);
>>>  bool drm_detect_monitor_audio(struct edid *edid);
>>>  bool drm_rgb_quant_range_selectable(struct edid *edid);
>>>  int drm_add_modes_noedid(struct drm_connector *connector,

Best regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Daniel Vetter]

On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > Hi Thierry, 
> > > > 
> > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > https://patchwork.kernel.org/patch/9452259/ 
> > > 
> > > I think there had been pushback before about caching capabilities from
> > > EDID, so from that point of view my patch is more inline with existing
> > > EDID parsing API.
> > 
> > Hm, where was that pushback? We do have a bit a mess between explicitly
> > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> 
> You did object to a very similar patch some time ago that did a similar
> thing for DPCD stuff. And also Villa had commented on an earlier patch
> from Jose about concerns of bloating core structures:
> 
> 	https://patchwork.freedesktop.org/patch/104806/

DPCD I complained about because somehow we ended up with 2 sets of
helpers, one filling a struct and the others returning directly. I
objected to the fact that there's 2 (and imo your patch duplicated even
more), not that I think one approach is clearly inferior to the other.

Demanding that there's some real users is also a valid point.

> > I think long-term stuffing it into drm_display_info is probably better,
> > since then we only have 1 interaction point between the probe code and the
> > atomic_check code. That should be useful for eventually fixing the lack of
> > locking between the two, if I ever get around to that ;-)
> 
> I don't really have objections to caching the results of parsing, it's
> what I had proposed and what seemed most natural back when I was working
> on the DPCD helpers. But if we now agree that this is the preferred way
> to do things, then we should at least agree that it applies to all areas
> for the sake of consistency.
> 
> Also, it might be worth looking into improving the structures, and maybe
> adding new ones to order things more conveniently or at least group them
> in some logical way. In my opinion some of our data structures are
> becoming somewhat... unwieldy.

We're pretty good at consuming fairly invasive refactorings in drm-misc.
So it just boils down to get some agreement on what things should look
like (+1 from my side to parsing stuff into structs as a general idea),
and then massaging all the existing users of the "wrong" interface using
cocci and sed.

Unfortunately "just" ;-)

> > > Other than that the patches are mostly equivalent, except yours parses
> > > more information than just the SCDC bits.
> > 
> > So merge patch 1 from your series + Shashank's parsing patch? Everyone
> > agrees and can you pls cross-r-b stamp so I can apply it all?
> 
> I think I spotted a mistake in Shashank's parsing patch. Let me take
> another look.
> 
> If we can agree on a common way forward on how to deal with this kind of
> static data, I have no objections to caching data for the duration of a
> hotplug period.

Cheers, 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

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2480 bytes --]

On Mon, Dec 05, 2016 at 02:19:48PM +0000, Jose Abreu wrote:
> Hi Thierry,
> 
> 
> On 05-12-2016 11:14, Thierry Reding wrote:
> > On Mon, Dec 05, 2016 at 11:06:15AM +0000, Jose Abreu wrote:
> >> Hi Thierry,
> >>
> >>
> >> Do you think while you are at it you could implement a
> >> set_scrambling() callback? It should be pretty straight forward:
> >> you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
> >> again.
> >>
> >>
> >> I think this is an important feature that we should have.
> > Yeah, agreed. I was actually thinking about going one step further and
> > provide more of the polling functionality as a helper. Even if we have
> > accessors that wrap the low-level functionality, most drivers would
> > still have to provide their own delayed workqueue to deal with sinks
> > (or sources) that don't support read requests. Having this in standard
> > helpers would help reduce the boilerplate a lot further.
> >
> > Does your hardware by any chance support read requests on SCDC? It'd be
> > interesting to see how that works in practice. Unfortunately Tegra does
> > not seem to support it.
> >
> > Thierry
> 
> Yes, my HW supports it though I don't have the setup here to test
> right now (but it was used before). In our controller it is
> pretty straight forward:
>     1) Enable interrupt for rr indication [source]
>     2) Enable update read on a rr [source]
>     3) Set rr flag in the sink [sink]
> 
> Point 2) makes it clear: Whenever we get a rr then the source
> will automatically read the sink and dump the contents read in
> the regbank. Then the SW gets an interrupt and it should read the
> contents of the registers.

Yes, I suspect that there will be three types of setups:

	1) fully supported and integrated RR capability (such as in your
	   case)

	2) external I2C controller with the means of detecting the read
	   request (and signal via an interrupt)

	3) no read request support at all, in which case a delayed work
	   queue is needed to poll periodically


For cases 1) and 3) it's probably only useful to have a helper to enable
scrambling. For 2) there might be some use in having a helper that can
setup the delayed work queue.

Given that we don't have any implementation yet, maybe it's better to
defer the creation of helpers until we see common patterns emerge. The
helper to enable scrambling could be useful in any case, though, so I'll
add one.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4401 bytes --]

On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote:
> On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> > On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > > Hi Thierry, 
> > > > > 
> > > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > > https://patchwork.kernel.org/patch/9452259/ 
> > > > 
> > > > I think there had been pushback before about caching capabilities from
> > > > EDID, so from that point of view my patch is more inline with existing
> > > > EDID parsing API.
> > > 
> > > Hm, where was that pushback? We do have a bit a mess between explicitly
> > > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> > 
> > You did object to a very similar patch some time ago that did a similar
> > thing for DPCD stuff. And also Villa had commented on an earlier patch
> > from Jose about concerns of bloating core structures:
> > 
> > 	https://patchwork.freedesktop.org/patch/104806/
> 
> DPCD I complained about because somehow we ended up with 2 sets of
> helpers, one filling a struct and the others returning directly. I
> objected to the fact that there's 2 (and imo your patch duplicated even
> more), not that I think one approach is clearly inferior to the other.

My recollection is that I had proposed that I do the work of
transitioning users of the parsers to the cached information but you had
said that it wasn't worth the churn and that we should just go with the
existing scheme of passing around the DPCD buffer and extending the
parsers as necessary.

From that I inferred that the same would be true for EDID and since we
already have a couple of helpers that operate on struct edid * and which
return features, continuing that scheme was preferred.

Anyway, I don't really care either way. Maybe you and Ville can duke it
out whether or not we want all of the fields parsed, irrespective of
whether or not they will be used. Then I'll go with whatever you decide.

> Demanding that there's some real users is also a valid point.
> 
> > > I think long-term stuffing it into drm_display_info is probably better,
> > > since then we only have 1 interaction point between the probe code and the
> > > atomic_check code. That should be useful for eventually fixing the lack of
> > > locking between the two, if I ever get around to that ;-)
> > 
> > I don't really have objections to caching the results of parsing, it's
> > what I had proposed and what seemed most natural back when I was working
> > on the DPCD helpers. But if we now agree that this is the preferred way
> > to do things, then we should at least agree that it applies to all areas
> > for the sake of consistency.
> > 
> > Also, it might be worth looking into improving the structures, and maybe
> > adding new ones to order things more conveniently or at least group them
> > in some logical way. In my opinion some of our data structures are
> > becoming somewhat... unwieldy.
> 
> We're pretty good at consuming fairly invasive refactorings in drm-misc.
> So it just boils down to get some agreement on what things should look
> like (+1 from my side to parsing stuff into structs as a general idea),
> and then massaging all the existing users of the "wrong" interface using
> cocci and sed.
> 
> Unfortunately "just" ;-)

I think by now it would be useful to have a nested structure within
struct drm_display_info that contains HDMI specific bits. We already
have a number of candidates that could be extracted into such a
structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(),
drm_rgb_quant_range_selectable(), ...).

Another possibility would be to subclass struct drm_display_info, as
in:

	struct drm_hdmi_info {
		struct drm_display_info display;

		/* HDMI specific information */
		...
	};

Or yet another would be to create struct drm_hdmi_info as a separate
structure and provide a helper which will extract the necessary info
from the EDID. Drivers could then store that in driver-private data
whereas struct drm_display_info could be reduced to the generic bits
that it used to have.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Daniel Vetter]

On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote:
> > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> > > On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > > > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > > > Hi Thierry, 
> > > > > > 
> > > > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > > > https://patchwork.kernel.org/patch/9452259/ 
> > > > > 
> > > > > I think there had been pushback before about caching capabilities from
> > > > > EDID, so from that point of view my patch is more inline with existing
> > > > > EDID parsing API.
> > > > 
> > > > Hm, where was that pushback? We do have a bit a mess between explicitly
> > > > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> > > 
> > > You did object to a very similar patch some time ago that did a similar
> > > thing for DPCD stuff. And also Villa had commented on an earlier patch
> > > from Jose about concerns of bloating core structures:
> > > 
> > > 	https://patchwork.freedesktop.org/patch/104806/
> > 
> > DPCD I complained about because somehow we ended up with 2 sets of
> > helpers, one filling a struct and the others returning directly. I
> > objected to the fact that there's 2 (and imo your patch duplicated even
> > more), not that I think one approach is clearly inferior to the other.
> 
> My recollection is that I had proposed that I do the work of
> transitioning users of the parsers to the cached information but you had
> said that it wasn't worth the churn and that we should just go with the
> existing scheme of passing around the DPCD buffer and extending the
> parsers as necessary.

Hm, I guess it wasn't clear to me that you've offered to do all the
conversions. Doing that would be awesome I think (but quite a bit of
work), and if we bother with it, parsing into a struct is imo the better
idea long-term.

> From that I inferred that the same would be true for EDID and since we
> already have a couple of helpers that operate on struct edid * and which
> return features, continuing that scheme was preferred.
> 
> Anyway, I don't really care either way. Maybe you and Ville can duke it
> out whether or not we want all of the fields parsed, irrespective of
> whether or not they will be used. Then I'll go with whatever you decide.
> 
> > Demanding that there's some real users is also a valid point.
> > 
> > > > I think long-term stuffing it into drm_display_info is probably better,
> > > > since then we only have 1 interaction point between the probe code and the
> > > > atomic_check code. That should be useful for eventually fixing the lack of
> > > > locking between the two, if I ever get around to that ;-)
> > > 
> > > I don't really have objections to caching the results of parsing, it's
> > > what I had proposed and what seemed most natural back when I was working
> > > on the DPCD helpers. But if we now agree that this is the preferred way
> > > to do things, then we should at least agree that it applies to all areas
> > > for the sake of consistency.
> > > 
> > > Also, it might be worth looking into improving the structures, and maybe
> > > adding new ones to order things more conveniently or at least group them
> > > in some logical way. In my opinion some of our data structures are
> > > becoming somewhat... unwieldy.
> > 
> > We're pretty good at consuming fairly invasive refactorings in drm-misc.
> > So it just boils down to get some agreement on what things should look
> > like (+1 from my side to parsing stuff into structs as a general idea),
> > and then massaging all the existing users of the "wrong" interface using
> > cocci and sed.
> > 
> > Unfortunately "just" ;-)
> 
> I think by now it would be useful to have a nested structure within
> struct drm_display_info that contains HDMI specific bits. We already
> have a number of candidates that could be extracted into such a
> structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(),
> drm_rgb_quant_range_selectable(), ...).
> 
> Another possibility would be to subclass struct drm_display_info, as
> in:
> 
> 	struct drm_hdmi_info {
> 		struct drm_display_info display;
> 
> 		/* HDMI specific information */
> 		...
> 	};
> 
> Or yet another would be to create struct drm_hdmi_info as a separate
> structure and provide a helper which will extract the necessary info
> from the EDID. Drivers could then store that in driver-private data
> whereas struct drm_display_info could be reduced to the generic bits
> that it used to have.

I think nested drm_hdmi_info within drm_display_info sounds like a fine
idea.
-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

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Daniel Vetter]

On Mon, Dec 05, 2016 at 05:37:22PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 02:19:48PM +0000, Jose Abreu wrote:
> > Hi Thierry,
> > 
> > 
> > On 05-12-2016 11:14, Thierry Reding wrote:
> > > On Mon, Dec 05, 2016 at 11:06:15AM +0000, Jose Abreu wrote:
> > >> Hi Thierry,
> > >>
> > >>
> > >> Do you think while you are at it you could implement a
> > >> set_scrambling() callback? It should be pretty straight forward:
> > >> you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
> > >> again.
> > >>
> > >>
> > >> I think this is an important feature that we should have.
> > > Yeah, agreed. I was actually thinking about going one step further and
> > > provide more of the polling functionality as a helper. Even if we have
> > > accessors that wrap the low-level functionality, most drivers would
> > > still have to provide their own delayed workqueue to deal with sinks
> > > (or sources) that don't support read requests. Having this in standard
> > > helpers would help reduce the boilerplate a lot further.
> > >
> > > Does your hardware by any chance support read requests on SCDC? It'd be
> > > interesting to see how that works in practice. Unfortunately Tegra does
> > > not seem to support it.
> > >
> > > Thierry
> > 
> > Yes, my HW supports it though I don't have the setup here to test
> > right now (but it was used before). In our controller it is
> > pretty straight forward:
> >     1) Enable interrupt for rr indication [source]
> >     2) Enable update read on a rr [source]
> >     3) Set rr flag in the sink [sink]
> > 
> > Point 2) makes it clear: Whenever we get a rr then the source
> > will automatically read the sink and dump the contents read in
> > the regbank. Then the SW gets an interrupt and it should read the
> > contents of the registers.
> 
> Yes, I suspect that there will be three types of setups:
> 
> 	1) fully supported and integrated RR capability (such as in your
> 	   case)
> 
> 	2) external I2C controller with the means of detecting the read
> 	   request (and signal via an interrupt)
> 
> 	3) no read request support at all, in which case a delayed work
> 	   queue is needed to poll periodically
> 
> 
> For cases 1) and 3) it's probably only useful to have a helper to enable
> scrambling. For 2) there might be some use in having a helper that can
> setup the delayed work queue.
> 
> Given that we don't have any implementation yet, maybe it's better to
> defer the creation of helpers until we see common patterns emerge. The
> helper to enable scrambling could be useful in any case, though, so I'll
> add one.

So no idea how this works, but how is a read request signalled? On the DP
side there's just a short pulse hpd when the sink wants the source's
attention. And iirc hdcp uses similar tricks when e.g. a downstream device
has changed. We don't yet have it for DP, but some helper to handle hdp
interrupts with a parameter to indicate long/short pulse is probably all
that's really needed. But we don't yet have that for DP either :(
-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

Re: [PATCH v2 3/3] drm/edid: Implement SCDC Read Request capability detection

[Jose Abreu]

Hi Daniel,


On 06-12-2016 08:23, Daniel Vetter wrote:
> On Mon, Dec 05, 2016 at 05:37:22PM +0100, Thierry Reding wrote:
>> On Mon, Dec 05, 2016 at 02:19:48PM +0000, Jose Abreu wrote:
>>> Hi Thierry,
>>>
>>>
>>> On 05-12-2016 11:14, Thierry Reding wrote:
>>>> On Mon, Dec 05, 2016 at 11:06:15AM +0000, Jose Abreu wrote:
>>>>> Hi Thierry,
>>>>>
>>>>>
>>>>> Do you think while you are at it you could implement a
>>>>> set_scrambling() callback? It should be pretty straight forward:
>>>>> you read the SCDC_TMDS_CONFIG reg, do a mask, and then write it
>>>>> again.
>>>>>
>>>>>
>>>>> I think this is an important feature that we should have.
>>>> Yeah, agreed. I was actually thinking about going one step further and
>>>> provide more of the polling functionality as a helper. Even if we have
>>>> accessors that wrap the low-level functionality, most drivers would
>>>> still have to provide their own delayed workqueue to deal with sinks
>>>> (or sources) that don't support read requests. Having this in standard
>>>> helpers would help reduce the boilerplate a lot further.
>>>>
>>>> Does your hardware by any chance support read requests on SCDC? It'd be
>>>> interesting to see how that works in practice. Unfortunately Tegra does
>>>> not seem to support it.
>>>>
>>>> Thierry
>>> Yes, my HW supports it though I don't have the setup here to test
>>> right now (but it was used before). In our controller it is
>>> pretty straight forward:
>>>     1) Enable interrupt for rr indication [source]
>>>     2) Enable update read on a rr [source]
>>>     3) Set rr flag in the sink [sink]
>>>
>>> Point 2) makes it clear: Whenever we get a rr then the source
>>> will automatically read the sink and dump the contents read in
>>> the regbank. Then the SW gets an interrupt and it should read the
>>> contents of the registers.
>> Yes, I suspect that there will be three types of setups:
>>
>> 	1) fully supported and integrated RR capability (such as in your
>> 	   case)
>>
>> 	2) external I2C controller with the means of detecting the read
>> 	   request (and signal via an interrupt)
>>
>> 	3) no read request support at all, in which case a delayed work
>> 	   queue is needed to poll periodically
>>
>>
>> For cases 1) and 3) it's probably only useful to have a helper to enable
>> scrambling. For 2) there might be some use in having a helper that can
>> setup the delayed work queue.
>>
>> Given that we don't have any implementation yet, maybe it's better to
>> defer the creation of helpers until we see common patterns emerge. The
>> helper to enable scrambling could be useful in any case, though, so I'll
>> add one.
> So no idea how this works, but how is a read request signalled? 

On HDMI this is done in the I2C line. When the sink wants the
source attention it pulses the SDA low when the bus is free. This
should be correctly handled by the I2C controller that is
connected or included in the HDMI controller.

> On the DP
> side there's just a short pulse hpd when the sink wants the source's
> attention. And iirc hdcp uses similar tricks when e.g. a downstream device
> has changed. We don't yet have it for DP, but some helper to handle hdp
> interrupts with a parameter to indicate long/short pulse is probably all
> that's really needed. But we don't yet have that for DP either :(
> -Daniel

Best regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Jani Nikula]

On Tue, 06 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote:
>> On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote:
>> > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
>> > > On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
>> > > > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
>> > > > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
>> > > > > > Hi Thierry, 
>> > > > > > 
>> > > > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
>> > > > > > https://patchwork.kernel.org/patch/9452259/ 
>> > > > > 
>> > > > > I think there had been pushback before about caching capabilities from
>> > > > > EDID, so from that point of view my patch is more inline with existing
>> > > > > EDID parsing API.
>> > > > 
>> > > > Hm, where was that pushback? We do have a bit a mess between explicitly
>> > > > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
>> > > 
>> > > You did object to a very similar patch some time ago that did a similar
>> > > thing for DPCD stuff. And also Villa had commented on an earlier patch
>> > > from Jose about concerns of bloating core structures:
>> > > 
>> > > 	https://patchwork.freedesktop.org/patch/104806/
>> > 
>> > DPCD I complained about because somehow we ended up with 2 sets of
>> > helpers, one filling a struct and the others returning directly. I
>> > objected to the fact that there's 2 (and imo your patch duplicated even
>> > more), not that I think one approach is clearly inferior to the other.
>> 
>> My recollection is that I had proposed that I do the work of
>> transitioning users of the parsers to the cached information but you had
>> said that it wasn't worth the churn and that we should just go with the
>> existing scheme of passing around the DPCD buffer and extending the
>> parsers as necessary.
>
> Hm, I guess it wasn't clear to me that you've offered to do all the
> conversions. Doing that would be awesome I think (but quite a bit of
> work), and if we bother with it, parsing into a struct is imo the better
> idea long-term.

I'm concerned about invalidating the data in the structs at the right
times. We keep having issues with that whenever we cache stuff.

BR,
Jani.



>
>> From that I inferred that the same would be true for EDID and since we
>> already have a couple of helpers that operate on struct edid * and which
>> return features, continuing that scheme was preferred.
>> 
>> Anyway, I don't really care either way. Maybe you and Ville can duke it
>> out whether or not we want all of the fields parsed, irrespective of
>> whether or not they will be used. Then I'll go with whatever you decide.
>> 
>> > Demanding that there's some real users is also a valid point.
>> > 
>> > > > I think long-term stuffing it into drm_display_info is probably better,
>> > > > since then we only have 1 interaction point between the probe code and the
>> > > > atomic_check code. That should be useful for eventually fixing the lack of
>> > > > locking between the two, if I ever get around to that ;-)
>> > > 
>> > > I don't really have objections to caching the results of parsing, it's
>> > > what I had proposed and what seemed most natural back when I was working
>> > > on the DPCD helpers. But if we now agree that this is the preferred way
>> > > to do things, then we should at least agree that it applies to all areas
>> > > for the sake of consistency.
>> > > 
>> > > Also, it might be worth looking into improving the structures, and maybe
>> > > adding new ones to order things more conveniently or at least group them
>> > > in some logical way. In my opinion some of our data structures are
>> > > becoming somewhat... unwieldy.
>> > 
>> > We're pretty good at consuming fairly invasive refactorings in drm-misc.
>> > So it just boils down to get some agreement on what things should look
>> > like (+1 from my side to parsing stuff into structs as a general idea),
>> > and then massaging all the existing users of the "wrong" interface using
>> > cocci and sed.
>> > 
>> > Unfortunately "just" ;-)
>> 
>> I think by now it would be useful to have a nested structure within
>> struct drm_display_info that contains HDMI specific bits. We already
>> have a number of candidates that could be extracted into such a
>> structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(),
>> drm_rgb_quant_range_selectable(), ...).
>> 
>> Another possibility would be to subclass struct drm_display_info, as
>> in:
>> 
>> 	struct drm_hdmi_info {
>> 		struct drm_display_info display;
>> 
>> 		/* HDMI specific information */
>> 		...
>> 	};
>> 
>> Or yet another would be to create struct drm_hdmi_info as a separate
>> structure and provide a helper which will extract the necessary info
>> from the EDID. Drivers could then store that in driver-private data
>> whereas struct drm_display_info could be reduced to the generic bits
>> that it used to have.
>
> I think nested drm_hdmi_info within drm_display_info sounds like a fine
> idea.
> -Daniel

-- 
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 v2 2/3] drm/edid: Implement SCDC support detection

[Sharma, Shashank]

Thanks Thierry and Daniel for concluding this discussion, while I was on vacation. 

Here are the next steps, which I am planning to go for (Please correct me if my understanding is not right)
-  Shashank to review first of the SCDC patch, and give comments or R-B. 
-  Shashank to work on Thierry's review comments on HF-VSDB parsing patch (including creation of a sub-class nested drm_hdmi_info within drm_display_info) and publish V2
- Thierry/Daniel to review V2 and give comments or R-B

Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, December 6, 2016 1:49 PM
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank <shashank.sharma@intel.com>; Jose Abreu <joabreu@synopsys.com>; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote:
> > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> > > On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > > > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > > > Hi Thierry,
> > > > > > 
> > > > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > > > https://patchwork.kernel.org/patch/9452259/
> > > > > 
> > > > > I think there had been pushback before about caching 
> > > > > capabilities from EDID, so from that point of view my patch is 
> > > > > more inline with existing EDID parsing API.
> > > > 
> > > > Hm, where was that pushback? We do have a bit a mess between 
> > > > explicitly parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> > > 
> > > You did object to a very similar patch some time ago that did a 
> > > similar thing for DPCD stuff. And also Villa had commented on an 
> > > earlier patch from Jose about concerns of bloating core structures:
> > > 
> > > 	https://patchwork.freedesktop.org/patch/104806/
> > 
> > DPCD I complained about because somehow we ended up with 2 sets of 
> > helpers, one filling a struct and the others returning directly. I 
> > objected to the fact that there's 2 (and imo your patch duplicated 
> > even more), not that I think one approach is clearly inferior to the other.
> 
> My recollection is that I had proposed that I do the work of 
> transitioning users of the parsers to the cached information but you 
> had said that it wasn't worth the churn and that we should just go 
> with the existing scheme of passing around the DPCD buffer and 
> extending the parsers as necessary.

Hm, I guess it wasn't clear to me that you've offered to do all the conversions. Doing that would be awesome I think (but quite a bit of work), and if we bother with it, parsing into a struct is imo the better idea long-term.

> From that I inferred that the same would be true for EDID and since we 
> already have a couple of helpers that operate on struct edid * and 
> which return features, continuing that scheme was preferred.
> 
> Anyway, I don't really care either way. Maybe you and Ville can duke 
> it out whether or not we want all of the fields parsed, irrespective 
> of whether or not they will be used. Then I'll go with whatever you decide.
> 
> > Demanding that there's some real users is also a valid point.
> > 
> > > > I think long-term stuffing it into drm_display_info is probably 
> > > > better, since then we only have 1 interaction point between the 
> > > > probe code and the atomic_check code. That should be useful for 
> > > > eventually fixing the lack of locking between the two, if I ever 
> > > > get around to that ;-)
> > > 
> > > I don't really have objections to caching the results of parsing, 
> > > it's what I had proposed and what seemed most natural back when I 
> > > was working on the DPCD helpers. But if we now agree that this is 
> > > the preferred way to do things, then we should at least agree that 
> > > it applies to all areas for the sake of consistency.
> > > 
> > > Also, it might be worth looking into improving the structures, and 
> > > maybe adding new ones to order things more conveniently or at 
> > > least group them in some logical way. In my opinion some of our 
> > > data structures are becoming somewhat... unwieldy.
> > 
> > We're pretty good at consuming fairly invasive refactorings in drm-misc.
> > So it just boils down to get some agreement on what things should 
> > look like (+1 from my side to parsing stuff into structs as a 
> > general idea), and then massaging all the existing users of the 
> > "wrong" interface using cocci and sed.
> > 
> > Unfortunately "just" ;-)
> 
> I think by now it would be useful to have a nested structure within 
> struct drm_display_info that contains HDMI specific bits. We already 
> have a number of candidates that could be extracted into such a 
> structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), 
> drm_rgb_quant_range_selectable(), ...).
> 
> Another possibility would be to subclass struct drm_display_info, as
> in:
> 
> 	struct drm_hdmi_info {
> 		struct drm_display_info display;
> 
> 		/* HDMI specific information */
> 		...
> 	};
> 
> Or yet another would be to create struct drm_hdmi_info as a separate 
> structure and provide a helper which will extract the necessary info 
> from the EDID. Drivers could then store that in driver-private data 
> whereas struct drm_display_info could be reduced to the generic bits 
> that it used to have.

I think nested drm_hdmi_info within drm_display_info sounds like a fine idea.
-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