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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Wed, 03 May 2023 09:48:54 +0200
Javier Martinez Canillas <javierm@redhat.com> 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 ?
> 
> > The other thing blocking this series was the testing of all the edge cases, I think
> > Simon and Pekka had some ideas for things to test (e.g. run mutter with support for
> > this and wayland without support for this in at the same time in different consoles
> > and see what happens). I never had the time to do that either.
> >  
> 
> I understand that every new feature needs tests but I fail to see why
> the bar is higher for this feature than others? I would prefer if this
> series are not blocked due some potential issues on hypothetical corner
> cases that might not happen in practice. Or do people really run two or
> more compositors on different console and switch between them ?

I have no recollection at all about what was talked about this, but in
certain virtualized cases you will always have two display systems
simultaneously:
- the guest display system which uses the nested KMS driver in the
  guest VM, which presents to
- a VM viewer application on the host, which presents via Wayland to
- the host display system which uses a hardware KMS driver.

Maybe it was more like that to test?


Thanks,
pq

> >> Zack, are you planning to re-spin a v3 of this patch-set? Asking because
> >> we want to take virtio-gpu out of the atomic KMS deny list in mutter, but
> >> first need this to land.
> >> 
> >> If you think that won't be able to do it in the short term, Bilal (Cc'ed)
> >> or me would be glad to help with that.  
> >
> > This has been on my todo for a while I just never had the time to go through all the
> > remaining issues. Fundamentally it's not so much a technical issue anymore, it's
> > about picking the least broken solution and trying to make the best out of a pretty
> > bad situation. In general it's hard to paint a bikeshed if all you have is a million
> > shades of gray ;)
> >  
> 
> Agreed. And I believe that other than the driver cap name, everyone agrees
> with the color of your bikeshed :)
> 


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