Re: [PATCH 2/2] drm/dp: Add quirk for panel with HBR3 without TPS4

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 14 May 2025, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> For DP, TPS4 is a requirement for supporting HBR3, but for eDP its a
> bit ambiguous. Some broken eDP sinks declare support for HBR3 without
> TPS4, but are unable to produce a stable output. For these panels
> add a quirk to reject HBR3 rate if TPS4 is not supported.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c |  2 +
>  drivers/gpu/drm/i915/display/intel_dp.c | 74 ++++++++++++++++++++++---
>  include/drm/display/drm_dp_helper.h     |  8 +++
>  3 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 56c7e3318f01..6f849146dd98 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2519,6 +2519,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
>  	/* 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) },
> +	/* Novatek panel */
> +	{ OUI(0x38, 0xEC, 0x11), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBR3_WITHOUT_TPS4) },
>  };
>  
>  #undef OUI
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 97cf80372264..6c5debc8310d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -173,12 +173,53 @@ int intel_dp_link_symbol_clock(int rate)
>  	return DIV_ROUND_CLOSEST(rate * 10, intel_dp_link_symbol_size(rate));
>  }
>  
> +static bool detected_hbr3_tps4_quirk(struct intel_dp *intel_dp)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct drm_dp_aux *aux = &intel_dp->aux;
> +	struct drm_dp_desc desc;
> +
> +	if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(intel_dp->dpcd)) < 0)
> +		return false;
> +
> +	if (!drm_dp_has_quirk(&desc, DP_DPCD_QUIRK_HBR3_WITHOUT_TPS4))
> +		return false;
> +
> +	drm_dbg_kms(display->drm,
> +		    "[CONNECTOR:%d:%s] HBR3 without TPS4 quirk detected\n",
> +		    connector->base.base.id, connector->base.name);
> +
> +	return true;
> +}

This function is unnecessary. It's just
drm_dp_has_quirk(&intel_dp->desc, ...) and that's it.

You already have the desc, no need to read it again, and the debug
logging is kind of excessive.

(We might want to add generic quirk detection debug logging in
drm_dp_get_quirks() but that's another matter.)

> +
>  static int max_dprx_rate(struct intel_dp *intel_dp)
>  {
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	int max_rate;
> +
>  	if (intel_dp_tunnel_bw_alloc_is_enabled(intel_dp))
> -		return drm_dp_tunnel_max_dprx_rate(intel_dp->tunnel);
> +		max_rate = drm_dp_tunnel_max_dprx_rate(intel_dp->tunnel);
> +	else
> +		max_rate = drm_dp_bw_code_to_link_rate(intel_dp->dpcd[DP_MAX_LINK_RATE]);
>  
> -	return drm_dp_bw_code_to_link_rate(intel_dp->dpcd[DP_MAX_LINK_RATE]);
> +	/*
> +	 * For DP TPS4 is a requirement for supporting HBR3, but for eDP its a
> +	 * bit ambiguous. Some broken eDP sinks declare support for HBR3 without
> +	 * TPS4, but are unable to produce a stable output. For these panels
> +	 * reject HBR3 when TPS4 is not available.
> +	 */
> +	if (max_rate >= 810000 &&
> +	    !drm_dp_tps4_supported(intel_dp->dpcd) &&
> +	    detected_hbr3_tps4_quirk(intel_dp)) {
> +		drm_dbg_kms(display->drm,
> +			    "[ENCODER:%d:%s] Rejecting HBR3 due to missing TPS4 support\n",
> +			    encoder->base.base.id, encoder->base.name);
> +		max_rate = 540000;
> +	}

All of this would be greatly simplified if you made the quirk just "max
rate is hbr2". The way this is defined now is complicated by the TPS4
stuff, and that's pretty much irrelevant for what the quirk actually
does, i.e. limits the max rate to 540000.

> +
> +	return max_rate;
>  }
>  
>  static int max_dprx_lane_count(struct intel_dp *intel_dp)
> @@ -4254,6 +4295,9 @@ static void intel_edp_mso_init(struct intel_dp *intel_dp)
>  static void
>  intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  {
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +
>  	intel_dp->num_sink_rates = 0;
>  
>  	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
> @@ -4264,10 +4308,7 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  				 sink_rates, sizeof(sink_rates));
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> -			int val = le16_to_cpu(sink_rates[i]);
> -
> -			if (val == 0)
> -				break;
> +			int rate;
>  
>  			/* Value read multiplied by 200kHz gives the per-lane
>  			 * link rate in kHz. The source rates are, however,
> @@ -4275,7 +4316,26 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  			 * back to symbols is
>  			 * (val * 200kHz)*(8/10 ch. encoding)*(1/8 bit to Byte)
>  			 */
> -			intel_dp->sink_rates[i] = (val * 200) / 10;
> +			rate = le16_to_cpu(sink_rates[i]) * 200 / 10;
> +
> +			if (rate == 0)
> +				break;
> +			/*
> +			 * For DP TPS4 is a requirement for supporting HBR3, but for eDP its a
> +			 * bit ambiugous. Some broken eDP sinks declare support for HBR3 without
> +			 * TPS4, but are unable to produce a stable output. For these panels
> +			 * reject HBR3 when TPS4 is not available.
> +			 */
> +			if (rate >= 810000 &&
> +			    !drm_dp_tps4_supported(intel_dp->dpcd) &&
> +			    detected_hbr3_tps4_quirk(intel_dp)) {
> +				drm_dbg_kms(display->drm,
> +					    "[ENCODER:%d:%s] Rejecting HBR3 due to missing TPS4 support\n",
> +					    encoder->base.base.id, encoder->base.name);
> +				break;
> +			}

Ditto.

> +
> +			intel_dp->sink_rates[i] = rate;
>  		}
>  		intel_dp->num_sink_rates = i;
>  	}
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 7b19192c7031..8021e9db67f2 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -809,6 +809,14 @@ enum drm_dp_quirk {
>  	 * requires enabling DSC.
>  	 */
>  	DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC,
> +
> +	/**
> +	 * @DP_DPCD_QUIRK_HBR3_WITHOUT_TPS4:
> +	 *
> +	 * The device supports HBR3 without TPS4 but is unable to produce
> +	 * stable output.
> +	 */
> +	DP_DPCD_QUIRK_HBR3_WITHOUT_TPS4,

Ditto.

>  };
>  
>  /**

-- 
Jani Nikula, Intel