Re: [PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers

[Zack Rusin <zackr@vmware.com>]

On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote:
> Zack Rusin <zackr@vmware.com> writes:
> 
> > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> > > !! External Email
> > > 
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > > 
> > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:
> > > > > From: Zack Rusin <zackr@vmware.com>
> > > > > 
> > > > > Cursor planes on virtualized drivers have special meaning and require
> > > > > that the clients handle them in specific ways, e.g. the cursor plane
> > > > > should react to the mouse movement the way a mouse cursor would be
> > > > > expected to and the client is required to set hotspot properties on it
> > > > > in order for the mouse events to be routed correctly.
> > > > > 
> > > > > This breaks the contract as specified by the "universal planes". Fix it
> > > > > by disabling the cursor planes on virtualized drivers while adding
> > > > > a foundation on top of which it's possible to special case mouse cursor
> > > > > planes for clients that want it.
> > > > > 
> > > > > Disabling the cursor planes makes some kms compositors which were broken,
> > > > > e.g. Weston, fallback to software cursor which works fine or at least
> > > > > better than currently while having no effect on others, e.g. gnome-shell
> > > > > or kwin, which put virtualized drivers on a deny-list when running in
> > > > > atomic context to make them fallback to legacy kms and avoid this issue.
> > > > > 
> > > > > Signed-off-by: Zack Rusin <zackr@vmware.com>
> > > > > Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> > > > > (v2)")
> > > 
> > > [...]
> > > 
> > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > index f6159acb8856..c4cd7fc350d9 100644
> > > > > --- a/include/drm/drm_drv.h
> > > > > +++ b/include/drm/drm_drv.h
> > > > > @@ -94,6 +94,16 @@ enum drm_driver_feature {
> > > > >       * synchronization of command submission.
> > > > >       */
> > > > >      DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > > > > +    /**
> > > > > +     * @DRIVER_VIRTUAL:
> > > > > +     *
> > > > > +     * Driver is running on top of virtual hardware. The most significant
> > > > > +     * implication of this is a requirement of special handling of the
> > > > > +     * cursor plane (e.g. cursor plane has to actually track the mouse
> > > > > +     * cursor and the clients are required to set hotspot in order for
> > > > > +     * the cursor planes to work correctly).
> > > > > +     */
> > > > > +    DRIVER_VIRTUAL                  = BIT(7),
> > > > 
> > > > I think the naming here is unfortunate, because people will vonder why
> > > > e.g. vkms doesn't set this, and then add it, and confuse stuff completely.
> > > > 
> > > > Also it feels a bit wrong to put this onto the driver, when really it's a
> > > > cursor flag. I guess you can make it some kind of flag in the drm_plane
> > > > structure, or a new plane type, but putting it there instead of into the
> > > > "random pile of midlayer-mistake driver flags" would be a lot better.
> > > > 
> > > > Otherwise I think the series looks roughly how I'd expect it to look.
> > > > -Daniel
> > > > 
> > > 
> > > AFAICT this is the only remaining thing to be addressed for this series ?
> > 
> > No, there was more. tbh I haven't had the time to think about whether the above
> > makes sense to me, e.g. I'm not sure if having virtualized drivers expose
> > "support
> > universal planes" and adding another plane which is not universal (the only
> > "universal" plane on them being the default one) makes more sense than a flag
> > that
> > says "this driver requires a cursor in the cursor plane". There's certainly a
> > huge
> > difference in how userspace would be required to handle it and it's way uglier
> > with
> > two different cursor planes. i.e. there's a lot of ways in which this could be
> > cleaner in the kernel but they all require significant changes to userspace,
> > that go
> > way beyond "attach hotspot info to this plane". I'd like to avoid approaches
> > that
> > mean running with atomic kms requires completely separate paths for virtualized
> > drivers because no one will ever support and maintain it.
> > 
> > It's not a trivial thing because it's fundamentally hard to untangle the fact
> > the
> > virtualized drivers have been advertising universal plane support without ever
> > supporting universal planes. Especially because most new userspace in general
> > checks
> > for "universal planes" to expose atomic kms paths.
> > 
> 
> After some discussion on the #dri-devel, your approach makes sense and the
> only contention point is the name of the driver feature flag name. The one
> you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact
> that vkms won't set and is a virtual driver as well, is a good example).
> 
> Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING
> would be more accurate and self explanatory ?

Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds like
Pekka doesn't agree with this approach. As I mentioned in my response to him, I'd be
happy with any approach that gets paravirtualized drivers working with atomic kms,
but atm I don't have enough time to be creating a new kernel subsystem or a new set
of uapi's for paravirtualized drivers and then porting mutter/kwin to those.

z