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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added fast path logic for joined cursor planes is unreachable due to an unremoved fallback, and lacks secondary plane hardware update logic.
--

--- Patch [1]: [PATCH 1/6] drm/i915/cursor: Check joiner cursor commit status ---
commit 217b50906b0ee44f4341de677a7c7eeba08394c6
Author: Nemesa Garg <[email protected]>

drm/i915/cursor: Check joiner cursor commit status

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.

> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 0673f16f6fd0d..bc1e58d5c4c5e 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
[ ... ]
> @@ -833,15 +877,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  	    crtc_state->joiner_pipes)
>  		goto slow;

[Severity: Medium]
Since this existing check unconditionally sends joiner configurations to
the slow path, doesn't it make the newly added fast path logic below
unreachable for joiner pipes?

[ ... ]
> @@ -855,6 +890,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;
> +

[Severity: Medium]
If the earlier fallback were removed, would the hardware update logic at the
end of intel_legacy_cursor_update() also need to be updated?

Currently, it only updates 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);
	}

It looks like the secondary joined planes won't receive hardware updates if
the fast path is actually taken.

>  	new_plane_state = to_intel_plane_state(intel_plane_duplicate_state(&plane->base));
>  	if (!new_plane_state)
>  		return -ENOMEM;

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