[PATCH v4 0/4] Add SCDC information to connector debugfs

[PATCH v4 0/4] Add SCDC information to connector debugfs

[Nicolas Frattaroli]

HDMI uses the DDC I2C bus for communicating various bits of link status
out of band with the actual HDMI video signal. This information can be
useful for debugging issues like questionable cables sabotaged by feline
teeth, Enthusiast Grade cables made of cow fencing wire, and other such
problems that ruin one's media viewing plans.

Consequently, this series exposes various bits of pertinent information
from the SCDC protocol in an HDMI connector's debugfs. To continually
poll the link status, userspace can poll the debugfs file.

---
Changes in v4:
- Don't use C struct bitfields for parsing status flags. Switch to
  bitwise AND for boolean flags, and FIELD_GET for multi-bit values.
- Drop the superfluous !! and parens
- Drop the __pure attributes on static functions
- Initialise stack local arrays with {}, not { 0 }.
- I've kept the print macros and %-30s format. Reason being that I don't
  want to repeat the format specifier and str_yes_no(foo) a bunch, and I
  like the %-30s format because it means all values are aligned with the
  value of the longest field, which is 30 chars long.
- Link to v3: https://patch.msgid.link/20260526-scdc-link-health-v3-0-59e4a4aaead1@collabora.com

Changes in v3:
- Add patch to change return type of drm_scdc_read/write.
- Rework error counter reading to duplicate less code.
- Also check lane 3 counter valid flag when reading its error counter.
- Use memset to clear buf for error counters, rather than doing it in
  the loop.
- Make read_error_counters not accept 0 as num_lanes; fix it up in the
  caller instead.
- Link to v2: https://patch.msgid.link/20260520-scdc-link-health-v2-0-511af18cd64b@collabora.com

Changes in v2:
- Add HDMI 2.1 SCDC status reporting
- Link to v1: https://patch.msgid.link/20260415-scdc-link-health-v1-0-8e731e88eaf0@collabora.com

To: Jani Nikula <jani.nikula@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Daniel Stone <daniel@fooishbar.org>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel@collabora.com
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

---
Nicolas Frattaroli (4):
      drm/scdc-helper: Don't use ssize_t return type for scdc_read/write
      drm/scdc-helper: Add scdc_status debugfs entry
      drm/display: bridge_connector: init scdc debugfs for HDMI
      drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

 drivers/gpu/drm/display/drm_bridge_connector.c |   4 +
 drivers/gpu/drm/display/drm_scdc_helper.c      | 377 ++++++++++++++++++++++++-
 include/drm/display/drm_scdc.h                 |  16 +-
 include/drm/display/drm_scdc_helper.h          | 105 ++++++-
 4 files changed, 493 insertions(+), 9 deletions(-)
---
base-commit: 971288d0e77f7a0d356287121ce549ab5dad570a
change-id: 20260413-scdc-link-health-89326013d96c

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v4 1/4] drm/scdc-helper: Don't use ssize_t return type for scdc_read/write

[Nicolas Frattaroli]

drm_scdc_read and drm_scdc_write, both of which are only used within
drm_scdc_helper (although exported), use a ssize_t as their return type.

This would make sense if they returned the number of bytes read/written
on success, and negative errno otherwise. However, they return 0 on
success.

Demote them to "int" as their return type, in order to avoid needlessly
using 64 bits when less suffices.

No functional change.

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 8 ++++----
 include/drm/display/drm_scdc_helper.h     | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index df878aad4a36..8403f2390ab6 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -67,8 +67,8 @@
  * Returns:
  * 0 on success, negative error code on failure.
  */
-ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
-		      size_t size)
+int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		  size_t size)
 {
 	int ret;
 	struct i2c_msg msgs[2] = {
@@ -107,8 +107,8 @@ EXPORT_SYMBOL(drm_scdc_read);
  * Returns:
  * 0 on success, negative error code on failure.
  */
-ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
-		       const void *buffer, size_t size)
+int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
+		   size_t size)
 {
 	struct i2c_msg msg = {
 		.addr = SCDC_I2C_SLAVE_ADDRESS,
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index 34600476a1b9..e9ccaeba56dd 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -31,10 +31,10 @@
 struct drm_connector;
 struct i2c_adapter;
 
-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);
+int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
+		  size_t size);
+int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
+		   size_t size);
 
 /**
  * drm_scdc_readb - read a single byte from SCDC

-- 
2.54.0

[PATCH v4 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Nicolas Frattaroli]

SCDC provides status information on the current display link. At the
very least, it may be useful to expose this info through debugfs.

Add a debugfs entry for it under the connector, which displays a few
more details parsed out of the SCDC registers. A new
drm_scdc_debugfs_init function can be called by the connector
implementation to initialise the debugfs file.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 237 ++++++++++++++++++++++++++++++
 include/drm/display/drm_scdc_helper.h     |  32 ++++
 2 files changed, 269 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index 8403f2390ab6..7739fb5e77a1 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -24,11 +24,14 @@
 #include <linux/export.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
+#include <linux/overflow.h>
 
 #include <drm/display/drm_scdc_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_device.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
 /**
@@ -55,6 +58,11 @@
 
 #define SCDC_I2C_SLAVE_ADDRESS 0x54
 
+struct scdc_debugfs_priv {
+	struct drm_connector *connector;
+	struct drm_scdc_state state;
+};
+
 /**
  * drm_scdc_read - read a block of data from SCDC
  * @adapter: I2C controller
@@ -276,3 +284,232 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
 	return true;
 }
 EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
+
+/**
+ * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
+ *
+ * Reads the SCDC Status Flags 0 register, and outputs its contents to the
+ * destination @flags. Contents of @flags are only valid if function returns 0.
+ *
+ * Returns: %0 on success, negative errno on error.
+ */
+int drm_scdc_read_status0_flags(struct drm_connector *connector,
+				struct drm_scdc_status_flags *flags)
+{
+	int ret;
+	u8 val;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
+	if (ret)
+		return ret;
+
+	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &val);
+	if (ret)
+		return ret;
+
+	flags->clock_detected = val & SCDC_CLOCK_DETECT;
+	flags->ch0_locked = val & SCDC_CH0_LOCK;
+	flags->ch1_locked = val & SCDC_CH1_LOCK;
+	flags->ch2_locked = val & SCDC_CH2_LOCK;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_scdc_read_status0_flags);
+
+/**
+ * drm_scdc_read_error_counters - Read and clear SCDC error counters
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @counter: Caller's u16 array with 3 elements to write the counter values into
+ *
+ * Read the SCDC channel error counters. If the count of channel *n* is valid,
+ * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
+ * in one read chunk, then clears every counter, as is mandated.
+ *
+ * Returns: %0 on success, negative errno on error.
+ */
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
+{
+	u8 buf[7] = {};
+	int ret;
+	u8 sum = 0;
+	int i;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
+	if (ret)
+		return ret;
+
+	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	if (ret)
+		return ret;
+
+	/*
+	 * Verify the "checksum", i.e. sum up everything including the checksum
+	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
+	 */
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		sum = wrapping_add(u8, sum, buf[i]);
+
+	if (sum)
+		return -EPROTO;
+
+	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
+		if (buf[i + 1] & SCDC_CHANNEL_VALID)
+			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
+		else
+			counter[i / 2] = 0;
+
+		buf[i] = 0;
+		buf[i + 1] = 0;
+	}
+	buf[ARRAY_SIZE(buf) - 1] = 0;
+
+	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+}
+EXPORT_SYMBOL(drm_scdc_read_error_counters);
+
+/**
+ * drm_scdc_read_state - Update state from SCDC
+ * @connector: pointer to a &struct drm_connector on which to operate on
+ * @state: pointer to a &struct drm_scdc_state to fill
+ *
+ * Reads update flags from SCDC, and updates the parts of @state that SCDC
+ * claims have changed, as well as populating those where such a distinction
+ * can't be made.
+ *
+ * Returns: %0 on success, negative errno on failure.
+ */
+int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
+{
+	u8 upd_flags[2] = {};
+	struct i2c_adapter *ddc;
+	struct drm_scdc *scdc;
+	int ret;
+	u8 val;
+
+	if (!state || !connector)
+		return -ENODEV;
+
+	scdc = &connector->display_info.hdmi.scdc;
+	ddc = connector->ddc;
+
+	if (!scdc->supported)
+		return -EOPNOTSUPP;
+
+	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
+	if (ret)
+		return ret;
+
+	state->scrambling_enabled = val & SCDC_SCRAMBLING_ENABLE;
+	state->tmds_bclk_x40 = val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
+
+	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
+
+	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
+	if (ret)
+		return ret;
+
+	if (upd_flags[0] & SCDC_STATUS_UPDATE) {
+		ret = drm_scdc_read_status0_flags(connector, &state->stf);
+		if (ret)
+			return ret;
+	}
+
+	if (upd_flags[0] & SCDC_CED_UPDATE) {
+		ret = drm_scdc_read_error_counters(connector, state->error_count);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_scdc_read_state);
+
+#define scdc_print_str(_f, key, s) \
+	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
+#define scdc_print_flag(_f, key, val) \
+	(scdc_print_str((_f), (key), str_yes_no((val))))
+#define scdc_print_dec(_f, key, val) \
+	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
+
+static int scdc_status_show(struct seq_file *m, void *data)
+{
+	struct scdc_debugfs_priv *priv = m->private;
+	struct drm_scdc_state *st = &priv->state;
+	struct drm_connector *connector = priv->connector;
+	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
+	int ret;
+
+	drm_connector_get(connector);
+
+	if (connector->status != connector_status_connected) {
+		ret = -ENODEV;
+		goto err_conn_put;
+	}
+
+	scdc_print_flag(m, "SCDC Supported", scdc->supported);
+	if (!scdc->supported) {
+		ret = 0;
+		goto err_conn_put;
+	}
+
+	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
+	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
+	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
+
+	ret = drm_scdc_read_state(connector, st);
+	drm_connector_put(connector);
+	if (ret)
+		return ret;
+
+	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
+	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
+
+	if (st->tmds_bclk_x40)
+		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
+	else
+		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
+
+	scdc_print_flag(m, "Clock Detected", st->stf.clock_detected);
+	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
+	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
+	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
+
+	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
+	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
+	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
+
+	return 0;
+
+err_conn_put:
+	drm_connector_put(connector);
+
+	return ret;
+}
+DEFINE_SHOW_ATTRIBUTE(scdc_status);
+
+/**
+ * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
+ * @connector: pointer to &struct drm_connector to operate on
+ * @root: debugfs &struct dentry for the debugfs root of @connector
+ *
+ * Creates SCDC-related debugfs files for @connector. Must be called after
+ * @root is already created.
+ */
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
+{
+	struct scdc_debugfs_priv *priv;
+
+	if (!root || !connector)
+		return;
+
+	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return;
+
+	priv->connector = connector;
+
+	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
+}
+EXPORT_SYMBOL(drm_scdc_debugfs_init);
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index e9ccaeba56dd..3b3a4e0e48ba 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -30,6 +30,31 @@
 
 struct drm_connector;
 struct i2c_adapter;
+struct dentry;
+
+struct drm_scdc_status_flags {
+	/* Status Register 0 */
+	bool clock_detected;
+	bool ch0_locked;
+	bool ch1_locked;
+	bool ch2_locked;
+};
+
+struct drm_scdc_state {
+	/** @stf: contents of the status flag registers */
+	struct drm_scdc_status_flags stf;
+	/** @scramling_enabled: true if TMDS scrambling is on */
+	bool scrambling_enabled;
+	/** @scrambling_detected: true if the sink actually detected scrambling */
+	bool scrambling_detected;
+	/**
+	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
+	 * clock period, false if it's 1/10th of the clock period.
+	 */
+	bool tmds_bclk_x40;
+	/** @error_count: character error counts for each channel */
+	u16 error_count[3];
+};
 
 int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
 		  size_t size);
