Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select

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

On Fri, 01 Dec 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
>> The eDP 1.5 spec adds a clarification for eDP 1.4x:
>> 
>> > For eDP v1.4x, if the Source device chooses the Main-Link rate by way
>> > of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
>> 
>> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
>> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the panel
>> to ignore the rate set method.
>
> What a terrible way to specify this :( This means the device must 
> hav some internal state to keep track of whethe BW_SET was ever
> written.

Indeed.

Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD 0020Ch)
which exposes the internal state as link rate set status, and whether
that status is valid or not.

Overall the spec looks like that's just for status, but the register is
annotated Write/Read so who knows.

>
>> 
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
>> Tested-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  .../drm/i915/display/intel_dp_link_training.c | 23 +++++++++++--------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> index dbc1b66c8ee4..6336a39030a4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
>>  			    const struct intel_crtc_state *crtc_state,
>>  			    u8 link_bw, u8 rate_select)
>>  {
>> -	u8 link_config[2];
>> +	u8 lane_count = crtc_state->lane_count;
>>  
>> -	/* Write the link configuration data */
>> -	link_config[0] = link_bw;
>> -	link_config[1] = crtc_state->lane_count;
>>  	if (crtc_state->enhanced_framing)
>> -		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> -	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> +		lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> +
>> +	if (link_bw) {
>> +		/* eDP 1.3 and earlier link bw set method. */
>> +		u8 link_config[] = { link_bw, lane_count };
>>  
>> -	/* eDP 1.4 rate select method. */
>> -	if (!link_bw)
>> -		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
>> -				  &rate_select, 1);
>> +		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config,
>> +				  ARRAY_SIZE(link_config));
>> +	} else {
>> +		/* eDP 1.4 rate select method. */
>> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_LANE_COUNT_SET, lane_count);
>> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_LINK_RATE_SET, rate_select);
>
> Doesn't look there's anything in the spec that specifies when the device
> is supposed to reset its internal state to stop ignoring DP_LINK_RATE_SET.
> Do we know when this panel does it? When VDD is removed?

No idea. Animesh?

I think it's just crazy writing 0 to explicitly disable DP_LINK_BW_SET
renders DP_LINK_RATE_SET unusable. Pretty sure we've seen panels where
this works as you'd expect.

And the above depends on pre-os using the same logic as us for choosing
DP_LINK_RATE_SET. GOP seems to do that. But if it or some other pre-os
used DP_LINK_BW_SET, we'd fail. With some other panels, writing the 0
might recover from that.

No r-b, so do you have any better ideas?


BR,
Jani.


>
>> +	}
>>  }
>>  
>>  /*
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel