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

[Daniel Stone <daniel@fooishbar.org>]

Hi Nicolas,

On Wed, 20 May 2026 at 14:36, Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
> -       for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> +       for (i = 0; i <= ERR_DET_OFF(SCDC_ERR_DET_2_H); i += 2) {
>                 if (buf[i + 1] & SCDC_CHANNEL_VALID)
>                         counter[i / 2] = buf[i] | (buf[i + 1] & ~SCDC_CHANNEL_VALID) << 8;
>                 else
> @@ -355,9 +433,15 @@ int drm_scdc_read_error_counters(struct drm_connector *connector, u16 counter[3]
>                 buf[i] = 0;
>                 buf[i + 1] = 0;
>         }
> -       buf[ARRAY_SIZE(buf) - 1] = 0;
> +       buf[ERR_DET_OFF(SCDC_ERR_DET_CHECKSUM)] = 0;
> +
> +       if (num_lanes == 4)
> +               counter[3] = buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] |
> +                            (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] & ~SCDC_CHANNEL_VALID) << 8;
> +       else
> +               counter[3] = 0;

I get that this is separate from the loop above because it's
discontiguous, but is it also missing the validity check? I mean,
lane_count == 3 means 'fsvo 3 which may be 1 or 2' so have to check
SCDC_CHANNEL_VALID there, but if we have 4 lanes explicitly specified
then we are we missing the check for the valid bit, or is it just
always valid / we shouldn't check?

tbh having it separately is a bit messy. I half-wonder if unrolling
the loop wouldn't be cleaner, e.g.:
#define GET_SCDC_ERR_CNT(c) { \
        bool valid = (buf[(c) * 2 + 1] & SCDC_CHANNEL_VALID; \
        if (valid) {  \
                counter[c] = buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _L)]; \
                counter[c] |= (buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _H)]
& ~SCDC_CHANNEL_VALID) << 8; \
        } else {
                counter[c] = 0;
        } \
}

GET_SCDC_ERR_CNT(0);
GET_SCDC_ERR_CNT(1);
GET_SCDC_ERR_CNT(2);
if (num_lanes == 4)
        GET_SCDC_ERR_CNT(3);
else
        counter[3] = 0;

Up to you though. It's a messy format, with no objectively great solution.

With or without that suggestion, series is:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Thanks for pulling all this together!

Cheers,
Daniel