Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/vmwgfx: cursor atomic_check accepts VMW_CURSOR_UPDATE_NONE framebuffers --
commit 9cca2550f2f0603a47b5047da765ad6aeb14d8cc Author: Zack Rusin <[email protected]> drm/vmwgfx: validate the framebuffer selected for cursor updates This commit resolves the user object directly from the selected framebuffer during cursor plane validation and preparation rather than relying on the duplicated plane state's previous user object. It also adds checks to reject zero cursor dimensions and require a backing buffer object for GB-only and MOB cursor updates. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c > index f0661e5effeb1..9e8ea9e07f40f 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c [ ... ] > @@ -763,7 +775,21 @@ int vmw_cursor_plane_atomic_check(struct drm_plane > *plane, > if (!fb) > return 0; > > - update_type = vmw_cursor_update_type(vmw, vps); > + if (!new_state->crtc_w || !new_state->crtc_h) { > + drm_dbg_kms(&vmw->drm, > + "Cursor dimensions (%u, %u) must be nonzero\n", > + new_state->crtc_w, new_state->crtc_h); > + return -EINVAL; > + } > + > + /* The duplicated plane state's user object still belongs to the old > FB. */ > + uo = vmw_cursor_plane_user_object(fb); > + update_type = vmw_cursor_update_type(vmw, &uo); > + if ((update_type == VMW_CURSOR_UPDATE_GB_ONLY || > + update_type == VMW_CURSOR_UPDATE_MOB) && > + !vmw_user_object_buffer(&uo)) > + return -EINVAL; > + > if (update_type == VMW_CURSOR_UPDATE_LEGACY) { > if (new_state->crtc_w != VMW_CURSOR_SNOOP_WIDTH || > new_state->crtc_h != VMW_CURSOR_SNOOP_HEIGHT) { > @@ -772,7 +798,7 @@ int vmw_cursor_plane_atomic_check(struct drm_plane *plane, > new_state->crtc_w, new_state->crtc_h); > return -EINVAL; > } > - surface = vmw_user_object_surface(&vps->uo); > + surface = vmw_user_object_surface(&uo); > if (!surface || !surface->snooper.image) { > drm_warn(&vmw->drm, > "surface not suitable for cursor\n"); [Severity: Medium] Can this check for !surface || !surface->snooper.image ever be reached, and what happens when update_type is VMW_CURSOR_UPDATE_NONE? Looking at vmw_cursor_update_type(), it only returns VMW_CURSOR_UPDATE_LEGACY when surface && surface->snooper.image is already true. If vmw->has_mob is false and the new framebuffer does not have a snooper image (for example, a BO-backed dumb framebuffer or an atomic client surface where surface->snooper.image is NULL), vmw_cursor_update_type() returns VMW_CURSOR_UPDATE_NONE: drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c:vmw_cursor_update_type() { struct vmw_surface *surface = vmw_user_object_surface(uo); if (surface && surface->snooper.image) return VMW_CURSOR_UPDATE_LEGACY; if (vmw->has_mob) { if ((vmw->capabilities2 & SVGA_CAP2_CURSOR_MOB) != 0) return VMW_CURSOR_UPDATE_MOB; else return VMW_CURSOR_UPDATE_GB_ONLY; } drm_warn_once(&vmw->drm, "Unknown Cursor Type!\n"); return VMW_CURSOR_UPDATE_NONE; } When vmw_cursor_update_type() returns VMW_CURSOR_UPDATE_NONE, vmw_cursor_plane_atomic_check() skips the VMW_CURSOR_UPDATE_LEGACY, VMW_CURSOR_UPDATE_GB_ONLY, and VMW_CURSOR_UPDATE_MOB branches and returns 0 at the end of the function. Should vmw_cursor_plane_atomic_check() reject VMW_CURSOR_UPDATE_NONE with -EINVAL so that non-snooped framebuffers on non-MOB devices are not accepted? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
