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

Mon Dec 4 16:52:30 UTC 2023

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Monday, December 4, 2023 10:18 PM
> To: Shankar, Uma <uma.shankar at intel.com>; Ville Syrjälä
> <ville.syrjala at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET
> when using rate select
> 
> On Mon, 04 Dec 2023, "Shankar, Uma" <uma.shankar at intel.com> wrote:
> >> -----Original Message-----
> >> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> >> Of Jani Nikula
> >> 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
> >> 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?
> >>
> >> 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.
> >
> > The spec does leave it a bit open on this one:
> >
> > 115h: LINK_RATE_SET and TX_GTC_CAPABILITY
> > • DPCD 00001h = 00h/DPCD 02201h = 00h – Source device shall use this field to
> choose
> > the link rate, and the Sink device shall ignore DPCD 00100h
> > • DPCD 00001h/DPCD 02201h = Valid link rate – Source device may optionally
> choose
> > a link rate associated with HBR3, HBR2,HBR, –or– RBR by writing to DPCD
> 00100h
> >
> > So the 2nd point here does mentions that sinks can optionally use value of
> 00100h
> > if 2201h is not 00. So programming a value to this DPCD is not right unless we
> program
> > the right value (not 0).
> >
> > I feel safe way would be be to go with LINK_BW_SET for DP1.3 and for DP1.4+
> always use
> > LINK_RATE_SET and have it mutually exclusive.
> >
> > Some TCONs would have ignored and we got lucky but we can't leave it
> ambiguous, we will be compliant
> > to spec if we don't write 0x100. So let's go with this change.
> 
> Moreover, there are only four documented valid values for this register,
> 0x06, 0xa, 0x14, and 0x1e, all other values are reserved. In that sense
> it's also wrong to write 0x00.

Yeah, writing 0 is done with an intention to disable it but that’s not the way
to have this option disabled. Infact there is no reason to write to it for DP1.4+
if sink is compliant.

Regards,
Uma Shankar

> BR,
> Jani.
> 
> 
> 
> 
> >
> > Regards,
> > Uma Shankar
> >
> >> No r-b, so do you have any better ideas?
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> >> +  }
> >> >>  }
> >> >>
> >> >>  /*
> >> >> --
> >> >> 2.39.2
> >>
> >> --
> >> Jani Nikula, Intel
> 
> --
> Jani Nikula, Intel