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

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

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

-- 
Jani Nikula, Intel Open Source Graphics Center