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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

> +			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