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