@@ -77,4 +102,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
 bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
 bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
 
+int drm_scdc_read_status0_flags(struct drm_connector *connector,
+				struct drm_scdc_status_flags *flags);
+int drm_scdc_read_state(struct drm_connector *connector,
+			struct drm_scdc_state *state);
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
+
 #endif

-- 
2.54.0

[PATCH v4 3/4] drm/display: bridge_connector: init scdc debugfs for HDMI

[Nicolas Frattaroli]

On drm_bridge_connectors that contain an HDMI bridge, initialise the
SCDC debugfs entry under the connector's debugfs root.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 649969fca141..8c2c890bd95d 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -25,6 +25,7 @@
 #include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
+#include <drm/display/drm_scdc_helper.h>
 
 /**
  * DOC: overview
@@ -263,6 +264,9 @@ static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
 		if (bridge->funcs->debugfs_init)
 			bridge->funcs->debugfs_init(bridge, root);
 	}
+
+	if (bridge_connector->bridge_hdmi)
+		drm_scdc_debugfs_init(connector, root);
 }
 
 static void drm_bridge_connector_reset(struct drm_connector *connector)

-- 
2.54.0

[PATCH v4 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Nicolas Frattaroli]

HDMI 2.1 redefines previously reserved fields in SCDC for various new
uses. No version check needs to be performed, as an HDMI 2.0 sink's
reserved SCDC fields are well-defined to be 0, and any zero-ness of
these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
HDMI 2.1.

Implement reading and outputting these fields over debugfs.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 164 +++++++++++++++++++++++++++---
 include/drm/display/drm_scdc.h            |  16 ++-
 include/drm/display/drm_scdc_helper.h     |  71 ++++++++++++-
 3 files changed, 231 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index 7739fb5e77a1..6e2ad335296e 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/bitfield.h>
 #include <linux/export.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
@@ -63,6 +64,38 @@ struct scdc_debugfs_priv {
 	struct drm_scdc_state state;
 };
 
+static const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)
+{
+	switch (rate) {
+	case SCDC_FRL_RATE_OFF:
+		return "Off";
+	case SCDC_FRL_RATE_3X3:
+		return "3 Gbit/s x 3 lanes";
+	case SCDC_FRL_RATE_6X3:
+		return "6 Gbit/s x 3 lanes";
+	case SCDC_FRL_RATE_6X4:
+		return "6 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_8X4:
+		return "8 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_10X4:
+		return "10 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_12X4:
+		return "12 Gbit/s x 4 lanes";
+	case SCDC_FRL_RATE_RESV_7:
+	case SCDC_FRL_RATE_RESV_8:
+	case SCDC_FRL_RATE_RESV_9:
+	case SCDC_FRL_RATE_RESV_10:
+	case SCDC_FRL_RATE_RESV_11:
+	case SCDC_FRL_RATE_RESV_12:
+	case SCDC_FRL_RATE_RESV_13:
+	case SCDC_FRL_RATE_RESV_14:
+	case SCDC_FRL_RATE_RESV_15:
+		return "(Reserved)";
+	default:
+		return NULL;
+	}
+}
+
 /**
  * drm_scdc_read - read a block of data from SCDC
  * @adapter: I2C controller
@@ -313,15 +346,72 @@ int drm_scdc_read_status0_flags(struct drm_connector *connector,
 	flags->ch0_locked = val & SCDC_CH0_LOCK;
 	flags->ch1_locked = val & SCDC_CH1_LOCK;
 	flags->ch2_locked = val & SCDC_CH2_LOCK;
+	flags->ln3_locked = val & SCDC_LN3_LOCK;
+	flags->flt_ready = val & SCDC_FLT_READY;
+	flags->dsc_fail = val & SCDC_DSC_FAIL;
 
 	return 0;
 }
 EXPORT_SYMBOL(drm_scdc_read_status0_flags);
 
+/**
+ * drm_scdc_read_status1_2_flags - Read SCDC "Status Flags" 1 and 2 Registers
+ * @connector: pointer to &struct drm_connector to issue the scdc request on
+ * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
+ *
+ * Reads the SCDC Status Flags 1 and 2 registers, and outputs their contents to
+ * the destination @flags. Contents of @flags are only valid if function returns
+ * 0.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
+				  struct drm_scdc_status_flags *flags)
+{
+	u8 val[2] = {};
+	int ret;
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_FLT_UPDATE);
+	if (ret)
+		return ret;
+
+	ret = drm_scdc_read(connector->ddc, SCDC_STATUS_FLAGS_1, val, 2);
+	if (ret)
+		return ret;
+
+	flags->ln0_training_pattern = FIELD_GET(SCDC_LN_EVEN_TRAIN_PTRN, val[0]);
+	flags->ln1_training_pattern = FIELD_GET(SCDC_LN_ODD_TRAIN_PTRN, val[0]);
+
+	flags->ln2_training_pattern = FIELD_GET(SCDC_LN_EVEN_TRAIN_PTRN, val[1]);
+	flags->ln3_training_pattern = FIELD_GET(SCDC_LN_ODD_TRAIN_PTRN, val[1]);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_scdc_read_status1_2_flags);
+
+#define ERR_DET_OFF(x) ((x) - SCDC_ERR_DET_0_L)
+
+static int scdc_err_cnt_buf_idx(unsigned int lane)
+{
+	switch (lane) {
+	case 0:
+		return ERR_DET_OFF(SCDC_ERR_DET_0_L);
+	case 1:
+		return ERR_DET_OFF(SCDC_ERR_DET_1_L);
+	case 2:
+		return ERR_DET_OFF(SCDC_ERR_DET_2_L);
+	case 3:
+		return ERR_DET_OFF(SCDC_ERR_DET_3_L);
+	default:
+		return -EINVAL;
+	}
+}
+
 /**
  * drm_scdc_read_error_counters - Read and clear SCDC error counters
  * @connector: pointer to &struct drm_connector to issue the scdc request on
- * @counter: Caller's u16 array with 3 elements to write the counter values into
+ * @counter: Caller's u16 array with 4 elements to write the counter values into
+ * @num_lanes: number of active lanes, either 3 or 4
  *
  * Read the SCDC channel error counters. If the count of channel *n* is valid,
  * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
@@ -329,18 +419,31 @@ EXPORT_SYMBOL(drm_scdc_read_status0_flags);
  *
  * Returns: %0 on success, negative errno on error.
  */
-int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
+				 unsigned int num_lanes)
 {
-	u8 buf[7] = {};
+	unsigned int buf_sz;
+	u8 buf[9] = {};
 	int ret;
 	u8 sum = 0;
-	int i;
+	int i, idx;
+
+	switch (num_lanes) {
+	case 3:
+		buf_sz = 7;
+		break;
+	case 4:
+		buf_sz = 9;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
 	if (ret)
 		return ret;
 
-	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
 	if (ret)
 		return ret;
 
@@ -348,24 +451,23 @@ int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]
 	 * Verify the "checksum", i.e. sum up everything including the checksum
 	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
 	 */
-	for (i = 0; i < ARRAY_SIZE(buf); i++)
+	for (i = 0; i < buf_sz; i++)
 		sum = wrapping_add(u8, sum, buf[i]);
 
 	if (sum)
 		return -EPROTO;
 
-	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
-		if (buf[i + 1] & SCDC_CHANNEL_VALID)
-			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
+	for (i = 0; i < num_lanes; i++) {
+		idx = scdc_err_cnt_buf_idx(i);
+		if (buf[idx + 1] & SCDC_CHANNEL_VALID)
+			counter[i] = buf[idx] | (buf[idx + 1] & ~SCDC_CHANNEL_VALID) << 8;
 		else
-			counter[i / 2] = 0;
-
-		buf[i] = 0;
-		buf[i + 1] = 0;
+			counter[i] = 0;
 	}
-	buf[ARRAY_SIZE(buf) - 1] = 0;
 
-	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
+	memset(buf, 0, buf_sz);
+
+	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
 }
 EXPORT_SYMBOL(drm_scdc_read_error_counters);
 
@@ -385,6 +487,7 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 	u8 upd_flags[2] = {};
 	struct i2c_adapter *ddc;
 	struct drm_scdc *scdc;
+	int num_lanes;
 	int ret;
 	u8 val;
 
@@ -406,6 +509,19 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 
 	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
 
+	ret = drm_scdc_readb(ddc, SCDC_CONFIG_1, &val);
+	if (ret)
+		return ret;
+
+	state->rate = FIELD_GET(SCDC_FRL_RATE, val);
+	num_lanes = drm_scdc_num_frl_lanes(state->rate);
+	if (num_lanes < 0)
+		return num_lanes;
+	if (!num_lanes)
+		num_lanes = 3;
+
+	state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, val);
+
 	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
 	if (ret)
 		return ret;
@@ -416,8 +532,15 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
 			return ret;
 	}
 
+	if (upd_flags[0] & SCDC_FLT_UPDATE) {
+		ret = drm_scdc_read_status1_2_flags(connector, &state->stf);
+		if (ret)
+			return ret;
+	}
+
 	if (upd_flags[0] & SCDC_CED_UPDATE) {
-		ret = drm_scdc_read_error_counters(connector, state->error_count);
+		ret = drm_scdc_read_error_counters(connector, state->error_count,
+						   num_lanes);
 		if (ret)
 			return ret;
 	}
@@ -465,6 +588,8 @@ static int scdc_status_show(struct seq_file *m, void *data)
 
 	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
 	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
+	scdc_print_str(m, "FRL Rate", drm_scdc_frl_rate_str(st->rate));
+	scdc_print_dec(m, "FFE Levels", st->ffe_levels);
 
 	if (st->tmds_bclk_x40)
 		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
@@ -475,10 +600,17 @@ static int scdc_status_show(struct seq_file *m, void *data)
 	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
 	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
 	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
+	if (drm_scdc_num_frl_lanes(st->rate) == 4)
+		scdc_print_flag(m, "Lane 3 Locked", st->stf.ln3_locked);
+
+	scdc_print_flag(m, "Sink Ready For Link Training", st->stf.flt_ready);
+	scdc_print_flag(m, "Sink Failed To Decode DSC", st->stf.dsc_fail);
 
 	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
 	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
 	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
+	if (drm_scdc_num_frl_lanes(st->rate) == 4)
+		scdc_print_dec(m, "Lane 3 Errors", st->error_count[3]);
 
 	return 0;
 
diff --git a/include/drm/display/drm_scdc.h b/include/drm/display/drm_scdc.h
index 3d58f37e8ed8..9e365a95828d 100644
--- a/include/drm/display/drm_scdc.h
+++ b/include/drm/display/drm_scdc.h
@@ -29,6 +29,7 @@
 #define SCDC_SOURCE_VERSION 0x02
 
 #define SCDC_UPDATE_0 0x10
+#define  SCDC_FLT_UPDATE (1 << 5)
 #define  SCDC_READ_REQUEST_TEST (1 << 2)
 #define  SCDC_CED_UPDATE (1 << 1)
 #define  SCDC_STATUS_UPDATE (1 << 0)
@@ -46,14 +47,24 @@
 #define SCDC_CONFIG_0 0x30
 #define  SCDC_READ_REQUEST_ENABLE (1 << 0)
 
