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

[Manna, Animesh]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Monday, December 4, 2023 3:28 PM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Manna, Animesh
> <animesh.manna at intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to
> DP_LINK_BW_SET when using rate select
> 
> On Fri, 01 Dec 2023, Ville Syrjälä <ville.syrjala at 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 at intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula at 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?

Hi Jani/Ville,

Tried below experiment and sharing my observation below:
Forcefully changed the value of dpcd 0x100 (LINK_BW_SET) to random value (0x99) in edp_init_connector and later while VDD is on during modeset sequence I can see it is not holding its value rather got reset to default value. This will confirm when VDD is removed panel reset its internal state.

Regards,
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