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

[Zack Rusin <zackr@vmware.com>]

On Wed, 2022-07-13 at 10:20 +0300, Pekka Paalanen wrote:
> On Wed, 13 Jul 2022 03:35:57 +0000
> Zack Rusin <zackr@vmware.com> wrote:
> 
> > On Tue, 2022-07-12 at 10:56 +0300, Pekka Paalanen wrote:
> > > On Mon, 11 Jul 2022 23:32:43 -0400
> > > Zack Rusin <zack@kde.org> wrote:
> > >   
> > > > From: Zack Rusin <zackr@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@vmware.com>
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > Cc: Daniel Vetter <daniel@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).
> 
> It will?
> 
> To my knowledge, KMS has never reset anything when one KMS client
> switches to the next, so that's new.
> 
> What triggers it?
> 
> Only if you actually switch to fbdev/fbcon instead of another userspace
> KMS client, the fbdev code might reset some things, but not all.
> 
> > > 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.
> 
> That's an interesting experiment, but how is "at the same time"
> different from what you tested?
> 
> As i mentioned above, if you switch to fbcon in between, then you are
> not switching from one userspace KMS client to the next.

FWIW, running weston inside gnome-shell as a window also works fine. And running 
weston-simple-damage --width=60 --height=60 which, currently would make that client
pop up in the cursor plane, while running weston in a window inside gnome-shell also
works. I'm not sure what are the paths clients are taking in those cases so I don't
want to speculate but I'd be happy to try any other experiments or cases you think
might break.

z