Re: [RFC] drm/i915/dp: Log message when limiting SST link rate

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 01, 2024 at 11:07:43AM +0200, Jani Nikula wrote:
> On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@intel.com> 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@linux.intel.com>
> > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Charlton Lin <charlton.lin@intel.com>
> > ---
> >  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