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

[Jani Nikula]

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/

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