[PATCH] drm/i915/dp: Add dpcd link_rate quirk for Apple 15" MBP 2017

[Mario Kleiner]

On Wed, Mar 4, 2020 at 4:32 PM Jani Nikula <jani.nikula at intel.com> wrote:
>
> On Sat, 29 Feb 2020, Mario Kleiner <mario.kleiner.de at 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 at gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at 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