Re: [PATCH v2 46/56] drm/i915/display: Add FRL members to the state checker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/i915/display: Incomplete FRL state verification missing tb_threshold_min and required_rate
- [Medium] drm/i915: spurious WARN_ON in state checker due to uncleared FRL config bits
--

commit 5c7a8a2a5c09104f1fb54f2b4082592bd27316bb
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/i915/display: Add FRL members to the state checker

This commit introduces hardware state verification for FRL (Fixed Rate Link)
by adding several FRL configuration members to the state checker.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5626,6 +5626,14 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  
>  	PIPE_CONF_CHECK_I(set_context_latency);
>  
> +	PIPE_CONF_CHECK_BOOL(frl.enable);
> +	PIPE_CONF_CHECK_BOOL(frl.rsrc_sched_en);
> +	PIPE_CONF_CHECK_I(frl.required_lanes);
> +	PIPE_CONF_CHECK_I(frl.tb_actual);
> +	PIPE_CONF_CHECK_I(frl.active_char_buf_threshold);

[Severity: Medium]
Will checking frl.rsrc_sched_en and frl.active_char_buf_threshold trigger
spurious WARN_ON state mismatches due to uncleared hardware bits?

Looking at intel_hdmi_frl_cfg_write() in intel_hdmi.c, it appears to use
intel_de_rmw() with a clear mask of 0:

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);
    ...
    intel_de_rmw(display, TRANS_HDMI_FRL_CFG(display, cpu_trans), 0, val);
}

Since the clear mask is 0, preexisting bits from BIOS or prior configurations
will persist in the hardware register. Will the state checker trip over these
leftover bits when it compares them against the software state?

> +	PIPE_CONF_CHECK_I(frl.link_m);
> +	PIPE_CONF_CHECK_I(frl.link_n);

[Severity: Medium]
Should frl.tb_threshold_min and frl.required_rate also be verified here?

The driver actively populates tb_threshold_min from hardware during state
readout in intel_hdmi_frl_dfm_read():

intel_hdmi_frl_dfm.c:intel_hdmi_frl_dfm_read() {
    ...
    val = intel_de_read(display, TRANS_HDMI_FRL_DFMTHRSH(display, cpu_trans));
    crtc_state->frl.tb_threshold_min = REG_FIELD_GET(TB_MIN_THESHOLD_MASK, val);
    ...
}

Similarly, frl.required_rate is populated during hardware readout in
intel_hdmi_frl_get_config(). Without adding these fields to the state checker,
can hardware state corruption or mismatches for the minimum threshold and
required rate go undetected?

> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_LLI

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