On Wed, 2022-07-13 at 10:20 +0300, Pekka Paalanen wrote: > On Wed, 13 Jul 2022 03:35:57 +0000 > Zack Rusin <zackr@xxxxxxxxxx> 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@xxxxxxx> wrote: > > > > > > > From: Zack Rusin <zackr@xxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxx> > > > > Cc: Hans de Goede <hdegoede@xxxxxxxxxx> > > > > Cc: David Airlie <airlied@xxxxxxxx> > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > --- > > > > 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