+#define SCDC_CONFIG_1 0x31
+#define  SCDC_FRL_RATE   0x0f
+#define  SCDC_FFE_LEVELS 0xf0
+
 #define SCDC_STATUS_FLAGS_0 0x40
+#define  SCDC_DSC_FAIL (1 << 7)
+#define  SCDC_FLT_READY (1 << 6)
+#define  SCDC_LN3_LOCK (1 << 4)
 #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_CH_LOCK_MASK (SCDC_LN3_LOCK | SCDC_CH2_LOCK | SCDC_CH1_LOCK | \
+			    SCDC_CH0_LOCK)
 #define  SCDC_CLOCK_DETECT (1 << 0)
 
 #define SCDC_STATUS_FLAGS_1 0x41
+#define  SCDC_LN_EVEN_TRAIN_PTRN 0x0f
+#define  SCDC_LN_ODD_TRAIN_PTRN 0xf0
 
 #define SCDC_ERR_DET_0_L 0x50
 #define SCDC_ERR_DET_0_H 0x51
@@ -65,6 +76,9 @@
 
 #define SCDC_ERR_DET_CHECKSUM 0x56
 
+#define SCDC_ERR_DET_3_L 0x57
+#define SCDC_ERR_DET_3_H 0x58
+
 #define SCDC_TEST_CONFIG_0 0xc0
 #define  SCDC_TEST_READ_REQUEST (1 << 7)
 #define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index 3b3a4e0e48ba..926acf282cd3 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -24,6 +24,7 @@
 #ifndef DRM_SCDC_HELPER_H
 #define DRM_SCDC_HELPER_H
 
+#include <linux/errno.h>
 #include <linux/types.h>
 
 #include <drm/display/drm_scdc.h>
@@ -38,8 +39,65 @@ struct drm_scdc_status_flags {
 	bool ch0_locked;
 	bool ch1_locked;
 	bool ch2_locked;
+	bool ln3_locked;
+	bool flt_ready;
+	bool dsc_fail;
+
+	/* Status Register 1 */
+	u8 ln0_training_pattern : 4;
+	u8 ln1_training_pattern : 4;
+
+	/* Status Register 2 */
+	u8 ln2_training_pattern : 4;
+	u8 ln3_training_pattern : 4;
+};
+
+enum drm_scdc_frl_rate {
+	SCDC_FRL_RATE_OFF = 0,
+	SCDC_FRL_RATE_3X3 = 1,
+	SCDC_FRL_RATE_6X3 = 2,
+	SCDC_FRL_RATE_6X4 = 3,
+	SCDC_FRL_RATE_8X4 = 4,
+	SCDC_FRL_RATE_10X4 = 5,
+	SCDC_FRL_RATE_12X4 = 6,
+	SCDC_FRL_RATE_RESV_7 = 7,
+	SCDC_FRL_RATE_RESV_8 = 8,
+	SCDC_FRL_RATE_RESV_9 = 9,
+	SCDC_FRL_RATE_RESV_10 = 10,
+	SCDC_FRL_RATE_RESV_11 = 11,
+	SCDC_FRL_RATE_RESV_12 = 12,
+	SCDC_FRL_RATE_RESV_13 = 13,
+	SCDC_FRL_RATE_RESV_14 = 14,
+	SCDC_FRL_RATE_RESV_15 = 15
 };
 
+/**
+ * drm_scdc_num_frl_lanes - get number of lanes for a given FRL rate
+ * @rate: one of &enum drm_scdc_frl_rate
+ *
+ * For a given @rate, return the number of lanes it uses.
+ *
+ * Returns: %-EINVAL if @rate is not a valid FRL rate, or the number of lanes
+ * for a given &enum drm_scdc_frl_rate on success (including %0 for "off")
+ */
+static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
+{
+	switch (rate) {
+	case SCDC_FRL_RATE_OFF:
+		return 0;
+	case SCDC_FRL_RATE_3X3:
+	case SCDC_FRL_RATE_6X3:
+		return 3;
+	case SCDC_FRL_RATE_6X4:
+	case SCDC_FRL_RATE_8X4:
+	case SCDC_FRL_RATE_10X4:
+	case SCDC_FRL_RATE_12X4:
+		return 4;
+	default:
+		return -EINVAL;
+	}
+}
+
 struct drm_scdc_state {
 	/** @stf: contents of the status flag registers */
 	struct drm_scdc_status_flags stf;
@@ -52,8 +110,12 @@ struct drm_scdc_state {
 	 * clock period, false if it's 1/10th of the clock period.
 	 */
 	bool tmds_bclk_x40;
-	/** @error_count: character error counts for each channel */
-	u16 error_count[3];
+	/** @rate: FRL rate set by the source */
+	enum drm_scdc_frl_rate rate : 4;
+	/** @ffe_levels: The FFE levels for @rate set by the source */
+	u8 ffe_levels : 4;
+	/** @error_count: character error counts for each channel/link */
+	u16 error_count[4];
 };
 
 int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
@@ -104,9 +166,12 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool se
 
 int drm_scdc_read_status0_flags(struct drm_connector *connector,
 				struct drm_scdc_status_flags *flags);
+int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
+				  struct drm_scdc_status_flags *flags);
 int drm_scdc_read_state(struct drm_connector *connector,
 			struct drm_scdc_state *state);
-int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
+int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
+				 unsigned int num_lanes);
 void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
 
 #endif

-- 
2.54.0

Re: [PATCH v4 0/4] Add SCDC information to connector debugfs

[Hans Verkuil]

Hi Nicolas,

On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> HDMI uses the DDC I2C bus for communicating various bits of link status
> out of band with the actual HDMI video signal. This information can be
> useful for debugging issues like questionable cables sabotaged by feline
> teeth, Enthusiast Grade cables made of cow fencing wire, and other such
> problems that ruin one's media viewing plans.
> 
> Consequently, this series exposes various bits of pertinent information
> from the SCDC protocol in an HDMI connector's debugfs. To continually
> poll the link status, userspace can poll the debugfs file.

Daniel pointed out this series to me when we met earlier this week at the
Embedded Recipes conference.

I very recently added SCDC parsing (up to HDMI 2.2) to the edid-decode utility
(git://linuxtv.org/v4l-utils.git).

Just as with the InfoFrames that are exposed in debugfs, it would be good to
ensure that the debugfs output of SCDC can be parsed by edid-decode.

Next week I'll review this series.

One option is to just start the debugfs output with the hexdump of the SCDC
data, and then either leave the parsing to edid-decode, or add the parsing
yourself, which edid-decode will just skip.

Regards,

	Hans

> 
> ---
> Changes in v4:
> - Don't use C struct bitfields for parsing status flags. Switch to
>   bitwise AND for boolean flags, and FIELD_GET for multi-bit values.
> - Drop the superfluous !! and parens
> - Drop the __pure attributes on static functions
> - Initialise stack local arrays with {}, not { 0 }.
> - I've kept the print macros and %-30s format. Reason being that I don't
>   want to repeat the format specifier and str_yes_no(foo) a bunch, and I
>   like the %-30s format because it means all values are aligned with the
>   value of the longest field, which is 30 chars long.
> - Link to v3: https://patch.msgid.link/20260526-scdc-link-health-v3-0-59e4a4aaead1@collabora.com
> 
> Changes in v3:
> - Add patch to change return type of drm_scdc_read/write.
> - Rework error counter reading to duplicate less code.
> - Also check lane 3 counter valid flag when reading its error counter.
> - Use memset to clear buf for error counters, rather than doing it in
>   the loop.
> - Make read_error_counters not accept 0 as num_lanes; fix it up in the
>   caller instead.
> - Link to v2: https://patch.msgid.link/20260520-scdc-link-health-v2-0-511af18cd64b@collabora.com
> 
> Changes in v2:
> - Add HDMI 2.1 SCDC status reporting
> - Link to v1: https://patch.msgid.link/20260415-scdc-link-health-v1-0-8e731e88eaf0@collabora.com
> 
> To: Jani Nikula <jani.nikula@linux.intel.com>
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> To: Maxime Ripard <mripard@kernel.org>
> To: Thomas Zimmermann <tzimmermann@suse.de>
> To: David Airlie <airlied@gmail.com>
> To: Simona Vetter <simona@ffwll.ch>
> To: Andrzej Hajda <andrzej.hajda@intel.com>
> To: Neil Armstrong <neil.armstrong@linaro.org>
> To: Robert Foss <rfoss@kernel.org>
> To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> To: Jonas Karlman <jonas@kwiboo.se>
> To: Jernej Skrabec <jernej.skrabec@gmail.com>
> To: Luca Ceresoli <luca.ceresoli@bootlin.com>
> To: Daniel Stone <daniel@fooishbar.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel@collabora.com
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> ---
> Nicolas Frattaroli (4):
>       drm/scdc-helper: Don't use ssize_t return type for scdc_read/write
>       drm/scdc-helper: Add scdc_status debugfs entry
>       drm/display: bridge_connector: init scdc debugfs for HDMI
>       drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields
> 
>  drivers/gpu/drm/display/drm_bridge_connector.c |   4 +
>  drivers/gpu/drm/display/drm_scdc_helper.c      | 377 ++++++++++++++++++++++++-
>  include/drm/display/drm_scdc.h                 |  16 +-
>  include/drm/display/drm_scdc_helper.h          | 105 ++++++-
>  4 files changed, 493 insertions(+), 9 deletions(-)
> ---
> base-commit: 971288d0e77f7a0d356287121ce549ab5dad570a
> change-id: 20260413-scdc-link-health-89326013d96c
> 
> Best regards,
> --  
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 

Re: [PATCH v4 1/4] drm/scdc-helper: Don't use ssize_t return type for scdc_read/write

[Hans Verkuil]

On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> drm_scdc_read and drm_scdc_write, both of which are only used within
> drm_scdc_helper (although exported), use a ssize_t as their return type.
> 
> This would make sense if they returned the number of bytes read/written
> on success, and negative errno otherwise. However, they return 0 on
> success.
> 
> Demote them to "int" as their return type, in order to avoid needlessly
> using 64 bits when less suffices.
> 
> No functional change.

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>

Regards,

	Hans

> 
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 8 ++++----
>  include/drm/display/drm_scdc_helper.h     | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index df878aad4a36..8403f2390ab6 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -67,8 +67,8 @@
>   * Returns:
>   * 0 on success, negative error code on failure.
>   */
> -ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> -		      size_t size)
> +int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		  size_t size)
>  {
>  	int ret;
>  	struct i2c_msg msgs[2] = {
> @@ -107,8 +107,8 @@ EXPORT_SYMBOL(drm_scdc_read);
>   * Returns:
>   * 0 on success, negative error code on failure.
>   */
> -ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> -		       const void *buffer, size_t size)
> +int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
> +		   size_t size)
>  {
>  	struct i2c_msg msg = {
>  		.addr = SCDC_I2C_SLAVE_ADDRESS,
> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> index 34600476a1b9..e9ccaeba56dd 100644
> --- a/include/drm/display/drm_scdc_helper.h
> +++ b/include/drm/display/drm_scdc_helper.h
> @@ -31,10 +31,10 @@
>  struct drm_connector;
>  struct i2c_adapter;
>  
> -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);
> +int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		  size_t size);
> +int drm_scdc_write(struct i2c_adapter *adapter, u8 offset, const void *buffer,
> +		   size_t size);
>  
>  /**
>   * drm_scdc_readb - read a single byte from SCDC
> 

Re: [PATCH v4 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Hans Verkuil]

Hi Nicolas,

As promised, here is my review:

On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> SCDC provides status information on the current display link. At the
> very least, it may be useful to expose this info through debugfs.
> 
> Add a debugfs entry for it under the connector, which displays a few
> more details parsed out of the SCDC registers. A new
> drm_scdc_debugfs_init function can be called by the connector
> implementation to initialise the debugfs file.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 237 ++++++++++++++++++++++++++++++
>  include/drm/display/drm_scdc_helper.h     |  32 ++++
>  2 files changed, 269 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 8403f2390ab6..7739fb5e77a1 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -24,11 +24,14 @@
>  #include <linux/export.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
> +#include <linux/overflow.h>
>  
>  #include <drm/display/drm_scdc_helper.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_print.h>
>  
>  /**
> @@ -55,6 +58,11 @@
>  
>  #define SCDC_I2C_SLAVE_ADDRESS 0x54
>  
> +struct scdc_debugfs_priv {
> +	struct drm_connector *connector;
> +	struct drm_scdc_state state;
> +};
> +
>  /**
>   * drm_scdc_read - read a block of data from SCDC
>   * @adapter: I2C controller
> @@ -276,3 +284,232 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
>  	return true;
>  }
>  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> +
> +/**
> + * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
> + *
> + * Reads the SCDC Status Flags 0 register, and outputs its contents to the
> + * destination @flags. Contents of @flags are only valid if function returns 0.
> + *
> + * Returns: %0 on success, negative errno on error.
> + */
> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> +				struct drm_scdc_status_flags *flags)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);

