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

[Zack Rusin]

On Wed, 2023-05-03 at 10:54 +0300, Pekka Paalanen wrote:
> On Wed, 3 May 2023 03:35:29 +0000
> Zack Rusin <zackr at vmware.com> wrote:
> 
> > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote:
> > > !! External Email
> > > 
> > > Daniel Vetter <daniel at ffwll.ch> writes:
> > >   
> > > > On Mon, Jul 11, 2022 at 11:32:39PM -0400, Zack Rusin wrote:  
> > > > > From: Zack Rusin <zackr at 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 at 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.
> 
> Hi Zack,
> 
> you'd like to avoid that, but fundamentally that really is what has to
> happen in userspace for *nested* KMS drivers (VKMS is a virtual driver
> but not part of the interest group here) to reach optimality.
> 
> It really is a different path. I see no way around that. But if you
> accept that fact, then you could possibly gain a lot more benefits by
> asking userspace to handle nested KMS drivers differently. What those
> benefits are exactly I'm not sure, but I have a feeling there should be
> some, where the knowledge of running on a nested KMS driver allows for
> better decisions that are not possible if the nested KMS driver just
> pretends to be like any other KMS hardware driver.

I'm not quite sure I buy the "virtualized drivers return immediately from a flip and
require two extra integers on the cursor plane, so there's no possible way drm api
could handle that" argument because it seems rather flimsy. If the premise is that
the paravirtualized drivers are so different that drm uapi can not handle them then
why would they stay in drm? What's the benefit if they'll have their own uapi and
their own interfaces?

I'd flip the argument and say you'd be a lot happier if you accepted that universal
planes aren't really universal no matter what. If weston puts a spreadsheet app in a
cursor plane presumably users would be concerned that their mouse cursor disappears
when they enter the spreadsheet app and responding to their concerns with "it's
cool, the planes are universal" wouldn't quite work. Something needs to special case
cursor plane no matter what. Anyway, I think we went through this argument of what
exactly "universal" refers to and whatever the definition of it is why I don't see
why two extra integers on cursor planes violates any part of it so I'll let it go. 

I have nothing against any of those solutions - from creating whole new kms uapi for
paravirtualized drivers, through creating a new subsystem for paravirtualized
drivers. I just have no interest in implementing those myself right now but if
someone implemented mutter/kwin code on top of either a new subsystem for
paravirtualized drivers or implemented atomic kms on top of some new api's for them,
we'll be sure to port vmwgfx to that. 

z