On 2020-01-09 4:13 p.m., Mario Kleiner wrote:
On Thu, Jan 9, 2020 at 7:44 PM Harry Wentland <hwentlan@amd.com mailto:hwentlan@amd.com> wrote:
On 2020-01-09 10:20 a.m., Mario Kleiner wrote: > If the current eDP link settings, as read from hw, provide a higher > bandwidth than the verified_link_cap ones (= reported_link_cap), then > override verified_link_cap with current settings. > > These initial current eDP link settings have been set up by > firmware during boot, so they should work on the eDP panel. > Therefore use them if the firmware thinks they are good and > they provide higher link bandwidth, e.g., to enable higher > resolutions / color depths. >Hi Harry, happy new year!
Frohes Neues. :)
This only works when taking over from UEFI, so on boot or resume from hibernate. This wouldn't work on a normal suspend/resume.See the other thread i just cc'ed you on. Depends if dc_link_detect_helper() gets skipped/early returns or not on EDP. Some if statement suggests it might get skipped on EDP + resume?
You've likely looked at the code more closely while debugging this than I have recently. It looks like we indeed skip detection if we've previously detected the eDP sink.
Can you check if setting link->dc->config.optimize_edp_link_rate (see first if statement in detect_edp_sink_caps) fixes this? I imagine we need to read the reported settings from DP_SUPPORTED_LINK_RATES and fail to do so.Tried that already (see other mail), replacing the whole if statement with a if (true) to force reading DP_SUPPORTED_LINK_RATES. The whole table reads back as all-zero, and versions are DP 1.1, eDP 1.3, not 1.4+ as what seems to be required. The use the classic link bw stuff, but with a non-standard link bandwidth multiplier of 0xc, and a reported DP_MAX_LINK_RATE of 0xa, contradicting the 0xc setting that the firmware sets at bootup.
Seems to be a very Apple thing...
Indeed. I think it was a funky panel that was "ahead of its time" and ahead of the spec.
I would prefer a DPCD quirk for this panel that updates the reported DP caps, rather than picking the "current" ones from the FW lightup.
Harry
-mario
Thanks, Harry > This fixes a problem found on the MacBookPro 2017 Retina panel: > > The panel reports 10 bpc color depth in its EDID, and the > firmware chooses link settings at boot which support enough > bandwidth for 10 bpc (324000 kbit/sec aka LINK_RATE_RBR2), > but the DP_MAX_LINK_RATE dpcd register only reports 2.7 Gbps > as possible, so verified_link_cap is only good for 2.7 Gbps > and 8 bpc, not providing the full color depth of the panel. > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com <mailto:mario.kleiner.de@gmail.com>> > Cc: Alex Deucher <alexander.deucher@amd.com <mailto:alexander.deucher@amd.com>> > --- > drivers/gpu/drm/amd/display/dc/core/dc_link.c | 21 +++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c > index 5ea4a1675259..f3acdb8fead5 100644 > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c > @@ -819,6 +819,27 @@ static bool dc_link_detect_helper(struct dc_link *link, > case SIGNAL_TYPE_EDP: { > detect_edp_sink_caps(link); > read_current_link_settings_on_detect(link); > + > + /* If cur_link_settings provides higher bandwidth than > + * verified_link_cap, then use cur_link_settings as new > + * verified_link_cap, as it obviously works according to > + * firmware boot setup. > + * > + * This has been observed on the Apple MacBookPro 2017 > + * Retina panel, which boots with a link setting higher > + * than what dpcd[DP_MAX_LINK_RATE] claims as possible. > + * Overriding allows to run the panel at 10 bpc / 30 bit. > + */ > + if (dc_link_bandwidth_kbps(link, &link->cur_link_settings) > > + dc_link_bandwidth_kbps(link, &link->verified_link_cap)) { > + DC_LOG_DETECTION_DP_CAPS( > + "eDP current link setting bw %d kbps > verified_link_cap %d kbps. Override.", > + dc_link_bandwidth_kbps(link, &link->cur_link_settings), > + dc_link_bandwidth_kbps(link, &link->verified_link_cap)); > + > + link->verified_link_cap = link->cur_link_settings; > + } > + > sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C_OVER_AUX; > sink_caps.signal = SIGNAL_TYPE_EDP; > break; >