| Message ID | 20200229054108.2781-1-mario.kleiner.de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Add dpcd link_rate quirk for Apple 15" MBP 2017 | expand |
On Sat, 29 Feb 2020, Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > 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 = multiplier 0xc), > but the DP_MAX_LINK_RATE dpcd register only reports > 2.7 Gbps (multiplier value 0xa) as possible, in direct > contradiction of what the firmware successfully set up. > > This restricts the panel to 8 bpc, not providing the full > color depth of the panel. > > This patch adds a quirk specific to the MBP 2017 15" Retina > panel to add the additiional 324000 kbps link rate during > edp setup. > > Link to previous discussion of a different attempted fix > with Ville and Jani: > > https://patchwork.kernel.org/patch/11325935/ > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 2 ++ > drivers/gpu/drm/i915/display/intel_dp.c | 7 +++++++ > include/drm/drm_dp_helper.h | 7 +++++++ > 3 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 5a103e9b3c86..36a371c016cb 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -1179,6 +1179,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = { > { OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'), false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) }, > /* Synaptics DP1.4 MST hubs can support DSC without virtual DPCD */ > { OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) }, > + /* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */ > + { OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) }, > }; > > #undef OUI > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 4074d83b1a5f..1f6bd659ad41 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -178,6 +178,13 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > } > > intel_dp->num_sink_rates = i; > + > + if (drm_dp_has_quirk(&intel_dp->desc, > + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS)) { > + /* Needed for Apple MBP 2017, 15 inch eDP Retina panel */ > + intel_dp->sink_rates[i] = 324000; > + intel_dp->num_sink_rates++; > + } If we can isolate the quirk to this one function, I'll be happy. \o/ However, even if this might work on said machine, I'd prefer it if we didn't give the idea that you could just append a value in sink_rates (it must be sorted). How about putting something like this in the beginning of the function, to be a bit more explicit: if (quirk) { static const int quirk_rates[] = { 162000, 270000, 324000 }; memcpy(intel_dp->sink_rates, quirk_rates, sizeof(quirk_rates)); intel_dp->num_sink_rates = ARRAY_SIZE(quirk_rates); return; } BR, Jani. > } > > /* Get length of rates array potentially limited by max_rate. */ > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 262faf9e5e94..4b86a1f2a559 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -1532,6 +1532,13 @@ enum drm_dp_quirk { > * The DSC caps can be read from the physical aux instead. > */ > DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD, > + /** > + * @DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS: > + * > + * The device supports a link rate of 3.24 Gbps (multiplier 0xc) despite > + * the DP_MAX_LINK_RATE register reporting a lower max multiplier. > + */ > + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS, > }; > > /**
On Wed, Mar 4, 2020 at 4:32 PM Jani Nikula <jani.nikula@intel.com> wrote: > > On Sat, 29 Feb 2020, Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > > 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 = multiplier 0xc), > > but the DP_MAX_LINK_RATE dpcd register only reports > > 2.7 Gbps (multiplier value 0xa) as possible, in direct > > contradiction of what the firmware successfully set up. > > > > This restricts the panel to 8 bpc, not providing the full > > color depth of the panel. > > > > This patch adds a quirk specific to the MBP 2017 15" Retina > > panel to add the additiional 324000 kbps link rate during > > edp setup. > > > > Link to previous discussion of a different attempted fix > > with Ville and Jani: > > > > https://patchwork.kernel.org/patch/11325935/ > > > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/drm_dp_helper.c | 2 ++ > > drivers/gpu/drm/i915/display/intel_dp.c | 7 +++++++ > > include/drm/drm_dp_helper.h | 7 +++++++ > > 3 files changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 5a103e9b3c86..36a371c016cb 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -1179,6 +1179,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = { > > { OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'), false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) }, > > /* Synaptics DP1.4 MST hubs can support DSC without virtual DPCD */ > > { OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) }, > > + /* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */ > > + { OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) }, > > }; > > > > #undef OUI > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 4074d83b1a5f..1f6bd659ad41 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -178,6 +178,13 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > > } > > > > intel_dp->num_sink_rates = i; > > + > > + if (drm_dp_has_quirk(&intel_dp->desc, > > + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS)) { > > + /* Needed for Apple MBP 2017, 15 inch eDP Retina panel */ > > + intel_dp->sink_rates[i] = 324000; > > + intel_dp->num_sink_rates++; > > + } > > If we can isolate the quirk to this one function, I'll be happy. \o/ > Me too \o/ - Patch v2 is out, following your proposal, retested on the machine, works. cat ... i915_display_info reports a pipe depth of 30 bpp, instead of 24 bpp. I didn't add a stable tag, but wonder if a cc stable tag could be added by you, if you think it is minimal enough, to get it also into the kernels for the spring distro updates. In any case, case closed. Thanks for the review, -mario > However, even if this might work on said machine, I'd prefer it if we > didn't give the idea that you could just append a value in sink_rates > (it must be sorted). How about putting something like this in the > beginning of the function, to be a bit more explicit: > > if (quirk) { > static const int quirk_rates[] = { 162000, 270000, 324000 }; > > memcpy(intel_dp->sink_rates, quirk_rates, sizeof(quirk_rates)); > intel_dp->num_sink_rates = ARRAY_SIZE(quirk_rates); > > return; > } > > BR, > Jani. > > > } > > > > /* Get length of rates array potentially limited by max_rate. */ > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index 262faf9e5e94..4b86a1f2a559 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -1532,6 +1532,13 @@ enum drm_dp_quirk { > > * The DSC caps can be read from the physical aux instead. > > */ > > DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD, > > + /** > > + * @DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS: > > + * > > + * The device supports a link rate of 3.24 Gbps (multiplier 0xc) despite > > + * the DP_MAX_LINK_RATE register reporting a lower max multiplier. > > + */ > > + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS, > > }; > > > > /** > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 5a103e9b3c86..36a371c016cb 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1179,6 +1179,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = { { OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'), false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) }, /* Synaptics DP1.4 MST hubs can support DSC without virtual DPCD */ { OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) }, + /* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */ + { OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) }, }; #undef OUI diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 4074d83b1a5f..1f6bd659ad41 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -178,6 +178,13 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) } intel_dp->num_sink_rates = i; + + if (drm_dp_has_quirk(&intel_dp->desc, + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS)) { + /* Needed for Apple MBP 2017, 15 inch eDP Retina panel */ + intel_dp->sink_rates[i] = 324000; + intel_dp->num_sink_rates++; + } } /* Get length of rates array potentially limited by max_rate. */ diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 262faf9e5e94..4b86a1f2a559 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1532,6 +1532,13 @@ enum drm_dp_quirk { * The DSC caps can be read from the physical aux instead. */ DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD, + /** + * @DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS: + * + * The device supports a link rate of 3.24 Gbps (multiplier 0xc) despite + * the DP_MAX_LINK_RATE register reporting a lower max multiplier. + */ + DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS, }; /**
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 = multiplier 0xc), but the DP_MAX_LINK_RATE dpcd register only reports 2.7 Gbps (multiplier value 0xa) as possible, in direct contradiction of what the firmware successfully set up. This restricts the panel to 8 bpc, not providing the full color depth of the panel. This patch adds a quirk specific to the MBP 2017 15" Retina panel to add the additiional 324000 kbps link rate during edp setup. Link to previous discussion of a different attempted fix with Ville and Jani: https://patchwork.kernel.org/patch/11325935/ Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_dp_helper.c | 2 ++ drivers/gpu/drm/i915/display/intel_dp.c | 7 +++++++ include/drm/drm_dp_helper.h | 7 +++++++ 3 files changed, 16 insertions(+)