It doesn't hurt to set SCDC_STATUS_UPDATE to 1, but neither is there a need for it.
It just causes unnecessary DDC traffic IMHO.

> +	if (ret)
> +		return ret;
> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &val);
> +	if (ret)
> +		return ret;
> +
> +	flags->clock_detected = val & SCDC_CLOCK_DETECT;
> +	flags->ch0_locked = val & SCDC_CH0_LOCK;
> +	flags->ch1_locked = val & SCDC_CH1_LOCK;
> +	flags->ch2_locked = val & SCDC_CH2_LOCK;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_scdc_read_status0_flags);
> +
> +/**
> + * drm_scdc_read_error_counters - Read and clear SCDC error counters
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @counter: Caller's u16 array with 3 elements to write the counter values into
> + *
> + * Read the SCDC channel error counters. If the count of channel *n* is valid,
> + * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
> + * in one read chunk, then clears every counter, as is mandated.
> + *
> + * Returns: %0 on success, negative errno on error.
> + */
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
> +{
> +	u8 buf[7] = {};
> +	int ret;
> +	u8 sum = 0;
> +	int i;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);

Same here: there is no need to set SCDC_CED_UPDATE.

> +	if (ret)
> +		return ret;
> +
> +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Verify the "checksum", i.e. sum up everything including the checksum
> +	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +		sum = wrapping_add(u8, sum, buf[i]);
> +
> +	if (sum)
> +		return -EPROTO;
> +
> +	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> +		if (buf[i + 1] & SCDC_CHANNEL_VALID)
> +			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
> +		else
> +			counter[i / 2] = 0;
> +
> +		buf[i] = 0;
> +		buf[i + 1] = 0;
> +	}
> +	buf[ARRAY_SIZE(buf) - 1] = 0;
> +
> +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));

Huh? Reading the CED registers will automatically zero them as per the HDMI spec
(section 10.4.1.8, first paragraph). So there is no need to write to these registers.
Besides, they are read-only (table 10-14).

> +}
> +EXPORT_SYMBOL(drm_scdc_read_error_counters);
> +
> +/**
> + * drm_scdc_read_state - Update state from SCDC
> + * @connector: pointer to a &struct drm_connector on which to operate on
> + * @state: pointer to a &struct drm_scdc_state to fill
> + *
> + * Reads update flags from SCDC, and updates the parts of @state that SCDC
> + * claims have changed, as well as populating those where such a distinction
> + * can't be made.
> + *
> + * Returns: %0 on success, negative errno on failure.
> + */
> +int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
> +{
> +	u8 upd_flags[2] = {};
> +	struct i2c_adapter *ddc;
> +	struct drm_scdc *scdc;
> +	int ret;
> +	u8 val;
> +
> +	if (!state || !connector)
> +		return -ENODEV;
> +
> +	scdc = &connector->display_info.hdmi.scdc;
> +	ddc = connector->ddc;
> +
> +	if (!scdc->supported)
> +		return -EOPNOTSUPP;
> +
> +	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
> +	if (ret)
> +		return ret;
> +
> +	state->scrambling_enabled = val & SCDC_SCRAMBLING_ENABLE;
> +	state->tmds_bclk_x40 = val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> +
> +	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
> +
> +	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
> +	if (ret)
> +		return ret;
> +
> +	if (upd_flags[0] & SCDC_STATUS_UPDATE) {

Ah, so here you use SCDC_STATUS_UPDATE/SCDC_CED_UPDATE.

I do not think this makes sense: for debugfs you just want to see the current
status and not 'what has changed since last time', which is what this basically
does.

> +		ret = drm_scdc_read_status0_flags(connector, &state->stf);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (upd_flags[0] & SCDC_CED_UPDATE) {

And if the counters change only a little bit, then CED_UPDATE may not be set at all,
so you don't see such small changes.

> +		ret = drm_scdc_read_error_counters(connector, state->error_count);
> +		if (ret)
> +			return ret;

I would just always read the status flags and CED counters and report them. Especially
for debugfs usage.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_scdc_read_state);
> +
> +#define scdc_print_str(_f, key, s) \
> +	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
> +#define scdc_print_flag(_f, key, val) \
> +	(scdc_print_str((_f), (key), str_yes_no((val))))
> +#define scdc_print_dec(_f, key, val) \
> +	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
> +
> +static int scdc_status_show(struct seq_file *m, void *data)
> +{
> +	struct scdc_debugfs_priv *priv = m->private;
> +	struct drm_scdc_state *st = &priv->state;
> +	struct drm_connector *connector = priv->connector;
> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> +	int ret;
> +
> +	drm_connector_get(connector);
> +
> +	if (connector->status != connector_status_connected) {
> +		ret = -ENODEV;
> +		goto err_conn_put;
> +	}
> +
> +	scdc_print_flag(m, "SCDC Supported", scdc->supported);
> +	if (!scdc->supported) {
> +		ret = 0;
> +		goto err_conn_put;
> +	}
> +
> +	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
> +	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
> +	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
> +
> +	ret = drm_scdc_read_state(connector, st);
> +	drm_connector_put(connector);
> +	if (ret)
> +		return ret;
> +
> +	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
> +	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> +
> +	if (st->tmds_bclk_x40)
> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
> +	else
> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
> +
> +	scdc_print_flag(m, "Clock Detected", st->stf.clock_detected);
> +	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
> +	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
> +	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
> +
> +	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
> +	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
> +	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);

So, does parsing SCDC really belong in the kernel? Wouldn't it be easier
to just give a hexdump and let edid-decode parse it?

We do the same for EDIDs and Infoframes.

The edid-decode output (tested against my 4k TV) looks like this:

edid-decode SCDC (hex):

00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00
03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
03 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 80 00 80 00 80 80 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

----------------

Sink Version: 1 Source Version: 1
Sink Supported Features: 0x00
Source Supported Features: 0x00
Update Flags: 0x03 0x00
        Status_Update
        CED_Update
TMDS Configuration: 0x03
        Scrambling_Enable
        TMDS_Bit_Clock_Ratio: 1/40
TMDS Scrambler Status: 0x01
        TMDS_Scrambling_Status
Sink Configuration: 0x00 0x00
        FRL_Rate: Disable FRL
        FFE_Levels: 0
Source Test Configuration: 0x00
Status Flags: 0x0f 0x00 0x00
        Clock_Detected
        Ch0_Ln0_Locked
        Ch1_Ln1_Locked
        Ch2_Ln2_Locked
Character Error Detection:
        Channel 0 Error Count: 0
        Channel 1 Error Count: 0
        Channel 2 Error Count: 0
Manufacturer Specific: none

The debugfs scdc_status_show() function could just dump the 256 byte SCDC data as a single
binary blob or output it as a hex dump, and leave the parsing to edid-decode, just as was
done for InfoFrames in debugfs.

This would simplify the kernel code quite a bit, and edid-decode is much easier to adapt
to new HDMI versions. So parsing of the SCDC data from the display is not dependent on
the kernel version.

At minimum I would suggest that you dump the hex values before your parsed output.

Regards,

	Hans


> +
> +	return 0;
> +
> +err_conn_put:
> +	drm_connector_put(connector);
> +
> +	return ret;
> +}
> +DEFINE_SHOW_ATTRIBUTE(scdc_status);
> +
> +/**
> + * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
> + * @connector: pointer to &struct drm_connector to operate on
> + * @root: debugfs &struct dentry for the debugfs root of @connector
> + *
> + * Creates SCDC-related debugfs files for @connector. Must be called after
> + * @root is already created.
> + */
> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
> +{
> +	struct scdc_debugfs_priv *priv;
> +
> +	if (!root || !connector)
> +		return;
> +
> +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return;
> +
> +	priv->connector = connector;
> +
> +	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
> +}
> +EXPORT_SYMBOL(drm_scdc_debugfs_init);
> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> index e9ccaeba56dd..3b3a4e0e48ba 100644
> --- a/include/drm/display/drm_scdc_helper.h
> +++ b/include/drm/display/drm_scdc_helper.h
> @@ -30,6 +30,31 @@
>  
>  struct drm_connector;
>  struct i2c_adapter;
> +struct dentry;
> +
> +struct drm_scdc_status_flags {
> +	/* Status Register 0 */
> +	bool clock_detected;
> +	bool ch0_locked;
> +	bool ch1_locked;
> +	bool ch2_locked;
> +};
> +
> +struct drm_scdc_state {
> +	/** @stf: contents of the status flag registers */
> +	struct drm_scdc_status_flags stf;
> +	/** @scramling_enabled: true if TMDS scrambling is on */
> +	bool scrambling_enabled;
> +	/** @scrambling_detected: true if the sink actually detected scrambling */
> +	bool scrambling_detected;
> +	/**
> +	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
> +	 * clock period, false if it's 1/10th of the clock period.
> +	 */
> +	bool tmds_bclk_x40;
> +	/** @error_count: character error counts for each channel */
> +	u16 error_count[3];
> +};
>  
>  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
>  		  size_t size);
> @@ -77,4 +102,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
>  bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
>  bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
>  
> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> +				struct drm_scdc_status_flags *flags);
> +int drm_scdc_read_state(struct drm_connector *connector,
> +			struct drm_scdc_state *state);
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
> +
>  #endif
> 

Re: [PATCH v4 3/4] drm/display: bridge_connector: init scdc debugfs for HDMI

[Hans Verkuil]

On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> On drm_bridge_connectors that contain an HDMI bridge, initialise the
> SCDC debugfs entry under the connector's debugfs root.
> 
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>

Regards,

	Hans

> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 649969fca141..8c2c890bd95d 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -25,6 +25,7 @@
>  #include <drm/display/drm_hdmi_cec_helper.h>
>  #include <drm/display/drm_hdmi_helper.h>
>  #include <drm/display/drm_hdmi_state_helper.h>
> +#include <drm/display/drm_scdc_helper.h>
>  
>  /**
>   * DOC: overview
> @@ -263,6 +264,9 @@ static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
>  		if (bridge->funcs->debugfs_init)
>  			bridge->funcs->debugfs_init(bridge, root);
>  	}
> +
> +	if (bridge_connector->bridge_hdmi)
> +		drm_scdc_debugfs_init(connector, root);
>  }
>  
>  static void drm_bridge_connector_reset(struct drm_connector *connector)
> 

Re: [PATCH v4 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Hans Verkuil]

On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> HDMI 2.1 redefines previously reserved fields in SCDC for various new
> uses. No version check needs to be performed, as an HDMI 2.0 sink's
> reserved SCDC fields are well-defined to be 0, and any zero-ness of
> these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
> HDMI 2.1.
> 
> Implement reading and outputting these fields over debugfs.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 164 +++++++++++++++++++++++++++---
>  include/drm/display/drm_scdc.h            |  16 ++-
>  include/drm/display/drm_scdc_helper.h     |  71 ++++++++++++-
>  3 files changed, 231 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 7739fb5e77a1..6e2ad335296e 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/export.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> @@ -63,6 +64,38 @@ struct scdc_debugfs_priv {
>  	struct drm_scdc_state state;
>  };
>  
> +static const char *drm_scdc_frl_rate_str(enum drm_scdc_frl_rate rate)
> +{
> +	switch (rate) {
> +	case SCDC_FRL_RATE_OFF:
> +		return "Off";
> +	case SCDC_FRL_RATE_3X3:
> +		return "3 Gbit/s x 3 lanes";
> +	case SCDC_FRL_RATE_6X3:
> +		return "6 Gbit/s x 3 lanes";
> +	case SCDC_FRL_RATE_6X4:
> +		return "6 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_8X4:
> +		return "8 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_10X4:
> +		return "10 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_12X4:
> +		return "12 Gbit/s x 4 lanes";
> +	case SCDC_FRL_RATE_RESV_7:
> +	case SCDC_FRL_RATE_RESV_8:
> +	case SCDC_FRL_RATE_RESV_9:
> +	case SCDC_FRL_RATE_RESV_10:
> +	case SCDC_FRL_RATE_RESV_11:
> +	case SCDC_FRL_RATE_RESV_12:
> +	case SCDC_FRL_RATE_RESV_13:
> +	case SCDC_FRL_RATE_RESV_14:
> +	case SCDC_FRL_RATE_RESV_15:
> +		return "(Reserved)";
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /**
>   * drm_scdc_read - read a block of data from SCDC
>   * @adapter: I2C controller
> @@ -313,15 +346,72 @@ int drm_scdc_read_status0_flags(struct drm_connector *connector,
>  	flags->ch0_locked = val & SCDC_CH0_LOCK;
>  	flags->ch1_locked = val & SCDC_CH1_LOCK;
>  	flags->ch2_locked = val & SCDC_CH2_LOCK;
> +	flags->ln3_locked = val & SCDC_LN3_LOCK;
> +	flags->flt_ready = val & SCDC_FLT_READY;
> +	flags->dsc_fail = val & SCDC_DSC_FAIL;
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_scdc_read_status0_flags);
>  
> +/**
> + * drm_scdc_read_status1_2_flags - Read SCDC "Status Flags" 1 and 2 Registers
> + * @connector: pointer to &struct drm_connector to issue the scdc request on
> + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
> + *
> + * Reads the SCDC Status Flags 1 and 2 registers, and outputs their contents to
> + * the destination @flags. Contents of @flags are only valid if function returns
> + * 0.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
> +				  struct drm_scdc_status_flags *flags)
> +{
> +	u8 val[2] = {};
> +	int ret;
> +
> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_FLT_UPDATE);

As mentioned in my earlier review, I don't think this is really needed.

And of course the same comments as in 2/4 apply: I think the actual parsing is
better done in userspace.

But otherwise this looks good, so:

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>

Regards,

	Hans

> +	if (ret)
> +		return ret;
> +
> +	ret = drm_scdc_read(connector->ddc, SCDC_STATUS_FLAGS_1, val, 2);
> +	if (ret)
> +		return ret;
> +
> +	flags->ln0_training_pattern = FIELD_GET(SCDC_LN_EVEN_TRAIN_PTRN, val[0]);
> +	flags->ln1_training_pattern = FIELD_GET(SCDC_LN_ODD_TRAIN_PTRN, val[0]);
> +
> +	flags->ln2_training_pattern = FIELD_GET(SCDC_LN_EVEN_TRAIN_PTRN, val[1]);
> +	flags->ln3_training_pattern = FIELD_GET(SCDC_LN_ODD_TRAIN_PTRN, val[1]);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_scdc_read_status1_2_flags);
> +
> +#define ERR_DET_OFF(x) ((x) - SCDC_ERR_DET_0_L)
> +
> +static int scdc_err_cnt_buf_idx(unsigned int lane)
> +{
> +	switch (lane) {
> +	case 0:
> +		return ERR_DET_OFF(SCDC_ERR_DET_0_L);
> +	case 1:
> +		return ERR_DET_OFF(SCDC_ERR_DET_1_L);
> +	case 2:
> +		return ERR_DET_OFF(SCDC_ERR_DET_2_L);
> +	case 3:
> +		return ERR_DET_OFF(SCDC_ERR_DET_3_L);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /**
>   * drm_scdc_read_error_counters - Read and clear SCDC error counters
>   * @connector: pointer to &struct drm_connector to issue the scdc request on
> - * @counter: Caller's u16 array with 3 elements to write the counter values into
> + * @counter: Caller's u16 array with 4 elements to write the counter values into
> + * @num_lanes: number of active lanes, either 3 or 4
>   *
>   * Read the SCDC channel error counters. If the count of channel *n* is valid,
>   * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
> @@ -329,18 +419,31 @@ EXPORT_SYMBOL(drm_scdc_read_status0_flags);
>   *
>   * Returns: %0 on success, negative errno on error.
>   */
> -int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
> +				 unsigned int num_lanes)
>  {
> -	u8 buf[7] = {};
> +	unsigned int buf_sz;
> +	u8 buf[9] = {};
>  	int ret;
>  	u8 sum = 0;
> -	int i;
> +	int i, idx;
> +
> +	switch (num_lanes) {
> +	case 3:
> +		buf_sz = 7;
> +		break;
> +	case 4:
> +		buf_sz = 9;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
>  	if (ret)
>  		return ret;
>  
> @@ -348,24 +451,23 @@ int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]
>  	 * Verify the "checksum", i.e. sum up everything including the checksum
>  	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
>  	 */
> -	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +	for (i = 0; i < buf_sz; i++)
>  		sum = wrapping_add(u8, sum, buf[i]);
>  
>  	if (sum)
>  		return -EPROTO;
>  
> -	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> -		if (buf[i + 1] & SCDC_CHANNEL_VALID)
> -			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
> +	for (i = 0; i < num_lanes; i++) {
> +		idx = scdc_err_cnt_buf_idx(i);
> +		if (buf[idx + 1] & SCDC_CHANNEL_VALID)
> +			counter[i] = buf[idx] | (buf[idx + 1] & ~SCDC_CHANNEL_VALID) << 8;
>  		else
> -			counter[i / 2] = 0;
> -
> -		buf[i] = 0;
> -		buf[i + 1] = 0;
> +			counter[i] = 0;
>  	}
> -	buf[ARRAY_SIZE(buf) - 1] = 0;
>  
> -	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> +	memset(buf, 0, buf_sz);
> +
> +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
>  }
>  EXPORT_SYMBOL(drm_scdc_read_error_counters);
>  
> @@ -385,6 +487,7 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  	u8 upd_flags[2] = {};
>  	struct i2c_adapter *ddc;
>  	struct drm_scdc *scdc;
> +	int num_lanes;
>  	int ret;
>  	u8 val;
>  
> @@ -406,6 +509,19 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  
>  	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
>  
> +	ret = drm_scdc_readb(ddc, SCDC_CONFIG_1, &val);
> +	if (ret)
> +		return ret;
> +
> +	state->rate = FIELD_GET(SCDC_FRL_RATE, val);
> +	num_lanes = drm_scdc_num_frl_lanes(state->rate);
> +	if (num_lanes < 0)
> +		return num_lanes;
> +	if (!num_lanes)
> +		num_lanes = 3;
> +
> +	state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, val);
> +
>  	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
>  	if (ret)
>  		return ret;
> @@ -416,8 +532,15 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  			return ret;
>  	}
>  
> +	if (upd_flags[0] & SCDC_FLT_UPDATE) {
> +		ret = drm_scdc_read_status1_2_flags(connector, &state->stf);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (upd_flags[0] & SCDC_CED_UPDATE) {
> -		ret = drm_scdc_read_error_counters(connector, state->error_count);
> +		ret = drm_scdc_read_error_counters(connector, state->error_count,
> +						   num_lanes);
>  		if (ret)
>  			return ret;
>  	}
> @@ -465,6 +588,8 @@ static int scdc_status_show(struct seq_file *m, void *data)
>  
>  	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
>  	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> +	scdc_print_str(m, "FRL Rate", drm_scdc_frl_rate_str(st->rate));
> +	scdc_print_dec(m, "FFE Levels", st->ffe_levels);
>  
>  	if (st->tmds_bclk_x40)
>  		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
> @@ -475,10 +600,17 @@ static int scdc_status_show(struct seq_file *m, void *data)
>  	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
>  	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
>  	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4)
> +		scdc_print_flag(m, "Lane 3 Locked", st->stf.ln3_locked);
> +
> +	scdc_print_flag(m, "Sink Ready For Link Training", st->stf.flt_ready);
> +	scdc_print_flag(m, "Sink Failed To Decode DSC", st->stf.dsc_fail);
>  
>  	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
>  	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
>  	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4)
> +		scdc_print_dec(m, "Lane 3 Errors", st->error_count[3]);
>  
>  	return 0;
>  
> diff --git a/include/drm/display/drm_scdc.h b/include/drm/display/drm_scdc.h
> index 3d58f37e8ed8..9e365a95828d 100644
> --- a/include/drm/display/drm_scdc.h
> +++ b/include/drm/display/drm_scdc.h
> @@ -29,6 +29,7 @@
>  #define SCDC_SOURCE_VERSION 0x02
>  
>  #define SCDC_UPDATE_0 0x10
> +#define  SCDC_FLT_UPDATE (1 << 5)
>  #define  SCDC_READ_REQUEST_TEST (1 << 2)
>  #define  SCDC_CED_UPDATE (1 << 1)
>  #define  SCDC_STATUS_UPDATE (1 << 0)
> @@ -46,14 +47,24 @@
>  #define SCDC_CONFIG_0 0x30
>  #define  SCDC_READ_REQUEST_ENABLE (1 << 0)
>  
> +#define SCDC_CONFIG_1 0x31
> +#define  SCDC_FRL_RATE   0x0f
> +#define  SCDC_FFE_LEVELS 0xf0
> +
>  #define SCDC_STATUS_FLAGS_0 0x40
> +#define  SCDC_DSC_FAIL (1 << 7)
> +#define  SCDC_FLT_READY (1 << 6)
> +#define  SCDC_LN3_LOCK (1 << 4)
>  #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_CH_LOCK_MASK (SCDC_LN3_LOCK | SCDC_CH2_LOCK | SCDC_CH1_LOCK | \
> +			    SCDC_CH0_LOCK)
>  #define  SCDC_CLOCK_DETECT (1 << 0)
>  
>  #define SCDC_STATUS_FLAGS_1 0x41
> +#define  SCDC_LN_EVEN_TRAIN_PTRN 0x0f
> +#define  SCDC_LN_ODD_TRAIN_PTRN 0xf0
>  
>  #define SCDC_ERR_DET_0_L 0x50
>  #define SCDC_ERR_DET_0_H 0x51
> @@ -65,6 +76,9 @@
>  
>  #define SCDC_ERR_DET_CHECKSUM 0x56
>  
> +#define SCDC_ERR_DET_3_L 0x57
> +#define SCDC_ERR_DET_3_H 0x58
> +
>  #define SCDC_TEST_CONFIG_0 0xc0
>  #define  SCDC_TEST_READ_REQUEST (1 << 7)
>  #define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> index 3b3a4e0e48ba..926acf282cd3 100644
> --- a/include/drm/display/drm_scdc_helper.h
> +++ b/include/drm/display/drm_scdc_helper.h
> @@ -24,6 +24,7 @@
>  #ifndef DRM_SCDC_HELPER_H
>  #define DRM_SCDC_HELPER_H
>  
> +#include <linux/errno.h>
>  #include <linux/types.h>
>  
>  #include <drm/display/drm_scdc.h>
> @@ -38,8 +39,65 @@ struct drm_scdc_status_flags {
>  	bool ch0_locked;
>  	bool ch1_locked;
>  	bool ch2_locked;
> +	bool ln3_locked;
> +	bool flt_ready;
> +	bool dsc_fail;
> +
> +	/* Status Register 1 */
> +	u8 ln0_training_pattern : 4;
> +	u8 ln1_training_pattern : 4;
> +
> +	/* Status Register 2 */
> +	u8 ln2_training_pattern : 4;
> +	u8 ln3_training_pattern : 4;
> +};
> +
> +enum drm_scdc_frl_rate {
> +	SCDC_FRL_RATE_OFF = 0,
> +	SCDC_FRL_RATE_3X3 = 1,
> +	SCDC_FRL_RATE_6X3 = 2,
> +	SCDC_FRL_RATE_6X4 = 3,
> +	SCDC_FRL_RATE_8X4 = 4,
> +	SCDC_FRL_RATE_10X4 = 5,
> +	SCDC_FRL_RATE_12X4 = 6,
> +	SCDC_FRL_RATE_RESV_7 = 7,
> +	SCDC_FRL_RATE_RESV_8 = 8,
> +	SCDC_FRL_RATE_RESV_9 = 9,
> +	SCDC_FRL_RATE_RESV_10 = 10,
> +	SCDC_FRL_RATE_RESV_11 = 11,
> +	SCDC_FRL_RATE_RESV_12 = 12,
> +	SCDC_FRL_RATE_RESV_13 = 13,
> +	SCDC_FRL_RATE_RESV_14 = 14,
> +	SCDC_FRL_RATE_RESV_15 = 15
>  };
>  
> +/**
> + * drm_scdc_num_frl_lanes - get number of lanes for a given FRL rate
> + * @rate: one of &enum drm_scdc_frl_rate
> + *
> + * For a given @rate, return the number of lanes it uses.
> + *
> + * Returns: %-EINVAL if @rate is not a valid FRL rate, or the number of lanes
> + * for a given &enum drm_scdc_frl_rate on success (including %0 for "off")
> + */
> +static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
> +{
> +	switch (rate) {
> +	case SCDC_FRL_RATE_OFF:
> +		return 0;
> +	case SCDC_FRL_RATE_3X3:
> +	case SCDC_FRL_RATE_6X3:
> +		return 3;
> +	case SCDC_FRL_RATE_6X4:
> +	case SCDC_FRL_RATE_8X4:
> +	case SCDC_FRL_RATE_10X4:
> +	case SCDC_FRL_RATE_12X4:
> +		return 4;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  struct drm_scdc_state {
>  	/** @stf: contents of the status flag registers */
>  	struct drm_scdc_status_flags stf;
> @@ -52,8 +110,12 @@ struct drm_scdc_state {
>  	 * clock period, false if it's 1/10th of the clock period.
>  	 */
>  	bool tmds_bclk_x40;
> -	/** @error_count: character error counts for each channel */
> -	u16 error_count[3];
> +	/** @rate: FRL rate set by the source */
> +	enum drm_scdc_frl_rate rate : 4;
> +	/** @ffe_levels: The FFE levels for @rate set by the source */
> +	u8 ffe_levels : 4;
> +	/** @error_count: character error counts for each channel/link */
> +	u16 error_count[4];
>  };
>  
>  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> @@ -104,9 +166,12 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool se
>  
>  int drm_scdc_read_status0_flags(struct drm_connector *connector,
>  				struct drm_scdc_status_flags *flags);
> +int drm_scdc_read_status1_2_flags(struct drm_connector *connector,
> +				  struct drm_scdc_status_flags *flags);
>  int drm_scdc_read_state(struct drm_connector *connector,
>  			struct drm_scdc_state *state);
> -int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[4],
> +				 unsigned int num_lanes);
>  void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>  
>  #endif
> 

Re: [PATCH v4 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Nicolas Frattaroli]

On Tuesday, 2 June 2026 08:40:34 Central European Summer Time Hans Verkuil wrote:
> Hi Nicolas,
> 
> As promised, here is my review:
> 
> On 27/05/2026 16:03, Nicolas Frattaroli wrote:
> > SCDC provides status information on the current display link. At the
> > very least, it may be useful to expose this info through debugfs.
> > 
> > Add a debugfs entry for it under the connector, which displays a few
> > more details parsed out of the SCDC registers. A new
> > drm_scdc_debugfs_init function can be called by the connector
> > implementation to initialise the debugfs file.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/display/drm_scdc_helper.c | 237 ++++++++++++++++++++++++++++++
> >  include/drm/display/drm_scdc_helper.h     |  32 ++++
> >  2 files changed, 269 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> > index 8403f2390ab6..7739fb5e77a1 100644
> > --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> > @@ -24,11 +24,14 @@
> >  #include <linux/export.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/delay.h>
> > +#include <linux/overflow.h>
> >  
> >  #include <drm/display/drm_scdc_helper.h>
> >  #include <drm/drm_connector.h>
> >  #include <drm/drm_device.h>
> > +#include <drm/drm_managed.h>
> >  #include <drm/drm_print.h>
> >  
> >  /**
> > @@ -55,6 +58,11 @@
> >  
> >  #define SCDC_I2C_SLAVE_ADDRESS 0x54
> >  
> > +struct scdc_debugfs_priv {
> > +	struct drm_connector *connector;
> > +	struct drm_scdc_state state;
> > +};
> > +
> >  /**
> >   * drm_scdc_read - read a block of data from SCDC
> >   * @adapter: I2C controller
> > @@ -276,3 +284,232 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
> >  	return true;
> >  }
> >  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> > +
> > +/**
> > + * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
> > + * @connector: pointer to &struct drm_connector to issue the scdc request on
> > + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
> > + *
> > + * Reads the SCDC Status Flags 0 register, and outputs its contents to the
> > + * destination @flags. Contents of @flags are only valid if function returns 0.
> > + *
> > + * Returns: %0 on success, negative errno on error.
> > + */
> > +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> > +				struct drm_scdc_status_flags *flags)
> > +{
> > +	int ret;
> > +	u8 val;
> > +
> > +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
> 
> It doesn't hurt to set SCDC_STATUS_UPDATE to 1, but neither is there a need for it.
> It just causes unnecessary DDC traffic IMHO.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	flags->clock_detected = val & SCDC_CLOCK_DETECT;
> > +	flags->ch0_locked = val & SCDC_CH0_LOCK;
> > +	flags->ch1_locked = val & SCDC_CH1_LOCK;
> > +	flags->ch2_locked = val & SCDC_CH2_LOCK;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_scdc_read_status0_flags);
> > +
> > +/**
> > + * drm_scdc_read_error_counters - Read and clear SCDC error counters
> > + * @connector: pointer to &struct drm_connector to issue the scdc request on
> > + * @counter: Caller's u16 array with 3 elements to write the counter values into
> > + *
> > + * Read the SCDC channel error counters. If the count of channel *n* is valid,
> > + * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
> > + * in one read chunk, then clears every counter, as is mandated.
> > + *
> > + * Returns: %0 on success, negative errno on error.
> > + */
> > +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
> > +{
> > +	u8 buf[7] = {};
> > +	int ret;
> > +	u8 sum = 0;
> > +	int i;
> > +
> > +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
> 
> Same here: there is no need to set SCDC_CED_UPDATE.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Verify the "checksum", i.e. sum up everything including the checksum
> > +	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
> > +	 */
> > +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> > +		sum = wrapping_add(u8, sum, buf[i]);
> > +
> > +	if (sum)
> > +		return -EPROTO;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> > +		if (buf[i + 1] & SCDC_CHANNEL_VALID)
> > +			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
> > +		else
> > +			counter[i / 2] = 0;
> > +
> > +		buf[i] = 0;
> > +		buf[i + 1] = 0;
> > +	}
> > +	buf[ARRAY_SIZE(buf) - 1] = 0;
> > +
> > +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
> 
> Huh? Reading the CED registers will automatically zero them as per the HDMI spec
> (section 10.4.1.8, first paragraph). So there is no need to write to these registers.
> Besides, they are read-only (table 10-14).

Yeah that's a mistake on my part. I thought the source had to clear
them to signal that it wants more data, but now that I think about it,
the sink obviously already knows when the source has read them because
it sends an i2c read command.

> > +}
> > +EXPORT_SYMBOL(drm_scdc_read_error_counters);
> > +
> > +/**
> > + * drm_scdc_read_state - Update state from SCDC
> > + * @connector: pointer to a &struct drm_connector on which to operate on
> > + * @state: pointer to a &struct drm_scdc_state to fill
> > + *
> > + * Reads update flags from SCDC, and updates the parts of @state that SCDC
> > + * claims have changed, as well as populating those where such a distinction
> > + * can't be made.
> > + *
> > + * Returns: %0 on success, negative errno on failure.
> > + */
> > +int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
> > +{
> > +	u8 upd_flags[2] = {};
> > +	struct i2c_adapter *ddc;
> > +	struct drm_scdc *scdc;
> > +	int ret;
> > +	u8 val;
> > +
> > +	if (!state || !connector)
> > +		return -ENODEV;
> > +
> > +	scdc = &connector->display_info.hdmi.scdc;
> > +	ddc = connector->ddc;
> > +
> > +	if (!scdc->supported)
> > +		return -EOPNOTSUPP;
> > +
> > +	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	state->scrambling_enabled = val & SCDC_SCRAMBLING_ENABLE;
> > +	state->tmds_bclk_x40 = val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
> > +
> > +	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
> > +
> > +	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (upd_flags[0] & SCDC_STATUS_UPDATE) {
> 
> Ah, so here you use SCDC_STATUS_UPDATE/SCDC_CED_UPDATE.
> 
> I do not think this makes sense: for debugfs you just want to see the current
> status and not 'what has changed since last time', which is what this basically
> does.

My thought was that people will do a

  while sleep 0.5; do cat scdc_status ; echo "------"; done

and the update flags will make sure there isn't unnecessary traffic.

> > +		ret = drm_scdc_read_status0_flags(connector, &state->stf);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (upd_flags[0] & SCDC_CED_UPDATE) {
> 
> And if the counters change only a little bit, then CED_UPDATE may not be set at all,
> so you don't see such small changes.
> 
> > +		ret = drm_scdc_read_error_counters(connector, state->error_count);
> > +		if (ret)
> > +			return ret;
> 
> I would just always read the status flags and CED counters and report them. Especially
> for debugfs usage.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_scdc_read_state);
> > +
> > +#define scdc_print_str(_f, key, s) \
> > +	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
> > +#define scdc_print_flag(_f, key, val) \
> > +	(scdc_print_str((_f), (key), str_yes_no((val))))
> > +#define scdc_print_dec(_f, key, val) \
> > +	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
> > +
> > +static int scdc_status_show(struct seq_file *m, void *data)
> > +{
> > +	struct scdc_debugfs_priv *priv = m->private;
> > +	struct drm_scdc_state *st = &priv->state;
> > +	struct drm_connector *connector = priv->connector;
> > +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > +	int ret;
> > +
> > +	drm_connector_get(connector);
> > +
> > +	if (connector->status != connector_status_connected) {
> > +		ret = -ENODEV;
> > +		goto err_conn_put;
> > +	}
> > +
> > +	scdc_print_flag(m, "SCDC Supported", scdc->supported);
> > +	if (!scdc->supported) {
> > +		ret = 0;
> > +		goto err_conn_put;
> > +	}
> > +
> > +	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
> > +	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
> > +	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
> > +
> > +	ret = drm_scdc_read_state(connector, st);
> > +	drm_connector_put(connector);
> > +	if (ret)
> > +		return ret;
> > +
> > +	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
> > +	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> > +
> > +	if (st->tmds_bclk_x40)
> > +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
> > +	else
> > +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
> > +
> > +	scdc_print_flag(m, "Clock Detected", st->stf.clock_detected);
> > +	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
> > +	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
> > +	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
> > +
> > +	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
> > +	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
> > +	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
> 
> So, does parsing SCDC really belong in the kernel? Wouldn't it be easier
> to just give a hexdump and let edid-decode parse it?

Assuming I follow your advice and no longer use the status update
flags, does this provide any value over just having edid-decode access
the i2c itself? This isn't a rhetorical question; I don't know if the
i2c things as exposed by the kernel have drawbacks for multiple readers
or even drawbacks being exposed at all.

> 
> We do the same for EDIDs and Infoframes.
> 
> The edid-decode output (tested against my 4k TV) looks like this:
> 
> edid-decode SCDC (hex):
> 
> 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00
> 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 03 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 80 00 80 00 80 80 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> ----------------
> 
> Sink Version: 1 Source Version: 1
> Sink Supported Features: 0x00
> Source Supported Features: 0x00
> Update Flags: 0x03 0x00
>         Status_Update
>         CED_Update
> TMDS Configuration: 0x03
>         Scrambling_Enable
>         TMDS_Bit_Clock_Ratio: 1/40
> TMDS Scrambler Status: 0x01
>         TMDS_Scrambling_Status
> Sink Configuration: 0x00 0x00
>         FRL_Rate: Disable FRL
>         FFE_Levels: 0
> Source Test Configuration: 0x00
> Status Flags: 0x0f 0x00 0x00
>         Clock_Detected
>         Ch0_Ln0_Locked
>         Ch1_Ln1_Locked
>         Ch2_Ln2_Locked
> Character Error Detection:
>         Channel 0 Error Count: 0
>         Channel 1 Error Count: 0
>         Channel 2 Error Count: 0
> Manufacturer Specific: none
> 
> The debugfs scdc_status_show() function could just dump the 256 byte SCDC data as a single
> binary blob or output it as a hex dump, and leave the parsing to edid-decode, just as was
> done for InfoFrames in debugfs.
> 
> This would simplify the kernel code quite a bit, and edid-decode is much easier to adapt
> to new HDMI versions. So parsing of the SCDC data from the display is not dependent on
> the kernel version.
> 
> At minimum I would suggest that you dump the hex values before your parsed output.

Hmmmm, I guess to keep it simple we could just dump the hex values or
the binary, and do none of the parsing. But I wonder if a hex dump or
a raw binary is preferable. I don't like potentially ruining people's
terminal when they go cat poking, so I suppose hex dumping is the
answer.

Kind regards,
Nicolas Frattaroli

> 
> Regards,
> 
> 	Hans
> 
> 
> > +
> > +	return 0;
> > +
> > +err_conn_put:
> > +	drm_connector_put(connector);
> > +
> > +	return ret;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(scdc_status);
> > +
> > +/**
> > + * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
> > + * @connector: pointer to &struct drm_connector to operate on
> > + * @root: debugfs &struct dentry for the debugfs root of @connector
> > + *
> > + * Creates SCDC-related debugfs files for @connector. Must be called after
> > + * @root is already created.
> > + */
> > +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
> > +{
> > +	struct scdc_debugfs_priv *priv;
> > +
> > +	if (!root || !connector)
> > +		return;
> > +
> > +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return;
> > +
> > +	priv->connector = connector;
> > +
> > +	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
> > +}
> > +EXPORT_SYMBOL(drm_scdc_debugfs_init);
> > diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
> > index e9ccaeba56dd..3b3a4e0e48ba 100644
> > --- a/include/drm/display/drm_scdc_helper.h
> > +++ b/include/drm/display/drm_scdc_helper.h
> > @@ -30,6 +30,31 @@
> >  
> >  struct drm_connector;
> >  struct i2c_adapter;
> > +struct dentry;
> > +
> > +struct drm_scdc_status_flags {
> > +	/* Status Register 0 */
> > +	bool clock_detected;
> > +	bool ch0_locked;
> > +	bool ch1_locked;
> > +	bool ch2_locked;
> > +};
> > +
> > +struct drm_scdc_state {
> > +	/** @stf: contents of the status flag registers */
> > +	struct drm_scdc_status_flags stf;
> > +	/** @scramling_enabled: true if TMDS scrambling is on */
> > +	bool scrambling_enabled;
> > +	/** @scrambling_detected: true if the sink actually detected scrambling */
> > +	bool scrambling_detected;
> > +	/**
> > +	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
> > +	 * clock period, false if it's 1/10th of the clock period.
> > +	 */
> > +	bool tmds_bclk_x40;
> > +	/** @error_count: character error counts for each channel */
> > +	u16 error_count[3];
> > +};
> >  
> >  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> >  		  size_t size);
> > @@ -77,4 +102,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
> >  bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
> >  bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
> >  
> > +int drm_scdc_read_status0_flags(struct drm_connector *connector,
> > +				struct drm_scdc_status_flags *flags);
> > +int drm_scdc_read_state(struct drm_connector *connector,
> > +			struct drm_scdc_state *state);
> > +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
> > +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
> > +
> >  #endif
> > 
> 
> 

Re: [PATCH v4 2/4] drm/scdc-helper: Add scdc_status debugfs entry

[Hans Verkuil]

On 02/06/2026 17:44, Nicolas Frattaroli wrote:
> On Tuesday, 2 June 2026 08:40:34 Central European Summer Time Hans Verkuil wrote:
>> Hi Nicolas,
>>
>> As promised, here is my review:
>>
>> On 27/05/2026 16:03, Nicolas Frattaroli wrote:
>>> SCDC provides status information on the current display link. At the
>>> very least, it may be useful to expose this info through debugfs.
>>>
>>> Add a debugfs entry for it under the connector, which displays a few
>>> more details parsed out of the SCDC registers. A new
>>> drm_scdc_debugfs_init function can be called by the connector
>>> implementation to initialise the debugfs file.
>>>
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>> ---
>>>  drivers/gpu/drm/display/drm_scdc_helper.c | 237 ++++++++++++++++++++++++++++++
>>>  include/drm/display/drm_scdc_helper.h     |  32 ++++
>>>  2 files changed, 269 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
>>> index 8403f2390ab6..7739fb5e77a1 100644
>>> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
>>> @@ -24,11 +24,14 @@
>>>  #include <linux/export.h>
>>>  #include <linux/i2c.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/debugfs.h>
>>>  #include <linux/delay.h>
>>> +#include <linux/overflow.h>
>>>  
>>>  #include <drm/display/drm_scdc_helper.h>
>>>  #include <drm/drm_connector.h>
>>>  #include <drm/drm_device.h>
>>> +#include <drm/drm_managed.h>
>>>  #include <drm/drm_print.h>
>>>  
>>>  /**
>>> @@ -55,6 +58,11 @@
>>>  
>>>  #define SCDC_I2C_SLAVE_ADDRESS 0x54
>>>  
>>> +struct scdc_debugfs_priv {
>>> +	struct drm_connector *connector;
>>> +	struct drm_scdc_state state;
>>> +};
>>> +
>>>  /**
>>>   * drm_scdc_read - read a block of data from SCDC
>>>   * @adapter: I2C controller
>>> @@ -276,3 +284,232 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
>>>  	return true;
>>>  }
>>>  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
>>> +
>>> +/**
>>> + * drm_scdc_read_status0_flags - Read SCDC "Status Flags" Register
>>> + * @connector: pointer to &struct drm_connector to issue the scdc request on
>>> + * @flags: pointer to the caller's &struct drm_scdc_status_flags to output to
>>> + *
>>> + * Reads the SCDC Status Flags 0 register, and outputs its contents to the
>>> + * destination @flags. Contents of @flags are only valid if function returns 0.
>>> + *
>>> + * Returns: %0 on success, negative errno on error.
>>> + */
>>> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
>>> +				struct drm_scdc_status_flags *flags)
>>> +{
>>> +	int ret;
>>> +	u8 val;
>>> +
>>> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_STATUS_UPDATE);
>>
>> It doesn't hurt to set SCDC_STATUS_UPDATE to 1, but neither is there a need for it.
>> It just causes unnecessary DDC traffic IMHO.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = drm_scdc_readb(connector->ddc, SCDC_STATUS_FLAGS_0, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	flags->clock_detected = val & SCDC_CLOCK_DETECT;
>>> +	flags->ch0_locked = val & SCDC_CH0_LOCK;
>>> +	flags->ch1_locked = val & SCDC_CH1_LOCK;
>>> +	flags->ch2_locked = val & SCDC_CH2_LOCK;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_scdc_read_status0_flags);
>>> +
>>> +/**
>>> + * drm_scdc_read_error_counters - Read and clear SCDC error counters
>>> + * @connector: pointer to &struct drm_connector to issue the scdc request on
>>> + * @counter: Caller's u16 array with 3 elements to write the counter values into
>>> + *
>>> + * Read the SCDC channel error counters. If the count of channel *n* is valid,
>>> + * write it into counter[n]. Otherwise, set counter[n] to 0. Reads all counters
>>> + * in one read chunk, then clears every counter, as is mandated.
>>> + *
>>> + * Returns: %0 on success, negative errno on error.
>>> + */
>>> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3])
>>> +{
>>> +	u8 buf[7] = {};
>>> +	int ret;
>>> +	u8 sum = 0;
>>> +	int i;
>>> +
>>> +	ret = drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_CED_UPDATE);
>>
>> Same here: there is no need to set SCDC_CED_UPDATE.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = drm_scdc_read(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * Verify the "checksum", i.e. sum up everything including the checksum
>>> +	 * register as a wrapping unsigned 8-bit addition and verify it's 0.
>>> +	 */
>>> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
>>> +		sum = wrapping_add(u8, sum, buf[i]);
>>> +
>>> +	if (sum)
>>> +		return -EPROTO;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
>>> +		if (buf[i + 1] & SCDC_CHANNEL_VALID)
>>> +			counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
>>> +		else
>>> +			counter[i / 2] = 0;
>>> +
>>> +		buf[i] = 0;
>>> +		buf[i + 1] = 0;
>>> +	}
>>> +	buf[ARRAY_SIZE(buf) - 1] = 0;
>>> +
>>> +	return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, ARRAY_SIZE(buf));
>>
>> Huh? Reading the CED registers will automatically zero them as per the HDMI spec
>> (section 10.4.1.8, first paragraph). So there is no need to write to these registers.
>> Besides, they are read-only (table 10-14).
> 
> Yeah that's a mistake on my part. I thought the source had to clear
> them to signal that it wants more data, but now that I think about it,
> the sink obviously already knows when the source has read them because
> it sends an i2c read command.
> 
>>> +}
>>> +EXPORT_SYMBOL(drm_scdc_read_error_counters);
>>> +
>>> +/**
>>> + * drm_scdc_read_state - Update state from SCDC
>>> + * @connector: pointer to a &struct drm_connector on which to operate on
>>> + * @state: pointer to a &struct drm_scdc_state to fill
>>> + *
>>> + * Reads update flags from SCDC, and updates the parts of @state that SCDC
>>> + * claims have changed, as well as populating those where such a distinction
>>> + * can't be made.
>>> + *
>>> + * Returns: %0 on success, negative errno on failure.
>>> + */
>>> +int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
>>> +{
>>> +	u8 upd_flags[2] = {};
>>> +	struct i2c_adapter *ddc;
>>> +	struct drm_scdc *scdc;
>>> +	int ret;
>>> +	u8 val;
>>> +
>>> +	if (!state || !connector)
>>> +		return -ENODEV;
>>> +
>>> +	scdc = &connector->display_info.hdmi.scdc;
>>> +	ddc = connector->ddc;
>>> +
>>> +	if (!scdc->supported)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	ret = drm_scdc_readb(ddc, SCDC_TMDS_CONFIG, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	state->scrambling_enabled = val & SCDC_SCRAMBLING_ENABLE;
>>> +	state->tmds_bclk_x40 = val & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>>> +
>>> +	state->scrambling_detected = drm_scdc_get_scrambling_status(connector);
>>> +
>>> +	ret = drm_scdc_read(ddc, SCDC_UPDATE_0, &upd_flags, sizeof(upd_flags));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (upd_flags[0] & SCDC_STATUS_UPDATE) {
>>
>> Ah, so here you use SCDC_STATUS_UPDATE/SCDC_CED_UPDATE.
>>
>> I do not think this makes sense: for debugfs you just want to see the current
>> status and not 'what has changed since last time', which is what this basically
>> does.
> 
> My thought was that people will do a
> 
>   while sleep 0.5; do cat scdc_status ; echo "------"; done
> 
> and the update flags will make sure there isn't unnecessary traffic.

Based on my experience that's not how this is used in practice. You only look at
the SCDC data if there is something wrong, and then you just want to dump it
all and parse it.

My understanding is that these update flags are more meant for use in a driver where
you poll periodically to check if there are updates and then do something with that
in the driver. But for a debugfs feature it makes more sense IMHO to just dump the
full SCDC contents.

> 
>>> +		ret = drm_scdc_read_status0_flags(connector, &state->stf);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (upd_flags[0] & SCDC_CED_UPDATE) {
>>
>> And if the counters change only a little bit, then CED_UPDATE may not be set at all,
>> so you don't see such small changes.
>>
>>> +		ret = drm_scdc_read_error_counters(connector, state->error_count);
>>> +		if (ret)
>>> +			return ret;
>>
>> I would just always read the status flags and CED counters and report them. Especially
>> for debugfs usage.
>>
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_scdc_read_state);
>>> +
>>> +#define scdc_print_str(_f, key, s) \
>>> +	(seq_printf((_f), "%-30s: %s\n", (key), (s)))
>>> +#define scdc_print_flag(_f, key, val) \
>>> +	(scdc_print_str((_f), (key), str_yes_no((val))))
>>> +#define scdc_print_dec(_f, key, val) \
>>> +	(seq_printf((_f), "%-30s: %d\n", (key), (val)))
>>> +
>>> +static int scdc_status_show(struct seq_file *m, void *data)
>>> +{
>>> +	struct scdc_debugfs_priv *priv = m->private;
>>> +	struct drm_scdc_state *st = &priv->state;
>>> +	struct drm_connector *connector = priv->connector;
>>> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
>>> +	int ret;
>>> +
>>> +	drm_connector_get(connector);
>>> +
>>> +	if (connector->status != connector_status_connected) {
>>> +		ret = -ENODEV;
>>> +		goto err_conn_put;
>>> +	}
>>> +
>>> +	scdc_print_flag(m, "SCDC Supported", scdc->supported);
>>> +	if (!scdc->supported) {
>>> +		ret = 0;
>>> +		goto err_conn_put;
>>> +	}
>>> +
>>> +	scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
>>> +	scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
>>> +	scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
>>> +
>>> +	ret = drm_scdc_read_state(connector, st);
>>> +	drm_connector_put(connector);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
>>> +	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
>>> +
>>> +	if (st->tmds_bclk_x40)
>>> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
>>> +	else
>>> +		scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
>>> +
>>> +	scdc_print_flag(m, "Clock Detected", st->stf.clock_detected);
>>> +	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
>>> +	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
>>> +	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
>>> +
>>> +	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
>>> +	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
>>> +	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
>>
>> So, does parsing SCDC really belong in the kernel? Wouldn't it be easier
>> to just give a hexdump and let edid-decode parse it?
> 
> Assuming I follow your advice and no longer use the status update
> flags, does this provide any value over just having edid-decode access
> the i2c itself? This isn't a rhetorical question; I don't know if the
> i2c things as exposed by the kernel have drawbacks for multiple readers
> or even drawbacks being exposed at all.

Good question. The only reason for it is if the DDC bus is not exposed
as an i2c device. Otherwise edid-decode can just read it straight from the
i2c device. 'edid-decode' is these days closer to being a 'ddc-decode'
since it can read and parse EDID, SCDC and HDCP data.

SCDC is different from InfoFrames that do not go over DDC, so there debugfs
was really required to get hold of them.

Whether the kernel reads from the DDC bus or userspace makes no difference.
It normally has no impact, but if you were to read e.g. all 256 SCDC bytes
continuously it would cause problems with other traffic that has to happen
periodically (scrambler status testing, HDCP).

But that's not a normal scenario, of course.

> 
>>
>> We do the same for EDIDs and Infoframes.
>>
>> The edid-decode output (tested against my 4k TV) looks like this:
>>
>> edid-decode SCDC (hex):
>>
>> 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 03 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 80 00 80 00 80 80 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>
>> ----------------
>>
>> Sink Version: 1 Source Version: 1
>> Sink Supported Features: 0x00
>> Source Supported Features: 0x00
>> Update Flags: 0x03 0x00
>>         Status_Update
>>         CED_Update
>> TMDS Configuration: 0x03
>>         Scrambling_Enable
>>         TMDS_Bit_Clock_Ratio: 1/40
>> TMDS Scrambler Status: 0x01
>>         TMDS_Scrambling_Status
>> Sink Configuration: 0x00 0x00
>>         FRL_Rate: Disable FRL
>>         FFE_Levels: 0
>> Source Test Configuration: 0x00
>> Status Flags: 0x0f 0x00 0x00
>>         Clock_Detected
>>         Ch0_Ln0_Locked
>>         Ch1_Ln1_Locked
>>         Ch2_Ln2_Locked
>> Character Error Detection:
>>         Channel 0 Error Count: 0
>>         Channel 1 Error Count: 0
>>         Channel 2 Error Count: 0
>> Manufacturer Specific: none
>>
>> The debugfs scdc_status_show() function could just dump the 256 byte SCDC data as a single
>> binary blob or output it as a hex dump, and leave the parsing to edid-decode, just as was
>> done for InfoFrames in debugfs.
>>
>> This would simplify the kernel code quite a bit, and edid-decode is much easier to adapt
>> to new HDMI versions. So parsing of the SCDC data from the display is not dependent on
>> the kernel version.
>>
>> At minimum I would suggest that you dump the hex values before your parsed output.
> 
> Hmmmm, I guess to keep it simple we could just dump the hex values or
> the binary, and do none of the parsing. But I wonder if a hex dump or
> a raw binary is preferable. I don't like potentially ruining people's
> terminal when they go cat poking, so I suppose hex dumping is the
> answer.

Both InfoFrames and the edid as exposed in debugfs and /sys are binary. edid-decode can
handle both binary and hex.

So binary would be more consistent. But I have no strong opinion on this.

If you decide to just dump the SCDC data, then read it in two blocks of 128 bytes.
You should not read 256 bytes of data over DDC in one go. This will fail if
DisplayPort-to-HDMI adapters are used: the DP REMOTE_I2C_READ/WRITE requests use 8 bit
for the read/write length, so 256 maps to 0, so nothing is actually read or written.

I was bitten by that in edid-decode :-) It took me a while to figure out why I never
saw any valid data.

Regards,

	Hans

> 
> Kind regards,
> Nicolas Frattaroli
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>
>>> +
>>> +	return 0;
>>> +
>>> +err_conn_put:
>>> +	drm_connector_put(connector);
>>> +
>>> +	return ret;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(scdc_status);
>>> +
>>> +/**
>>> + * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
>>> + * @connector: pointer to &struct drm_connector to operate on
>>> + * @root: debugfs &struct dentry for the debugfs root of @connector
>>> + *
>>> + * Creates SCDC-related debugfs files for @connector. Must be called after
>>> + * @root is already created.
>>> + */
>>> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
>>> +{
>>> +	struct scdc_debugfs_priv *priv;
>>> +
>>> +	if (!root || !connector)
>>> +		return;
>>> +
>>> +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return;
>>> +
>>> +	priv->connector = connector;
>>> +
>>> +	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
>>> +}
>>> +EXPORT_SYMBOL(drm_scdc_debugfs_init);
>>> diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
>>> index e9ccaeba56dd..3b3a4e0e48ba 100644
>>> --- a/include/drm/display/drm_scdc_helper.h
>>> +++ b/include/drm/display/drm_scdc_helper.h
>>> @@ -30,6 +30,31 @@
>>>  
>>>  struct drm_connector;
>>>  struct i2c_adapter;
>>> +struct dentry;
>>> +
>>> +struct drm_scdc_status_flags {
>>> +	/* Status Register 0 */
>>> +	bool clock_detected;
>>> +	bool ch0_locked;
>>> +	bool ch1_locked;
>>> +	bool ch2_locked;
>>> +};
>>> +
>>> +struct drm_scdc_state {
>>> +	/** @stf: contents of the status flag registers */
>>> +	struct drm_scdc_status_flags stf;
>>> +	/** @scramling_enabled: true if TMDS scrambling is on */
>>> +	bool scrambling_enabled;
>>> +	/** @scrambling_detected: true if the sink actually detected scrambling */
>>> +	bool scrambling_detected;
>>> +	/**
>>> +	 * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
>>> +	 * clock period, false if it's 1/10th of the clock period.
>>> +	 */
>>> +	bool tmds_bclk_x40;
>>> +	/** @error_count: character error counts for each channel */
>>> +	u16 error_count[3];
>>> +};
>>>  
>>>  int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
>>>  		  size_t size);
>>> @@ -77,4 +102,11 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
>>>  bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
>>>  bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
>>>  
>>> +int drm_scdc_read_status0_flags(struct drm_connector *connector,
>>> +				struct drm_scdc_status_flags *flags);
>>> +int drm_scdc_read_state(struct drm_connector *connector,
>>> +			struct drm_scdc_state *state);
>>> +int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]);
>>> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>> +
>>>  #endif
>>>
>>
>>
> 
> 
> 
>