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 <[email protected]> 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/[email protected]?part=1
