[Intel-gfx] [PATCH] drm/i915/dp: Skip the HW readout of DPCD on disabled encoders

[Imre Deak]

On Sat, Oct 16, 2021 at 12:59:46AM +0300, Jani Nikula wrote:
> On Fri, 15 Oct 2021, "Souza, Jose" <jose.souza at intel.com> wrote:
> > On Fri, 2021-10-15 at 15:10 +0300, Imre Deak wrote:
> >> Reading out the DP encoders' DPCD during booting or resume is only
> >> required for enabled encoders: such encoders may be modesetted during
> >> the initial commit and the link training this involves depends on an
> >> initialized DPCD. For DDI encoders reading out the DPCD is skipped, do
> >> the same on pre-DDI platforms.
> >
> > Missing fixes tag
> >
> >> 
> >> Cc: José Roberto de Souza <jose.souza at intel.com>
> >> Signed-off-by: Imre Deak <imre.deak at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 9d8132dd4cc5a..23de500d56b52 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -2007,6 +2007,9 @@ void intel_dp_sync_state(struct intel_encoder *encoder,
> >>  {
> >>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>  
> >> +	if (!crtc_state)
> >> +		return;
> >
> > crtc_state is not used
> 
> This is why it's so subtle. The commit a532cde31de3 ("drm/i915/tc: Fix
> TypeC port init/resume time sanitization") changes when the sync_state
> hook is called, and now it's also called for disabled encoders, and
> crtc_state != NULL is the way to check that now. Which absolutely must
> be documented in this fix! (And I'm not sure if even that is enough in
> the long term, it seems to me the change is just too subtle and we'll
> get it wrong again.)

The intention was to call the hook on TypeC platforms, where the
encoder/PHY state has to be synced even if the encoder is disabled. I
missed both the dsi and - as now turns out - the g4x dp hooks which I
intended in a532cde31de3 to keep behaving as before.

> I'm guessing the intel_dp_max_common_rate() call gets inlined in
> intel_dp_sync_state(), and it goes wrong with intel_dp->num_common_rates
> being 0 and the array index being -1.

Yes, I came to the same conclusion, see
https://gitlab.freedesktop.org/drm/intel/-/issues/4297

Luckily this doesn't cause an actual problem for regular users, since
the out-of-bound 

intel_dp->common_rates[intel_dp->num_common_rates - 1];

access in intel_dp_max_common_rate() in case num_common_rates is 0 will
just return the value of intel_dp->num_common_rates (0). If
KCONFIG_UBSAN is enabled this access will trigger a kernel crash assert
(again luckily for us, even though there could be an explanation message
for the assert).

I'll resend this patch with the root cause for 4297 explained, and
stable CC'd. Also I'll send related patches that will ensure that the
link config parameters derived from DPCD have a valid default value
even in the lack of a valid DPCD.

Imo we should also enable KCONFIG_UBSAN in CI.

> Anyway, having said that, we'll need to stop guessing and dig into the
> root cause.
> 
> BR,
> Jani.
> 
> >
> >> +
> >>  	/*
> >>  	 * Don't clobber DPCD if it's been already read out during output
> >>  	 * setup (eDP) or detect.
> >
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center