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