On Fri, Mar 01, 2024 at 11:07:43AM +0200, Jani Nikula wrote:
> On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@xxxxxxxxx> wrote:
> > Driver currently limits link rate up to HBR3 in SST mode. Log a
> > message with monitor vendor, product id, and MSTM_CAP to
> > help understand what monitors are being downgraded by this limit.
>
> Any logging of the sink details should be done exactly once at detect
> time. No matter what happens after that, there's no need to spam the
> dmesg with duplicate information. If the information currently logged is
> insufficient, please add it at detect, where it helps *all* debugging,
> not just a single restricted case.
>
> More details inline.
>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Khaled Almahallawy <khaled.almahallawy@xxxxxxxxx>
> > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > Signed-off-by: Charlton Lin <charlton.lin@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 6ece2c563c7a..0b2d6d88fd37 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> > false,
> > &limits);
> >
> > + if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) {
>
> If link training has failed and the max rate is limited due to that,
> you'll hit this on all retraining attempts.
>
> And, of course, this is hit at every single modeset for 128b/132b
> capable displays that don't support MST. That's just excessive.
>
> > + u8 mstm_cap;
> > + u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc);
>
> That reads the entire EDID base block again in the middle of compute
> config. A big no. We've also taken care to cache the EDID to avoid
> duplicate reads otherwise.
>
> Moreover, it ignores any EDID overrides etc.
>
> > + char vend[4];
> > + u16 product_id;
> > +
> > + drm_dbg_kms(&i915->drm,
> > + "Limiting LR from max common rate %d to %d\n",
> > + intel_dp_max_common_rate(intel_dp), limits.max_rate);
> > +
> > + drm_edid_decode_panel_id(panel_id, vend, &product_id);
> > +
> > + if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 &&
> > + drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1)
>
> We shouldn't add extra DPCD reads nilly willy either. This should be
> debug logged once at detect. I've got a WIP series that should address
> this [1], once I fix it.
Also the stone tablets say "thou shalt not touch the hardware
in atomic_check()/.compute_config()".
This stuff gets executed for every TEST_ONLY commit, which means it:
- potentially runs while the hardware is asleep
- should be relatively fast
>
> > + drm_dbg_kms(&i915->drm,
> > + "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n",
> > + vend, product_id, mstm_cap);
>
> Ville and I have discussed adding entire EDID debug logging at
> drm_edid.c level, which would address a lot more things than this.
>
> BR,
> Jani.
>
>
> [1] https://patchwork.freedesktop.org/series/129468/
>
>
>
> > + }
> > +
> > if (!dsc_needed) {
> > /*
> > * Optimize for slow and wide for everything, because there are some
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
