Re: [PATCH 3/6] drm/i915/cursor: Handle secondary cursor state

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The fast path cursor update fails to iterate over all joined pipes when unpinning their old framebuffers, leaking VMA pins and triggering use-after-free.
- [High] The fast path cursor update fails to write the new plane state to the hardware registers of secondary joined cursors.
- [Medium] The secondary cursor plane's uapi.crtc is incorrectly assigned to the secondary CRTC, breaking bigjoiner atomic state invariants.
--

--- Patch [3]: [PATCH 3/6] drm/i915/cursor: Handle secondary cursor state ---
commit 98f6f039bea454d797d8103020e7553cf275702d
Author: Nemesa Garg <[email protected]>

drm/i915/cursor: Handle secondary cursor state

This commit modifies the fast path cursor update to handle secondary pipes
in joiner mode. It iterates over all joined pipes to duplicate and prepare
the plane states, tracking them in a joined_pipe_state array.

> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
[ ... ]
> @@ -928,38 +938,70 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
[ ... ]
> +	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
> +					 intel_crtc_joined_pipe_mask(crtc_state)) {
> +		struct intel_cursor_joiner_state *j = &joined_pipe_state[num_pipes];
> +
> +		j->plane = intel_crtc_get_plane(pipe_crtc, PLANE_CURSOR);
[ ... ]
> +		j->new_plane_state->uapi.crtc = &pipe_crtc->base;

[Severity: Medium]
Does this assignment break the bigjoiner atomic state invariants?

The DRM framework requires that for joined pipes, a secondary plane's
uapi.crtc must point to the primary CRTC. This represents the single
logical CRTC to userspace, while only its hw.crtc points to the
secondary CRTC.

Assigning the secondary CRTC to the uapi.crtc field of secondary planes
could corrupt the atomic state tracker and lead to failures during
subsequent atomic commits.

Should this point to the primary CRTC instead?

> +
> +		intel_cursor_fastpath_update_plane_state(j->new_plane_state, fb,
[ ... ]
>  	/*
>  	 * We cannot swap crtc_state as it may be in use by an atomic commit or
>  	 * page flip that's running simultaneously. If we swap crtc_state and
>  	 * destroy the old state, we will cause a use-after-free there.
>  	 *
>  	 * Only update active_planes, which is needed for our internal
>  	 * bookkeeping. Either value will do the right thing when updating
>  	 * planes atomically. If the cursor was part of the atomic update then
>  	 * we would have taken the slowpath.
>  	 */
> -	crtc_state->active_planes = new_crtc_state->active_planes;
>  
>  	intel_vblank_evade_init(crtc_state, crtc_state, &evade);
>  
>  	intel_psr_lock(crtc_state);

[Severity: High]
Are the hardware updates properly applied to the secondary cursor planes?

The unpatched hardware update block immediately following this code only calls
update functions for the primary plane:

    if (new_plane_state->uapi.visible) {
        intel_plane_update_noarm(NULL, plane, crtc_state, new_plane_state);
        intel_plane_update_arm(NULL, plane, crtc_state, new_plane_state);
    } else {
        intel_plane_disable_arm(NULL, plane, crtc_state);
    }

Because the fast path now handles joined pipes and populates the
joined_pipe_state array, failing to write the new plane state to the
hardware registers of the secondary joined cursors means the cursor will
not visually update on the secondary half of the display.

Could this block be updated to iterate over all pipes in joined_pipe_state?

> @@ -1004,6 +1045,10 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  
>  	intel_psr_unlock(crtc_state);
>  
> +	/*
> +	 * Schedule or immediately unpin old framebuffers.
> +	 * Protect against concurrent access.
> +	 */
>  	if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) {
>  		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
>  				     intel_cursor_unpin_work);
> @@ -1012,18 +1057,24 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
>  					 false);
>  
> -		old_plane_state = NULL;
> +		joined_pipe_state[0].old_plane_state = NULL;
>  	} else {
>  		intel_plane_unpin_fb(old_plane_state);
>  	}

[Severity: High]
Can this unpin path leak VMA pins and trigger a use-after-free for
secondary framebuffers?

This code explicitly unpins only the primary pipe's old state
(old_plane_state). However, in the out_free block below:

    for (int i = 0; i < num_pipes; i++)
        if (joined_pipe_state[i].old_plane_state)
            intel_plane_destroy_state(&joined_pipe_state[i].plane->base,
                                      &joined_pipe_state[i].old_plane_state->uapi);

The secondary pipes' old states are destroyed without being unpinned first.
This frees the state and drops the active framebuffer reference while the
secondary display hardware may still be actively scanning out the old
framebuffer.

Would it make sense to update the unpinning logic to iterate over all joined
pipes?

>  
>  out_free:
> -	if (new_crtc_state)
> -		intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi);
> -	if (ret)
> -		intel_plane_destroy_state(&plane->base, &new_plane_state->uapi);
> -	else if (old_plane_state)
> -		intel_plane_destroy_state(&plane->base, &old_plane_state->uapi);
> +	if (ret) {
> +		for (int i = 0; i < num_pipes; i++) {
> +			intel_plane_unpin_fb(joined_pipe_state[i].new_plane_state);
> +			intel_plane_destroy_state(&joined_pipe_state[i].plane->base,
> +						  &joined_pipe_state[i].new_plane_state->uapi);
> +		}
> +	} else {
> +		for (int i = 0; i < num_pipes; i++)
> +			if (joined_pipe_state[i].old_plane_state)
> +				intel_plane_destroy_state(&joined_pipe_state[i].plane->base,
> +							  &joined_pipe_state[i].old_plane_state->uapi);
> +	}
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3