| Message ID | 20211015121031.870282-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Skip the HW readout of DPCD on disabled encoders | expand |
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@intel.com> > Signed-off-by: Imre Deak <imre.deak@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 > + > /* > * Don't clobber DPCD if it's been already read out during output > * setup (eDP) or detect.
On Fri, 15 Oct 2021, "Souza, Jose" <jose.souza@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@intel.com> >> Signed-off-by: Imre Deak <imre.deak@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.) 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. 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. >
On Sat, Oct 16, 2021 at 12:59:46AM +0300, Jani Nikula wrote: > On Fri, 15 Oct 2021, "Souza, Jose" <jose.souza@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@intel.com> > >> Signed-off-by: Imre Deak <imre.deak@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
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; + /* * Don't clobber DPCD if it's been already read out during output * setup (eDP) or detect.
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. Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 3 +++ 1 file changed, 3 insertions(+)