Re: [PATCH v3 2/8] drm/atomic: Add support for mouse hotspots

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 27 Jun 2023 10:56:39 +0200
Javier Martinez Canillas <javierm@redhat.com> wrote:

> Pekka Paalanen <ppaalanen@gmail.com> writes:
> 
> > On Mon, 26 Jun 2023 23:58:33 -0400
> > Zack Rusin <zack@kde.org> wrote:
> >  
> >> From: Zack Rusin <zackr@vmware.com>
> >> 
> >> Atomic modesetting code lacked support for specifying mouse cursor
> >> hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting
> >> the hotspot but the functionality was not implemented in the new atomic
> >> paths.
> >> 
> >> Due to the lack of hotspots in the atomic paths userspace compositors
> >> completely disable atomic modesetting for drivers that require it (i.e.
> >> all paravirtualized drivers).
> >> 
> >> This change adds hotspot properties to the atomic codepaths throughtout
> >> the DRM core and will allow enabling atomic modesetting for virtualized
> >> drivers in the userspace.
> >> 
> >> Signed-off-by: Zack Rusin <zackr@vmware.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> ---
> >>  drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++++++
> >>  drivers/gpu/drm/drm_atomic_uapi.c         | 20 +++++++++
> >>  drivers/gpu/drm/drm_plane.c               | 51 +++++++++++++++++++++++
> >>  include/drm/drm_plane.h                   | 15 +++++++
> >>  4 files changed, 100 insertions(+)  
> >
> > Hi Zack,
> >
> > where is the UAPI documentation for these new properties? I mean
> > something ending up in the HTML docs like what other properties have in
> > e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties
> >
> > Otherwise looks fine to me. Could drm_plane_create_hotspot_properties()
> > perhaps assert that the plane type is CURSOR?
> >  
> 
> I thought the same when reviewing but then I noticed this function is only
> called from __drm_universal_plane_init() if type is DRM_PLANE_TYPE_CURSOR.

Right, so why bother checking for DRIVER_CURSOR_HOTSPOT either?
Shouldn't the function be 'static' too, not exported, and not added to
a header?


Thanks,
pq

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