On Mon, 04 Dec 2023, "Shankar, Uma" <uma.shankar@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Jani >> Nikula >> Sent: Monday, December 4, 2023 3:28 PM >> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Subject: Re: [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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> >> --- >> >> .../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. 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
