Re: [PATCH v4 1/8] drm: Move sink link rates reading helpers out of i915

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/dp: HBR2 quirk in intel_edp_set_sink_rates() skips fallback
--

commit 0b240b52afdb3eb8ae0e254bde919dbb5abd0487
Author: Kory Maincent <kory.maincent@bootlin.com>

drm: Move sink link rates reading helpers out of i915

This commit moves the DisplayPort and eDP sink link rate reading helpers out
of the i915 driver into generic display helpers in drm_dp_helper.c, while
keeping the Intel-specific rate limit quirk handling in intel_dp.c.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0cd5e6b5034cf..087afcea5395f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c

[ ... ]

> @@ -4722,60 +4619,35 @@ static void
>  intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> +	int num_sink_rates;
>  
> -	intel_dp->num_sink_rates = 0;
> +	num_sink_rates = drm_dp_read_edp_sink_rates(&intel_dp->aux,
> +						    intel_dp->edp_dpcd[0],
> +						    intel_dp->sink_rates);
> +	if (num_sink_rates < 0)
> +		num_sink_rates = 0;
>  
> -	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
> -		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> -		int ret;
> -		int i;
> -
> -		ret = drm_dp_dpcd_read_data(&intel_dp->aux,
> -					    DP_SUPPORTED_LINK_RATES,
> -					    sink_rates, sizeof(sink_rates));
> -		if (ret < 0) {
> -			drm_dbg_kms(display->drm,
> -				    "Unable to read eDP supported link rates, using default rates\n");
> -			memset(sink_rates, 0, sizeof(sink_rates));
> -		}
> -
> -		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> -			int rate;
> -
> -			/* Value read multiplied by 200kHz gives the per-lane
> -			 * link rate in kHz. The source rates are, however,
> -			 * stored in terms of LS_Clk kHz. The full conversion
> -			 * back to symbols is
> -			 * (val * 200kHz)*(8/10 ch. encoding)*(1/8 bit to Byte)
> -			 */
> -			rate = le16_to_cpu(sink_rates[i]) * 200 / 10;
> -
> -			if (rate == 0)
> -				break;
> -
> -			/*
> -			 * Some platforms cannot reliably drive HBR3 rates due to PHY limitations,
> -			 * even if the sink advertises support. Reject any sink rates above HBR2 on
> -			 * the known machines for stable output.
> -			 */
> -			if (rate > 540000 &&
> -			    intel_has_quirk(display, QUIRK_EDP_LIMIT_RATE_HBR2))
> -				break;
> -
> -			intel_dp->sink_rates[i] = rate;
> -		}
> -		intel_dp->num_sink_rates = i;
> -	}
> +	intel_dp->num_sink_rates = num_sink_rates;
>  
>  	/*
>  	 * Use DP_LINK_RATE_SET if DP_SUPPORTED_LINK_RATES are available,
>  	 * default to DP_MAX_LINK_RATE and DP_LINK_BW_SET otherwise.
>  	 */
> -	if (intel_dp->num_sink_rates)
> +	if (num_sink_rates)
>  		intel_dp->use_rate_select = true;
>  	else
>  		intel_dp_set_sink_rates(intel_dp);
>  
> +	/*
> +	 * Some platforms cannot reliably drive HBR3 rates due to PHY limitations,
> +	 * even if the sink advertises support. Reject any sink rates above HBR2 on
> +	 * the known machines for stable output.
> +	 */
> +	if (intel_dp->num_sink_rates && intel_has_quirk(display, QUIRK_EDP_LIMIT_RATE_HBR2))
> +		intel_dp->num_sink_rates = intel_dp_rate_limit_len(intel_dp->sink_rates,
> +								   intel_dp->num_sink_rates,
> +								   540000);
> +
>  	intel_edp_set_data_override_rates(intel_dp);
>  }

[Severity: Medium]
In drivers/gpu/drm/i915/display/intel_dp.c, does moving the hbr2 rate limit
check in intel_edp_set_sink_rates() after the num_sink_rates check skip the
fallback to intel_dp_set_sink_rates() when all rates read by
drm_dp_read_edp_sink_rates() exceed 540000 kHz?

Previously, intel_edp_set_sink_rates() rejected rates above 540000 kHz inside
the loop before checking if intel_dp->num_sink_rates was non-zero:

    if (rate > 540000 &&
        intel_has_quirk(display, QUIRK_EDP_LIMIT_RATE_HBR2))
            break;

If all advertised rates in that table exceeded 540000 kHz,
intel_dp->num_sink_rates remained 0, intel_dp->use_rate_select stayed false,
and execution fell back to intel_dp_set_sink_rates() where max_dprx_rate()
capped the maximum link rate to 540000 kHz.

With this change, drm_dp_read_edp_sink_rates() returns the unfiltered count,
so num_sink_rates is non-zero, intel_dp->use_rate_select is set to true, and
intel_dp_set_sink_rates() is skipped. When intel_dp_rate_limit_len() then
filters out the rates above 540000 kHz, intel_dp->num_sink_rates becomes 0
while intel_dp->use_rate_select remains true.

During connector initialization via intel_dp_init_connector() ->
intel_edp_init_connector() -> intel_edp_init_dpcd() ->
intel_edp_set_sink_rates(), the subsequent call to
intel_dp_set_common_link_params() -> intel_dp_get_common_rates() will then
hit both warnings and force common_rates[0] = 162000:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_get_common_rates() {
    drm_WARN_ON(display->drm,
                !intel_dp->num_source_rates || !intel_dp->num_sink_rates);

    *num_common_rates = intersect_rates(intel_dp->source_rates,
                                        intel_dp->num_source_rates,
                                        intel_dp->sink_rates,
                                        intel_dp->num_sink_rates,
                                        common_rates);

    /* Paranoia, there should always be something in common. */
    if (drm_WARN_ON(display->drm, *num_common_rates == 0)) {
            common_rates[0] = 162000;
            *num_common_rates = 1;
    }
}

And during modeset link computation via intel_dp_compute_rate() ->
intel_dp_rate_select(), because intel_dp->use_rate_select is true while
intel_dp->num_sink_rates is 0, intel_dp_rate_select() will fail to find the
rate, warn, and return index 0:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_rate_select() {
    int i = intel_dp_rate_index(intel_dp->sink_rates,
                                intel_dp->num_sink_rates, rate);

    if (drm_WARN_ON(display->drm, i < 0))
            i = 0;

    return i;
}

Should intel_dp_rate_limit_len() be applied to num_sink_rates before checking
if (num_sink_rates), and applied again if it falls back to
intel_dp_set_sink_rates()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-feat_link_cap-v4-0-a4f3f6e8cad9@bootlin.com?part=1