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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

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?

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?

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?

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?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]