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

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

On 8/27/2026 10:12 PM, 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
> for each secondary cursor.
> 
> Track every successfully prepared pipe in a joined_pipe_state[] array
> of struct intel_cursor_joiner_state 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.
> 
> Convert the primary-only vblank unpin block into a loop over the
> joined_pipe_state[] array so each pipe's old cursor framebuffer is scheduled for
> unpin (or unpinned inline when unchanged) independently. All unpin
> work is armed on the primary crtc's vblank (&crtc->base), consistent
> with the single primary vblank evasion used above.
> 
> v2: Use intel_crtc_joined_pipe_mask(). [Ville]
>      Add locking mechanism. [Ville]
> v3: Drop the per-pipe fastpath mutex array. [sashiko]
> v4: Fold parallel arrays into struct intel_cursor_joiner_state joined_pipe_state[].
>      unify primary/secondary in a single loop.
>      use bare check_plane(). [Chaitanya]
> v5: Fix secondary uapi.crtc to each pipe's own crtc.
>      Rename intel_cursor_pipe to intel_cursor_joiner_state and
>      joined[] to joined_pipe_state[]. [Chaitanya]
> v6: Mirror primary uapi into secondary hw state, don't set uapi.crtc. [sashiko]
> v7: Squash patch 5 with this patch. [Chaitanya]

Thank you for squashing the patches. One small nit, "patch 5" won't mean 
much when this series is merged, use the following instead.

v7: Squash the per-pipe cursor unpin scheduling into this patch

The change can be made while merging no need to resend.

==
Chaitanya

