[PATCH v2 5/8] drm/vboxvideo: Use the hotspot properties from cursor planes

[Zack Rusin]

On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:
> On Mon, 11 Jul 2022 23:32:43 -0400
> Zack Rusin <zack at kde.org> wrote:
> 
> > From: Zack Rusin <zackr at vmware.com>
> > 
> > Atomic modesetting got support for mouse hotspots via the hotspot
> > properties. Port the legacy kms hotspot handling to the new properties
> > on cursor planes.
> > 
> > Signed-off-by: Zack Rusin <zackr at vmware.com>
> > Cc: Hans de Goede <hdegoede at redhat.com>
> > Cc: David Airlie <airlied at linux.ie>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > ---
> >  drivers/gpu/drm/vboxvideo/vbox_mode.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > index fa0d73ce07bc..ca3134adb104 100644
> > --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > @@ -429,8 +429,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
> >  	flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
> >  		VBOX_MOUSE_POINTER_ALPHA;
> >  	hgsmi_update_pointer_shape(vbox->guest_pool, flags,
> > -				   min_t(u32, max(fb->hot_x, 0), width),
> > -				   min_t(u32, max(fb->hot_y, 0), height),
> > +				   min_t(u32, max(new_state->hotspot_x, 0), width),
> > +				   min_t(u32, max(new_state->hotspot_y, 0), height),
> >  				   width, height, vbox->cursor_data, data_size);
> >  
> >  	mutex_unlock(&vbox->hw_mutex);
> 
> Hi,
> 
> this looks like silent clamping of the hotspot coordinates.
> 
> Should the DRM core perhaps already ensure that the hotspot must reside
> inside the plane (fb) boundaries and reject the atomic (TEST_ONLY)
> commit when it's outside?
> 
> Or if this restriction is not universal, maybe this driver should
> reject invalid hotspots rather than silently mangle them?

TBH, I'm not really sure why vboxvideo is clamping those values. I didn't want to
introduce any regressions by changing it here, but the hotspot code never specified
that the hotspot offsets have to be positive or even within the plane. In a quick
search I couldn't find real world cursors that were doing anything like that though
so I just left it.

> Also, if supports_virtual_cursor_plane is false, should there not be
> somewhere code that will ignore the values set to the atomic hotspot
> properties?

Hmm, good question. I'm not sure if there's a case where that could be possible:
without supports_virtual_cursor we either won't have a cursor plane or the client
won't be atomic and will only be allowed to use the old legacy kms ioctl's, i.e.
drmModeSetCursor2.

> When one KMS client switches to another, the first one being hotspot
> aware and the latter not, and both atomic, then the latter KMS client
> who doesn't know to drive the hotspot will inherit potentially invalid
> hotspot from the previous KMS client. Does something prevent that from
> being a problem?

That switch will result in plane state reset, ending in
__drm_atomic_helper_plane_state_reset which will reset both hotspot properties (set
them to 0).

> The old KMS client may have left the virtual cursor plane visible, and
> the new KMS client doesn't even know the virtual cursor plane exists
> because it doesn't set the DRM client cap. Something should probably
> ensure the virtual cursor plane gets disabled, right?

Hah, that's also a good question. I *think* the same code to above would be ran,
i.e. plane reset which should result in the plane disappearing and the new client
not being able to drive it anymore. At least when running gnome-shell, switching to
weston and then switching to gnome-shell things look ok, but I haven't tried running
such clients at the same time.

z