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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 6/8/2026 11:56 AM, Nemesa Garg wrote:
> In joiner mode the fast path cursor update must handle
> secondary pipes. Iterate over all joined pipes uniformly
> to duplicate plane state, run check_plane(), pin the
> framebuffer and on success swap in the new plane state
> or each secondary cursor.
> 
> Track every successfully prepared pipe in per-pipe arrays
> so that later frontbuffer, unpin and error-cleanup paths
> treat primary and secondaries uniformly, and ensures the primary's
> pinned framebuffer is released if a secondary fails partway through.
> 

This whole loop remains dormant until patch 6. Worth a line in the 
commit message

> v2: Use intel_crtc_joined_pipe_mask(). [Ville]
>      Add locking mechanism. [Ville]
> v3: Drop the per-pipe fastpath mutex array. [sashiko]
> 
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c | 141 +++++++++++++++++---
>   1 file changed, 119 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 38234c6292ec..3da2c2308081 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -889,11 +889,17 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	struct intel_display *display = to_intel_display(plane);
>   	struct intel_plane_state *old_plane_state =
>   		to_intel_plane_state(plane->base.state);
> -	struct intel_plane_state *new_plane_state;
> +	struct intel_plane_state *new_plane_state = NULL;
>   	struct intel_crtc_state *crtc_state =
>   		to_intel_crtc_state(crtc->base.state);
> -	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc_state *new_crtc_state = NULL;
>   	struct intel_vblank_evade_ctx evade;
> +	struct intel_plane_state *old_pipe_states[4] = {};
> +	struct intel_plane_state *new_pipe_states[4] = {};
> +	struct intel_plane *pipe_planes[4] = {};
> +	struct intel_crtc *pipe_crtcs[4] = {};

Use I915_MAX_PIPES instead of magic 4.

> +	struct intel_crtc *pipe_crtc;
> +	int num_pipes = 0;
>   	int ret;
>   
>   	/*
> @@ -944,8 +950,10 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   		goto slow;
>   
>   	new_plane_state = to_intel_plane_state(intel_plane_duplicate_state(&plane->base));
> -	if (!new_plane_state)
> -		return -ENOMEM;
> +	if (!new_plane_state) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
>   
>   	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(&crtc->base));
>   	if (!new_crtc_state) {
> @@ -967,11 +975,75 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	if (ret)
>   		goto out_free;
>   
> +	pipe_planes[num_pipes] = plane;
> +	pipe_crtcs[num_pipes] = crtc;
> +	old_pipe_states[num_pipes] = old_plane_state;
> +	new_pipe_states[num_pipes] = new_plane_state;
> +	num_pipes++;
> +
> +	/*
> +	 * Iterate over all joined pipes (primary and secondary) uniformly.
> +	 * The joined pipe mask includes both the primary pipe and all
> +	 * secondary joiner pipes, allowing us to handle them all the same way.
> +	 */
> +	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
> +					 intel_crtc_joined_pipe_mask(crtc_state)) {
> +		struct intel_plane *pipe_plane;
> +		struct intel_crtc_state *pipe_crtc_state;
> +		struct intel_plane_state *old_pipe_plane_state;
> +		struct intel_plane_state *new_pipe_plane_state;
> +
> +		if (pipe_crtc == crtc)
> +			continue;
> +

Third way of "skip the primary" in this series.

> +		pipe_plane = intel_crtc_get_plane(pipe_crtc, PLANE_CURSOR);
> +		pipe_crtc_state = to_intel_crtc_state(pipe_crtc->base.state);
> +		old_pipe_plane_state = to_intel_plane_state(pipe_plane->base.state);
> +
> +		new_pipe_plane_state =
> +			to_intel_plane_state(intel_plane_duplicate_state(&pipe_plane->base));
> +
> +		if (!new_pipe_plane_state) {
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
> +
> +		intel_cursor_fastpath_update_plane_state(new_pipe_plane_state, fb,
> +							 &pipe_crtc->base,
> +							 pipe_crtc,
> +							 crtc_x, crtc_y,
> +							 crtc_w, crtc_h,
> +							 src_x, src_y,
> +							 src_w, src_h);
> +
> +		ret = pipe_plane->check_plane(pipe_crtc_state, new_pipe_plane_state);

This seems to be departure from the single pipe flow of calling 
intel_plane_atomic_check_with_state(). Any reason we don't call this 
wrapper for the secondary pipes? And if unneeded can we skip this for 
the primary pipe as well. More on it in Patch 4.

> +		if (ret) {
> +			intel_plane_destroy_state(&pipe_plane->base,
> +						  &new_pipe_plane_state->uapi);
> +			goto out_free;
> +		}
> +
> +		ret = intel_plane_pin_fb(new_pipe_plane_state, old_pipe_plane_state);
> +		if (ret) {
> +			intel_plane_destroy_state(&pipe_plane->base,
> +						  &new_pipe_plane_state->uapi);
> +			goto out_free;
> +		}
> +
> +		pipe_planes[num_pipes] = pipe_plane;
> +		pipe_crtcs[num_pipes] = pipe_crtc;
> +		old_pipe_states[num_pipes] = old_pipe_plane_state;
> +		new_pipe_states[num_pipes] = new_pipe_plane_state;
> +		num_pipes++;
> +	}
> +
>   	intel_frontbuffer_flush(to_intel_frontbuffer(new_plane_state->hw.fb),
>   				ORIGIN_CURSOR_UPDATE);
> -	intel_frontbuffer_track(to_intel_frontbuffer(old_plane_state->hw.fb),
> -				to_intel_frontbuffer(new_plane_state->hw.fb),
> -				plane->frontbuffer_bit);
> +
> +	for (int i = 0; i < num_pipes; i++)
> +		intel_frontbuffer_track(to_intel_frontbuffer(old_pipe_states[i]->hw.fb),
> +					to_intel_frontbuffer(new_pipe_states[i]->hw.fb),
> +					pipe_planes[i]->frontbuffer_bit);
>   
>   	/* Swap plane state */
>   	plane->base.state = &new_plane_state->uapi;
> @@ -1019,26 +1091,51 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   
>   	intel_psr_unlock(crtc_state);
>   
> -	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);
> -
> -		drm_vblank_work_schedule(&old_plane_state->unpin_work,
> -					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> -					 false);
> -
> -		old_plane_state = NULL;
> -	} else {
> -		intel_plane_unpin_fb(old_plane_state);
> +	/*
> +	 * Schedule or immediately unpin old framebuffers.
> +	 * Protect against concurrent access.
> +	 */
> +	for (int i = 0; i < num_pipes; i++) {
> +		struct intel_plane_state *old_pipe = old_pipe_states[i];
> +		struct intel_crtc *owner_crtc = pipe_crtcs[i];
> +
> +		if (old_pipe->ggtt_vma != new_pipe_states[i]->ggtt_vma) {
> +			drm_vblank_work_init(&old_pipe->unpin_work,
> +					     &owner_crtc->base,
> +					     intel_cursor_unpin_work);
> +			drm_vblank_work_schedule(&old_pipe->unpin_work,
> +						 drm_crtc_accurate_vblank_count(&owner_crtc->base) + 1,
> +						 false);
> +			old_pipe_states[i] = NULL;
> +		} else {
> +			intel_plane_unpin_fb(old_pipe);
> +		}

This schedules each pipe's unpin on its own crtc's vblank. Patch 6 then
changes this to the primary (&crtc->base). Any reason why? Either way 
the correct approach should be added in this patch instead of 
retro-fitting it in a future patch.

Functionally this looks more or less correct, but the code structure is 
quite fragmented and fragile - multiple parallel arrays indexed by 
num_pipes, ~6 separate passes over them, a
primary special-case, and error cleanup split across three sites.

I would also suggest to fold the four index-aligned arrays into one 
struct so they can't drift out of sync.
Try to unify the the primary and secondary pipes' code as much as possible.

I would also suggest splitting the patch into two:

- Reshape the existing primary-only fast path into num_pipes/array +
   loop form (num_pipes==1), including the goto out_free conversion and
   the per-pipe frontbuffer/unpin/cleanup loops. No functional change.
- Add the secondary-pipe loop  (or even better a unified loop for both 
primary and joined pipes).

In addition to this, most of the variable names needs a re-look in this 
patch. Some that really sticks out - new_pipe_plane_state for secondary 
pipe's plane_state, *_old_pipe_states for struct intel_plane_state.

Perhaps follow the convention of unprefixed for primary pipe and 
secondary_*/joined* for the joined pipes.

==
Chaitanya

>   	}
>   
>   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(new_pipe_states[i]);
> +			intel_plane_destroy_state(new_pipe_states[i]->uapi.plane,
> +						  &new_pipe_states[i]->uapi);
> +		}
> +
> +		/*
> +		 * Primary failed before being pushed (atomic_check_with_state
> +		 * or pin_fb): fb was never pinned, only destroy the state.
> +		 */
> +		if (!num_pipes && new_plane_state)
> +			intel_plane_destroy_state(&plane->base, &new_plane_state->uapi);
> +	} else {
> +		for (int i = 0; i < num_pipes; i++) {
> +			if (old_pipe_states[i])
> +				intel_plane_destroy_state(old_pipe_states[i]->uapi.plane,
> +							  &old_pipe_states[i]->uapi);
> +		}
> +	}
> +
>   	return ret;
>   
>   slow: