Re: [PATCH] drm/i915/dp: Remove redundant link capabilities reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/i915/dp: missing link capabilities reset in intel_dp_reset_link_params()
- [Low] drm/i915/dp: unused function intel_dp_link_caps_reset() left behind
- [Low] drm/i915/dp: space indentation used in intel_dp_reset_link_params()
--

commit a8d5bf6dc42f9bcd49f6371d597ac7f839e9b04f
Author: George Karagounis <taterraster@gmail.com>

drm/i915/dp: Remove redundant link capabilities reset

This commit removes the intel_dp_link_caps_reset() call from
intel_dp_reset_link_params(), assuming that intel_dp_link_caps_update() with
reset=true has already been invoked beforehand.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d8c0d9381471d..0e753824bc281 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3674,17 +3674,10 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  
>  void intel_dp_reset_link_params(struct intel_dp *intel_dp)
>  {
> -	/*
> -	 * TODO: Remove the following reset of link capabilities, as
> -	 * this isn't needed after intel_dp_link_caps_update(reset=true)
> -	 * was called.
> -	 */
> -	intel_dp_link_caps_reset(intel_dp->link.caps);

[Severity: High]
Are all callers of intel_dp_reset_link_params() guaranteed to have called
intel_dp_link_caps_update(reset=true) beforehand?

Several paths call intel_dp_reset_link_params() without invoking
intel_dp_link_caps_update():

1. In drivers/gpu/drm/i915/display/intel_dp_link_caps.c, the debugfs handlers
i915_dp_force_link_rate_write() and i915_dp_force_lane_count_write() call
intel_dp_reset_link_params() directly before setting forced_params:

	intel_dp_flush_connector_commits(connector);

	intel_dp_reset_link_params(intel_dp);
	link_caps->forced_params.rate = rate;

2. In drivers/gpu/drm/i915/display/intel_dp.c, when intel_dp_detect() runs on
an eDP connector after intel_dp->reset_link_params is set (such as on
suspend/resume, a long HPD pulse in intel_dp_hpd_pulse(), or RX_CAP_CHANGED in
intel_dp_handle_link_service_irq()), it takes the edp_detect() branch rather
than intel_dp_detect_dpcd():

	if (intel_dp_is_edp(intel_dp))
		status = edp_detect(intel_dp);
	else if (intel_digital_port_connected(encoder))
		status = intel_dp_detect_dpcd(intel_dp);
	...
	if (intel_dp->reset_link_params) {
		intel_dp_reset_link_params(intel_dp);
		intel_dp->reset_link_params = false;
	}

Because intel_dp_get_dpcd() also skips intel_dp_update_sink_caps() for eDP,
intel_dp_link_caps_update() is never called on this path.

3. In drivers/gpu/drm/i915/display/intel_dp.c, intel_dp_sync_state() skips
intel_dp_get_dpcd() during system resume when intel_dp->dpcd[DP_DPCD_REV] != 0
(and on eDP) and calls intel_dp_reset_link_params() directly:

	if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) {
		intel_dp_get_dpcd(intel_dp);
		dpcd_updated = true;
	}

	intel_dp_tunnel_resume(intel_dp, crtc_state, dpcd_updated);

	if (crtc_state) {
		intel_dp_reset_link_params(intel_dp);
		intel_dp_set_link_params(intel_dp, crtc_state->port_clock, crtc_state->lane_count);
		intel_dp->link.active = true;
	}

Without intel_dp_link_caps_reset() here, won't reduced link_caps->max_limits
and disabled configurations in link_caps->enabled_configs from earlier link
training fallbacks remain stuck on these paths?

[Severity: Low]
Also, since this was the only caller of intel_dp_link_caps_reset(), removing
this call leaves the function definition in
drivers/gpu/drm/i915/display/intel_dp_link_caps.c and its declaration in
drivers/gpu/drm/i915/display/intel_dp_link_caps.h unused:

void intel_dp_link_caps_reset(struct intel_dp_link_caps *link_caps)
{
	reset_max_link_limits_reenable_all(link_caps);
}

If intel_dp_link_caps_reset() were no longer needed here, should its
definition and declaration be removed as well?

> -	intel_dp_tunnel_uhbr_lanes_wa_apply(intel_dp);
> -
> -	intel_dp->link.mst_probed_lane_count = 0;
> -	intel_dp->link.mst_probed_rate = 0;
> -	intel_dp_link_training_reset(intel_dp->link.training);
> +       intel_dp_tunnel_uhbr_lanes_wa_apply(intel_dp);
> +       intel_dp->link.mst_probed_lane_count = 0;
> +       intel_dp->link.mst_probed_rate = 0;
> +       intel_dp_link_training_reset(intel_dp->link.training);

[Severity: Low]
This isn't a bug, but the replacement lines in intel_dp_reset_link_params()
replace the leading tab indentation with spaces. Could these lines keep
standard tab indentation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025113200.25097-1-mail@taterr.org?part=1