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

[Nicolas Frattaroli]

On Friday, 22 May 2026 19:38:24 Central European Summer Time Daniel Stone wrote:
> Hi Nicolas,
> 
> On Wed, 20 May 2026 at 14:36, Nicolas Frattaroli
> <nicolas.frattaroli@xxxxxxxxxxxxx> 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.

Yeah, I think you found an actual bug here. Maybe iterating once over
i = 0 to buf_sz is the better choice here anyway.

I'm also realising now that for lane 4 I'm not writing 0 to the fields
after being done with them, so that's not right either.

Thanks for the review, will send v3 in short order.

Kind regards,
Nicolas Frattaroli

> 
> With or without that suggestion, series is:
> Reviewed-by: Daniel Stone <daniels@xxxxxxxxxxxxx>
> 
> Thanks for pulling all this together!
> 
> Cheers,
> Daniel
>