* Some eDP fixes/improvements.
@ 2020-01-09 15:20 Mario Kleiner
2020-01-09 15:20 ` [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read Mario Kleiner
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Mario Kleiner @ 2020-01-09 15:20 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: alexander.deucher, mario.kleiner.de
Hi and happy new year!
Since i now have a MBP 2017 to play with, with a 10 bit Retina panel,
and Polaris gpu, i'm trying to get it to get 10 bits, and found one
small bug [fix: patch1], and a quirk of Apples Retina eDP sink, for
which i propose patch2 as solution. I sent a similar patch to i915 to
make 10 bit Retina work with the Intel Kabylake igp on that machine.
If these make sense, it would be cool to still get them into drm-fixes
for Linux 5.5, so users of spring distro updates like Ubuntu 20.04 LTS
can get a more colorful new year.
thanks,
-mario
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read. 2020-01-09 15:20 Some eDP fixes/improvements Mario Kleiner @ 2020-01-09 15:20 ` Mario Kleiner 2020-01-09 18:40 ` Harry Wentland 2020-01-09 15:20 ` [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones Mario Kleiner 2020-01-11 11:41 ` Some eDP fixes/improvements Timo Aaltonen 2 siblings, 1 reply; 11+ messages in thread From: Mario Kleiner @ 2020-01-09 15:20 UTC (permalink / raw) To: dri-devel, amd-gfx; +Cc: alexander.deucher, mario.kleiner.de, Martin Leung read_current_link_settings_on_detect() on eDP 1.4+ may use the edp_supported_link_rates table which is set up by detect_edp_sink_caps(), so that function needs to be called first. Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: Martin Leung <martin.leung@amd.com> --- drivers/gpu/drm/amd/display/dc/core/dc_link.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cef8c1ba9797..5ea4a1675259 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c @@ -817,8 +817,8 @@ static bool dc_link_detect_helper(struct dc_link *link, } case SIGNAL_TYPE_EDP: { - read_current_link_settings_on_detect(link); detect_edp_sink_caps(link); + read_current_link_settings_on_detect(link); sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C_OVER_AUX; sink_caps.signal = SIGNAL_TYPE_EDP; break; -- 2.24.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read. 2020-01-09 15:20 ` [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read Mario Kleiner @ 2020-01-09 18:40 ` Harry Wentland 2020-01-11 0:17 ` Alex Deucher 0 siblings, 1 reply; 11+ messages in thread From: Harry Wentland @ 2020-01-09 18:40 UTC (permalink / raw) To: Mario Kleiner, dri-devel, amd-gfx Cc: alexander.deucher, mario.kleiner.de, Martin Leung On 2020-01-09 10:20 a.m., Mario Kleiner wrote: > read_current_link_settings_on_detect() on eDP 1.4+ may use the > edp_supported_link_rates table which is set up by > detect_edp_sink_caps(), so that function needs to be called first. > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: Martin Leung <martin.leung@amd.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> This also fixes our assumption that retrieve_link_cap is the first DPCD reads we perform during detection. Harry > --- > drivers/gpu/drm/amd/display/dc/core/dc_link.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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 cef8c1ba9797..5ea4a1675259 100644 > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c > @@ -817,8 +817,8 @@ static bool dc_link_detect_helper(struct dc_link *link, > } > > case SIGNAL_TYPE_EDP: { > - read_current_link_settings_on_detect(link); > detect_edp_sink_caps(link); > + read_current_link_settings_on_detect(link); > sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C_OVER_AUX; > sink_caps.signal = SIGNAL_TYPE_EDP; > break; > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read. 2020-01-09 18:40 ` Harry Wentland @ 2020-01-11 0:17 ` Alex Deucher 0 siblings, 0 replies; 11+ messages in thread From: Alex Deucher @ 2020-01-11 0:17 UTC (permalink / raw) To: Harry Wentland Cc: mario.kleiner.de, amd-gfx list, Maling list - DRI developers, Martin Leung, Deucher, Alexander Applied. Thanks! Alex On Thu, Jan 9, 2020 at 1:41 PM Harry Wentland <hwentlan@amd.com> wrote: > > On 2020-01-09 10:20 a.m., Mario Kleiner wrote: > > read_current_link_settings_on_detect() on eDP 1.4+ may use the > > edp_supported_link_rates table which is set up by > > detect_edp_sink_caps(), so that function needs to be called first. > > > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > > Cc: Martin Leung <martin.leung@amd.com> > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > This also fixes our assumption that retrieve_link_cap is the first DPCD > reads we perform during detection. > > Harry > > > --- > > drivers/gpu/drm/amd/display/dc/core/dc_link.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > 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 cef8c1ba9797..5ea4a1675259 100644 > > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c > > @@ -817,8 +817,8 @@ static bool dc_link_detect_helper(struct dc_link *link, > > } > > > > case SIGNAL_TYPE_EDP: { > > - read_current_link_settings_on_detect(link); > > detect_edp_sink_caps(link); > > + read_current_link_settings_on_detect(link); > > sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C_OVER_AUX; > > sink_caps.signal = SIGNAL_TYPE_EDP; > > break; > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-01-09 15:20 Some eDP fixes/improvements Mario Kleiner 2020-01-09 15:20 ` [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read Mario Kleiner @ 2020-01-09 15:20 ` Mario Kleiner 2020-01-09 18:44 ` Harry Wentland 2020-01-11 11:41 ` Some eDP fixes/improvements Timo Aaltonen 2 siblings, 1 reply; 11+ messages in thread From: Mario Kleiner @ 2020-01-09 15:20 UTC (permalink / raw) To: dri-devel, amd-gfx; +Cc: alexander.deucher, mario.kleiner.de 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. 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> Cc: Alex Deucher <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; -- 2.24.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-01-09 15:20 ` [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones Mario Kleiner @ 2020-01-09 18:44 ` Harry Wentland 2020-01-09 21:13 ` Mario Kleiner 0 siblings, 1 reply; 11+ messages in thread From: Harry Wentland @ 2020-01-09 18:44 UTC (permalink / raw) To: Mario Kleiner, dri-devel, amd-gfx; +Cc: alexander.deucher, mario.kleiner.de 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. > This only works when taking over from UEFI, so on boot or resume from hibernate. This wouldn't work on a normal suspend/resume. 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. 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> > Cc: Alex Deucher <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; > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-01-09 18:44 ` Harry Wentland @ 2020-01-09 21:13 ` Mario Kleiner 2020-01-09 21:26 ` Harry Wentland 0 siblings, 1 reply; 11+ messages in thread From: Mario Kleiner @ 2020-01-09 21:13 UTC (permalink / raw) To: Harry Wentland; +Cc: Alex Deucher, mario.kleiner.de, amd-gfx list, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 4481 bytes --] On Thu, Jan 9, 2020 at 7:44 PM Harry Wentland <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! 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? > 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... -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> > > Cc: Alex Deucher <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; > > > [-- Attachment #1.2: Type: text/html, Size: 6145 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-01-09 21:13 ` Mario Kleiner @ 2020-01-09 21:26 ` Harry Wentland 2020-02-27 19:11 ` Mario Kleiner 0 siblings, 1 reply; 11+ messages in thread From: Harry Wentland @ 2020-01-09 21:26 UTC (permalink / raw) To: Mario Kleiner; +Cc: Alex Deucher, mario.kleiner.de, amd-gfx list, dri-devel 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; > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-01-09 21:26 ` Harry Wentland @ 2020-02-27 19:11 ` Mario Kleiner 2020-02-28 21:41 ` Mario Kleiner 0 siblings, 1 reply; 11+ messages in thread From: Mario Kleiner @ 2020-02-27 19:11 UTC (permalink / raw) To: Harry Wentland; +Cc: Alex Deucher, amd-gfx list, dri-devel Hi Harry Ok, back from various other emergencies and deadlines, sorry for the late reply. I also fixed my e-mail address - it was mistyped, causing all these delivery failures :/ On Thu, Jan 9, 2020 at 10:26 PM Harry Wentland <hwentlan@amd.com> wrote: > > 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. > > > ... snip ... > > > > > > 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 > How would i do this? I see various options: I could rewrite my current patch, move it down inside dc_link_detect_helper() until after the edid was read and we have vendor/model id available, then say if(everything that's there now && (vendor=Apple) && (model=Troublesomepanel)) { ... } Or i could add quirk code to detect_edp_sink_caps() after retrieve_link_cap() [or inside retrieve_link_cap] to override the reported_link_cap. But at that point we don't have edid yet and therefore no vendor/model id. Is there something inside the dpcd one can use to uniquely identify this display model? struct dp_device_vendor_id sink_id; queried inside retrieve_link_cap() sounds like it could be a unique id? I don't know about that. My intention was to actually do nothing on the AMD side here, as my photometer measurements suggest that the panel gives better quality results for >= 10 bpc output if it is operated at 8 bit and then the gpu's spatial dithering convincingly fakes the extra bits. Quality seems worse if one actually switches the panel into 10 bpc, as it doesn't seem to be a real 10 bit panel, just a 8 bit panel that accepts 10 bit and then badly dithers it to 10 bit. The situation has changed for Linux 5.6-rc, because of this recent commit from Roman Li, which is already in 5.6-rc: 4a8ca46bae8affba063aabac85a0b1401ba810a3 "drm/amd/display: Default max bpc to 16 for eDP" While that commit supposedly fixes some darkness on some other eDP panel, it now breaks my eDP panel. It leaves edid reported bpc unclamped, so the driver uses 10 bpc as basis for required bandwidth calculations and then the required bandwidth for all modes exceeds the link bandwidth. I end with the eDP panel having no valid modes at all ==> Panel goes black, game over. We either need to revert that commit for drm-fixes, or quirk it for the specific panels that are troublesome, or need to get some solution into 5.6-rc, otherwise there will be a lot of regressions for at least Apple MBP users. thanks, -mario > > -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; > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones. 2020-02-27 19:11 ` Mario Kleiner @ 2020-02-28 21:41 ` Mario Kleiner 0 siblings, 0 replies; 11+ messages in thread From: Mario Kleiner @ 2020-02-28 21:41 UTC (permalink / raw) To: Harry Wentland; +Cc: Alex Deucher, amd-gfx list, dri-devel On Thu, Feb 27, 2020 at 8:11 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > > Hi Harry > > Ok, back from various other emergencies and deadlines, sorry for the > late reply. I also fixed my e-mail address - it was mistyped, causing > all these delivery failures :/ > > On Thu, Jan 9, 2020 at 10:26 PM Harry Wentland <hwentlan@amd.com> wrote: > > > > 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. > > > > > ... snip ... > > > > > > > > > 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 > > > > How would i do this? I see various options: > > I could rewrite my current patch, move it down inside > dc_link_detect_helper() until after the edid was read and we have > vendor/model id available, then say if(everything that's there now && > (vendor=Apple) && (model=Troublesomepanel)) { ... } > > Or i could add quirk code to detect_edp_sink_caps() after > retrieve_link_cap() [or inside retrieve_link_cap] to override the > reported_link_cap. But at that point we don't have edid yet and > therefore no vendor/model id. Is there something inside the dpcd one > can use to uniquely identify this display model? > > struct dp_device_vendor_id sink_id; queried inside retrieve_link_cap() > sounds like it could be a unique id? I don't know about that. > > My intention was to actually do nothing on the AMD side here, as my > photometer measurements suggest that the panel gives better quality > results for >= 10 bpc output if it is operated at 8 bit and then the > gpu's spatial dithering convincingly fakes the extra bits. Quality > seems worse if one actually switches the panel into 10 bpc, as it > doesn't seem to be a real 10 bit panel, just a 8 bit panel that > accepts 10 bit and then badly dithers it to 10 bit. > > The situation has changed for Linux 5.6-rc, because of this recent > commit from Roman Li, which is already in 5.6-rc: > 4a8ca46bae8affba063aabac85a0b1401ba810a3 "drm/amd/display: Default max > bpc to 16 for eDP" > > While that commit supposedly fixes some darkness on some other eDP > panel, it now breaks my eDP panel. It leaves edid reported bpc > unclamped, so the driver uses 10 bpc as basis for required bandwidth > calculations and then the required bandwidth for all modes exceeds the > link bandwidth. I end with the eDP panel having no valid modes at all > ==> Panel goes black, game over. > > We either need to revert that commit for drm-fixes, or quirk it for > the specific panels that are troublesome, or need to get some solution > into 5.6-rc, otherwise there will be a lot of regressions for at least > Apple MBP users. > > thanks, > -mario > Ok, just sent out a patch with a specific dpcd quirk for this as: [PATCH] drm/amd/display: Add link_rate quirk for Apple 15" MBP 2017 Tested against drm-next for Linux 5.6, works. -mario _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Some eDP fixes/improvements. 2020-01-09 15:20 Some eDP fixes/improvements Mario Kleiner 2020-01-09 15:20 ` [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read Mario Kleiner 2020-01-09 15:20 ` [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones Mario Kleiner @ 2020-01-11 11:41 ` Timo Aaltonen 2 siblings, 0 replies; 11+ messages in thread From: Timo Aaltonen @ 2020-01-11 11:41 UTC (permalink / raw) To: Mario Kleiner, dri-devel, amd-gfx; +Cc: alexander.deucher, mario.kleiner.de On 9.1.2020 17.20, Mario Kleiner wrote: > Hi and happy new year! > > Since i now have a MBP 2017 to play with, with a 10 bit Retina panel, > and Polaris gpu, i'm trying to get it to get 10 bits, and found one > small bug [fix: patch1], and a quirk of Apples Retina eDP sink, for > which i propose patch2 as solution. I sent a similar patch to i915 to > make 10 bit Retina work with the Intel Kabylake igp on that machine. > > If these make sense, it would be cool to still get them into drm-fixes > for Linux 5.5, so users of spring distro updates like Ubuntu 20.04 LTS > can get a more colorful new year. Just FTR, 20.04 will ship with 5.4 longterm kernel, not 5.5. -- t _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-02-28 21:41 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-09 15:20 Some eDP fixes/improvements Mario Kleiner 2020-01-09 15:20 ` [PATCH 1/2] drm/amd/display: Reorder detect_edp_sink_caps before link settings read Mario Kleiner 2020-01-09 18:40 ` Harry Wentland 2020-01-11 0:17 ` Alex Deucher 2020-01-09 15:20 ` [PATCH 2/2] drm/amd/display: Allow current eDP link settings to override verified ones Mario Kleiner 2020-01-09 18:44 ` Harry Wentland 2020-01-09 21:13 ` Mario Kleiner 2020-01-09 21:26 ` Harry Wentland 2020-02-27 19:11 ` Mario Kleiner 2020-02-28 21:41 ` Mario Kleiner 2020-01-11 11:41 ` Some eDP fixes/improvements Timo Aaltonen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).