Re: [v3 4/4] drm/i915/display: Gate periodic AS SDP skip frames behind a module parameter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Time-Of-Check to Time-Of-Use (TOCTOU) race condition due to evaluating a writable module parameter (`periodic_assdp_enable`) dynamically during both the `atomic_check` and `atomic_commit` phases.
--

commit c6beee35e1043bf1a9c47428a8b14e66a470f378
Author: Uma Shankar <uma.shankar@intel.com>

drm/i915/display: Gate periodic AS SDP skip frames behind a module parameter

This commit adds a periodic_assdp_enable module parameter to optionally enable
periodic AS SDP (skip frames) for Panel Replay. The feature is disabled by
default and gated dynamically inside intel_pr_as_sdp_skip_frames().

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c

[ ... ]

> @@ -392,11 +392,16 @@ static u32 get_pr_alpm_as_sdp_transmission_time(const struct intel_crtc_state *c
>   */
>  static u32 intel_pr_as_sdp_skip_frames(struct intel_dp *intel_dp)
>  {
> +	struct intel_display *display = to_intel_display(intel_dp);
>  	const struct drm_display_info *info =
>  		&intel_dp->attached_connector->base.display_info;
>  	int max_vrefresh = info->monitor_range.max_vfreq;
>  	int min_vrefresh = info->monitor_range.min_vfreq;
>  
> +	/* Off by default; gated by the periodic_assdp_enable module parameter. */
> +	if (!display->params.periodic_assdp_enable)
> +		return 0;

[Severity: Medium]
Since the module parameter is writable at runtime (0600), could reading it
dynamically here introduce a race condition between the atomic_check and
atomic_commit phases?

The commit message notes this function serves as a choke point for both the
DC3co force-disable predicate (evaluated during check phase in 
intel_display_power_dc3co_compute()) and PR_ALPM_CTL programming (evaluated
during commit phase in intel_alpm_configure_pr_as_sdp()).

If a user with root privileges changes the sysfs parameter from 0 to 1 after
the check phase completes but before hardware programming begins, could
the driver program the hardware inconsistently? For example, by keeping DC3co
enabled but also enabling AS SDP skip frames, which are documented as
incompatible?

Should this parameter be read once during the atomic check phase and cached
in the crtc_state to guarantee consistency across the DRM atomic pipeline?

> +
>  	if (min_vrefresh <= 0 || max_vrefresh <= min_vrefresh)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c

[ ... ]

> @@ -139,6 +139,12 @@ intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>  	"(-1=use per-chip default, 0=disabled, 1=enabled, 2=match any register, 3=always locked) "
>  	"Default: -1");
>  
> +intel_display_param_named_unsafe(periodic_assdp_enable, bool, 0600,
> +	"Enable periodic Adaptive-Sync SDP (AS SDP skip frames) for Panel Replay "
> +	"to drive the panel down to its minimum refresh rate "
> +	"(0=disabled, 1=enabled) "
> +	"Default: 0");

[Severity: Medium]
Is it intentional to expose this as a writable parameter with 0600 
permissions given the potential time-of-check to time-of-use issues 
mentioned above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831125914.1736501-1-uma.shankar@intel.com?part=4