Re: [PATCH v2 40/56] drm/i915/ddi: Wire FRL into the HDMI DDI enable/disable path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/i915/hdmi: connector refcount underflow via modeset_retry_work
- [Medium] drm/i915/hdmi: intel_hdmi_frl_cfg_write corrupts register by omitting clear mask
- [Medium] drm/i915: unconditional FRL configuration and transcoder enablement contrary to commit message
--

commit cfc5e8ca998b654bfa60f35c76733a4d8332204f
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Subject: drm/i915/ddi: Wire FRL into the HDMI DDI enable/disable path

This commit enables HDMI FRL in the DDI enable and disable sequences for
MTL+. It prepares for FRL mode, starts FRL link training, and configures
the FRL registers if enabled. On disable, it handles LTS:L teardown and
register cleanup.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 53e889c0d0223..22a49eb50b203 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
[ ... ]
> @@ -3605,8 +3614,15 @@ static void intel_ddi_enable_hdmi(struct intel_atomic_state *state,
>  
>  	intel_ddi_buf_enable(encoder, buf_ctl);
>  
> -	if (DISPLAY_VER(display) >= 14)
> +	if (DISPLAY_VER(display) >= 14) {
> +		if (crtc_state->frl.enable &&
> +		    intel_hdmi_start_frl(encoder, crtc_state) < 0)
> +			schedule_work(&intel_connector->modeset_retry_work);

[Severity: High]
Does this code underflow the connector reference count?

If intel_hdmi_start_frl() fails, schedule_work() is called directly without
taking a reference via drm_connector_get(). If the driver suspends, unbinds,
or calls intel_hpd_cancel_work(), it calls
intel_connector_cancel_modeset_retry_work(). This generic cancel helper
invokes cancel_work_sync() and unconditionally calls drm_connector_put() if
the work was pending. Because the work was queued without taking a reference,
this drops an unowned reference.

> +
> +		intel_hdmi_frl_cfg_write(crtc_state);

[Severity: Medium]
Does intel_hdmi_frl_cfg_write() corrupt the register by omitting a clear mask?

When this function is called here, it performs a read-modify-write on the
TRANS_HDMI_FRL_CFG register:

intel_hdmi.c:intel_hdmi_frl_cfg_write() {
    ...
    val = TRANS_HDMI_R_B_SCHED_ENABLE(crtc_state->frl.rsrc_sched_en);
    val |= TRANS_HDMI_ACTIVE_CHAR_BUF_THRESH(crtc_state->frl.active_char_buf_threshold);
    val |= TRANS_HDMI_MIN_BLANK_CHAR(TRANS_HDMI_MIN_BLANK_CHAR_VAL);

    intel_de_rmw(display, TRANS_HDMI_FRL_CFG(display, cpu_trans), 0, val);
}

Because the clear mask is 0, intel_de_rmw() performs a read, leaves all
existing bits intact, and then bitwise ORs the new val. This could
permanently corrupt the hardware configuration if the previous register
value had bits set that should be zero in the new configuration.

[Severity: Medium]
Does this execute hardware configuration unconditionally despite failures?

The commit message explicitly states: "If FRL link training fails, notify
userspace. On success, write DFM registers and enable the transcoder."
However, the code unconditionally calls intel_hdmi_frl_cfg_write(),
intel_hdmi_frl_dfm_write(), and intel_ddi_enable_transcoder_and_vblank()
even if intel_hdmi_start_frl() returns an error. While this might be
architecturally consistent with how DisplayPort handles link training
failures, is there a discrepancy between the documented intent and the
actual implementation?

> +		intel_hdmi_frl_dfm_write(crtc_state);
>  		intel_ddi_enable_transcoder_and_vblank(state, encoder, crtc_state);
> +	}
>  
>  	intel_hdmi_poll_for_scrambling_enable(crtc_state, connector);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012035806.3361439-1-ankit.k.nautiyal@intel.com?part=40