Re: [PATCH 1/6] drm/i915/cursor: Check joiner cursor commit status

On 7/6/2026 5:26 PM, Nemesa Garg wrote:
> In joiner mode, secondary cursor commits may still be running
> even when the primary cursor commit is done. Walking the secondary
> pipes also requires holding the secondary planes modeset locks.
> Add intel_cursor_lock_joined_planes() to acquire modeset locks
> for all secondary cursor planes. Check all joined cursor commit
> status before taking the fast path. If any commit is still pending,
> fallback to slow path.
> 
> v2: Use intel_crtc_joined_pipe_mask(). [Ville]
> v3: Lock secondary cursor CRTCs and planes. [sashiko]
> v4: Iterate the full joined mask uniformly in both helpers, no
>      primary special-case.
>      Move the parameter-change check above the lock acquisition so
>      we don't grab secondary locks just to fall to slow path. [Chaitanya]
> 
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Nemesa Garg <[email protected]>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c | 69 ++++++++++++++++++---
>   1 file changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 88384dea868b..af7fc1f888c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -13,6 +13,7 @@
>   #include <drm/drm_vblank.h>
>   
>   #include "intel_atomic.h"
> +#include "intel_crtc.h"

do we need this header here?
I think it is only needed for intel_crtc_get_vblank_counter(). Add it 
with the patch that adds the dependency.

>   #include "intel_cursor.h"
>   #include "intel_cursor_regs.h"
>   #include "intel_de.h"
> @@ -796,6 +797,50 @@ void intel_cursor_unpin_work(struct kthread_work *base)
>   	intel_plane_destroy_state(&plane->base, &plane_state->uapi);
>   }
>   
> +static int intel_cursor_lock_joined_planes(struct intel_display *display,
> +					   const struct intel_crtc_state *crtc_state,
> +					   struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct intel_crtc *pipe_crtc;
> +	int ret;
> +
> +	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
> +					 intel_crtc_joined_pipe_mask(crtc_state)) {
> +		struct intel_plane *pipe_plane =
> +			intel_crtc_get_plane(pipe_crtc, PLANE_CURSOR);
> +
> +		ret = drm_modeset_lock(&pipe_crtc->base.mutex, ctx);
> +		if (ret)
> +			return ret;
> +
> +		ret = drm_modeset_lock(&pipe_plane->base.mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static bool
> +intel_cursor_joiner_commits_idle(struct intel_display *display,
> +				 const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *pipe_crtc;
> +
> +	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
> +					 intel_crtc_joined_pipe_mask(crtc_state)) {
> +		struct intel_plane *pipe_plane =
> +			intel_crtc_get_plane(pipe_crtc, PLANE_CURSOR);
> +		struct intel_plane_state *pipe_plane_state =
> +			to_intel_plane_state(pipe_plane->base.state);
> +
> +		if (pipe_plane_state->uapi.commit &&
> +		    !try_wait_for_completion(&pipe_plane_state->uapi.commit->hw_done))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   static int
>   intel_legacy_cursor_update(struct drm_plane *_plane,
>   			   struct drm_crtc *_crtc,
> @@ -833,15 +878,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	    crtc_state->joiner_pipes)
>   		goto slow;
>   
> -	/*
> -	 * Don't do an async update if there is an outstanding commit modifying
> -	 * the plane.  This prevents our async update's changes from getting
> -	 * overridden by a previous synchronous update's state.
> -	 */
> -	if (old_plane_state->uapi.commit &&
> -	    !try_wait_for_completion(&old_plane_state->uapi.commit->hw_done))
> -		goto slow;
> -
>   	/*
>   	 * If any parameters change that may affect watermarks,
>   	 * take the slowpath. Only changing fb or position should be
> @@ -855,6 +891,21 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	    !old_plane_state->uapi.fb != !fb)
>   		goto slow;
>   
> +	ret = intel_cursor_lock_joined_planes(display, crtc_state, ctx);
> +	if (ret == -EDEADLK)
> +		return ret;
> +	if (ret)
> +		goto slow;
> +
> +	/*
> +	 * Don't do an async update if there is an outstanding commit modifying
> +	 * any of the joined cursor planes. This prevents our async update's
> +	 * changes from getting overridden by a previous synchronous update's
> +	 * state.
> +	 */
> +	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;