> 
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c | 181 +++++++++++++-------
>   1 file changed, 120 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index b57b446b9788..e4a7d4d2915a 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -847,6 +847,7 @@ intel_cursor_joiner_commits_idle(struct intel_display *display,
>   
>   static void
>   intel_cursor_fastpath_update_plane_state(struct intel_plane_state *plane_state,
> +					 const struct intel_plane_state *from_plane_state,
>   					 struct drm_framebuffer *fb,
>   					 struct intel_crtc *hw_crtc,
>   					 int crtc_x, int crtc_y,
> @@ -854,20 +855,34 @@ intel_cursor_fastpath_update_plane_state(struct intel_plane_state *plane_state,
>   					 u32 src_x, u32 src_y,
>   					 u32 src_w, u32 src_h)
>   {
> -	drm_atomic_set_fb_for_plane(&plane_state->uapi, fb);
> -
> -	plane_state->uapi.src_x = src_x;
> -	plane_state->uapi.src_y = src_y;
> -	plane_state->uapi.src_w = src_w;
> -	plane_state->uapi.src_h = src_h;
> -	plane_state->uapi.crtc_x = crtc_x;
> -	plane_state->uapi.crtc_y = crtc_y;
> -	plane_state->uapi.crtc_w = crtc_w;
> -	plane_state->uapi.crtc_h = crtc_h;
> -
> -	intel_plane_copy_uapi_to_hw_state(NULL, plane_state, plane_state, hw_crtc);
> +	/*
> +	 * Only the primary owns its uapi state; a secondary mirrors it, so
> +	 * its uapi.crtc/fb stay NULL and hw.crtc comes from hw_crtc.
> +	 */
> +	if (plane_state == from_plane_state) {
> +		drm_atomic_set_fb_for_plane(&plane_state->uapi, fb);
> +
> +		plane_state->uapi.src_x = src_x;
> +		plane_state->uapi.src_y = src_y;
> +		plane_state->uapi.src_w = src_w;
> +		plane_state->uapi.src_h = src_h;
> +		plane_state->uapi.crtc_x = crtc_x;
> +		plane_state->uapi.crtc_y = crtc_y;
> +		plane_state->uapi.crtc_w = crtc_w;
> +		plane_state->uapi.crtc_h = crtc_h;
> +	}
> +
> +	intel_plane_copy_uapi_to_hw_state(NULL, plane_state, from_plane_state, hw_crtc);
>   }
>   
> +struct intel_cursor_joiner_state {
> +	struct intel_plane *plane;
> +	struct intel_crtc *crtc;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_plane_state *old_plane_state;
> +	struct intel_plane_state *new_plane_state;
> +};
> +
>   static int
>   intel_legacy_cursor_update(struct drm_plane *_plane,
>   			   struct drm_crtc *_crtc,
> @@ -883,11 +898,12 @@ 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_crtc_state *crtc_state =
>   		to_intel_crtc_state(crtc->base.state);
> -	struct intel_crtc_state *new_crtc_state;
>   	struct intel_vblank_evade_ctx evade;
> +	struct intel_cursor_joiner_state joined_pipe_state[I915_MAX_PIPES] = {};
> +	struct intel_crtc *pipe_crtc;
> +	int num_pipes = 0;
>   	int ret;
>   
>   	/*
> @@ -933,38 +949,68 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	if (!intel_cursor_joiner_commits_idle(display, crtc_state))
>   		goto slow;
>   
> -	new_plane_state = to_intel_plane_state(intel_plane_duplicate_state(&plane->base));
> -	if (!new_plane_state)
> -		return -ENOMEM;
> -
> -	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(&crtc->base));
> -	if (!new_crtc_state) {
> -		ret = -ENOMEM;
> -		goto out_free;
> -	}
> +	/*
> +	 * 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_cursor_joiner_state *j = &joined_pipe_state[num_pipes];
> +
> +		j->plane = intel_crtc_get_plane(pipe_crtc, PLANE_CURSOR);
> +		j->crtc = pipe_crtc;
> +		j->crtc_state = to_intel_crtc_state(pipe_crtc->base.state);
> +		j->old_plane_state = to_intel_plane_state(j->plane->base.state);
> +		j->new_plane_state =
> +			to_intel_plane_state(intel_plane_duplicate_state(&j->plane->base));
> +
> +		if (!j->new_plane_state) {
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
>   
> -	intel_cursor_fastpath_update_plane_state(new_plane_state, fb,
> -						 crtc,
> -						 crtc_x, crtc_y, crtc_w, crtc_h,
> -						 src_x, src_y, src_w, src_h);
> +		intel_cursor_fastpath_update_plane_state(j->new_plane_state,
> +							 joined_pipe_state[0].new_plane_state,
> +							 fb, pipe_crtc,
> +							 crtc_x, crtc_y,
> +							 crtc_w, crtc_h,
> +							 src_x, src_y,
> +							 src_w, src_h);
> +
> +		ret = j->plane->check_plane(j->crtc_state, j->new_plane_state);
> +		if (ret) {
> +			intel_plane_destroy_state(&j->plane->base,
> +						  &j->new_plane_state->uapi);
> +			goto out_free;
> +		}
>   
> -	ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
> -						  old_plane_state, new_plane_state);
> -	if (ret)
> -		goto out_free;
> +		ret = intel_plane_pin_fb(j->new_plane_state, j->old_plane_state);
> +		if (ret) {
> +			intel_plane_destroy_state(&j->plane->base,
> +						  &j->new_plane_state->uapi);
> +			goto out_free;
> +		}
>   
> -	ret = intel_plane_pin_fb(new_plane_state, old_plane_state);
> -	if (ret)
> -		goto out_free;
> +		num_pipes++;
> +	}
>   
> -	intel_frontbuffer_flush(to_intel_frontbuffer(new_plane_state->hw.fb),
> +	intel_frontbuffer_flush(to_intel_frontbuffer(joined_pipe_state[0].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);
>   
> -	/* Swap plane state */
> -	plane->base.state = &new_plane_state->uapi;
> +	for (int i = 0; i < num_pipes; i++)
> +		intel_frontbuffer_track(to_intel_frontbuffer(joined_pipe_state[i].old_plane_state->hw.fb),
> +					to_intel_frontbuffer(joined_pipe_state[i].new_plane_state->hw.fb),
> +					joined_pipe_state[i].plane->frontbuffer_bit);
> +
> +	for (int i = 0; i < num_pipes; i++) {
> +		joined_pipe_state[i].plane->base.state = &joined_pipe_state[i].new_plane_state->uapi;
> +
> +		if (joined_pipe_state[i].new_plane_state->uapi.visible)
> +			joined_pipe_state[i].crtc_state->active_planes |= BIT(PLANE_CURSOR);
> +		else
> +			joined_pipe_state[i].crtc_state->active_planes &= ~BIT(PLANE_CURSOR);
> +	}
>   
>   	/*
>   	 * We cannot swap crtc_state as it may be in use by an atomic commit or
> @@ -976,7 +1022,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	 * 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);
>   
> @@ -998,9 +1043,11 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   		local_irq_disable();
>   	}
>   
> -	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);
> +	if (joined_pipe_state[0].new_plane_state->uapi.visible) {
> +		intel_plane_update_noarm(NULL, plane, crtc_state,
> +					 joined_pipe_state[0].new_plane_state);
> +		intel_plane_update_arm(NULL, plane, crtc_state,
> +				       joined_pipe_state[0].new_plane_state);
>   	} else {
>   		intel_plane_disable_arm(NULL, plane, crtc_state);
>   	}
> @@ -1009,26 +1056,38 @@ 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 = joined_pipe_state[i].old_plane_state;
> +
> +		if (old->ggtt_vma != joined_pipe_state[i].new_plane_state->ggtt_vma) {
> +			drm_vblank_work_init(&old->unpin_work, &crtc->base,
> +					     intel_cursor_unpin_work);
> +			drm_vblank_work_schedule(&old->unpin_work,
> +						 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> +						 false);
> +			joined_pipe_state[i].old_plane_state = NULL;
> +		} else {
> +			intel_plane_unpin_fb(old);
> +		}
>   	}
>   
>   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;
>   
>   slow: