Re: [PATCH v8 20/27] drm/i915/display: Remove uncore lock from vlv_atomic_update_fifo

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the `uncore->lock` in `vlv_atomic_update_fifo` introduces a data race on the shared `DSPARB` registers and permits unlocked concurrent MMIO accesses, leading to corrupted hardware FIFO state and potential hardware crashes.
--

commit a9d530199c31de5dd37f0bfe14d6b4c1885aebb4
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915/display: Remove uncore lock from vlv_atomic_update_fifo

This commit removes the uncore lock from vlv_atomic_update_fifo(), with
the assumption that disabling interrupts via intel_pipe_update_start()
is sufficient for synchronization.

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

[ ... ]

> @@ -1892,7 +1891,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	 * intel_pipe_update_start() has already disabled interrupts
>  	 * for us, so a plain spin_lock() is sufficient here.
>  	 */
> -	spin_lock(&uncore->lock);
>  
>  	switch (crtc->pipe) {
>  	case PIPE_A:

[Severity: High]
Will removing the uncore lock here introduce a data race on the shared
DSPARB registers during concurrent updates?

While intel_pipe_update_start() disables local CPU interrupts to prevent
local preemption, non-blocking atomic commits for disjoint sets of CRTCs
can execute concurrently on different CPUs.

If two CPUs commit updates for PIPE_A and PIPE_B simultaneously, could
both CPUs read the same DSPARB register value using the unlocked
intel_de_read_fw() accessors?

For example, this sequence might occur:

CPU1 (updating PIPE_A):
    dsparb = intel_de_read_fw(display, DSPARB(display));

CPU2 (updating PIPE_B):
    dsparb = intel_de_read_fw(display, DSPARB(display));

When both CPUs modify their respective bits and write the value back, one
CPU could overwrite the other's modifications. This would silently erase
the FIFO split configuration for the other pipe and corrupt the hardware
FIFO state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201846.307297-1-dev@lankhorst.se?part=20