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

On Thu, 4 May 2023 01:43:51 +0000
Zack Rusin <zackr at vmware.com> wrote:
> 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?
Hi Zack,

I never implied to go that far as you make it sound here.

I only pointed out that there definitely are behavioral differences
that userspace MUST acknowledge and handle. The cursor plane being
special is just the start.

It does not invalidate the whole existing KMS UAPI, but *if* you aim
for optimal performance, you cannot ignore the differences or paper
over them either.

This NOT a NAK to this patch series in any way!
> 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.?
If you say so.

But then you need userspace to set those two integers. The concept of
those two integers does not even exist without explicit care in
userspace. You are already letting go of your goal to not need special
case code or changes in userspace like you said in (copied from above):
> > > 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.  
You want to make cursor planes special and not universal, which means
userspace needs special code to use them at all. That's a separate code
path. That is good! That is the way to get more performance through
better userspace decisions.
> 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. 
Again, I said half a meter, but you are jumping a mile.

I think you are seeing my comments as NAKs towards everything you try
to do. That's not my intention, and I cannot NAK anything anyway. I am
only pointing out that you seem too fixed to these details to see the
rest of the problems that hinder performance of nested KMS drivers.

See for example
https://gitlab.freedesktop.org/wayland/weston/-/issues/514

It's a nested KMS use case with qemu (would vmwgfx be any different?),
where the goal is to have zero-copy direct scanout from guest
application through guest KMS planes, qemu, host Weston onto host KMS
planes. The direct scanout works, but due to the UAPI design I
explained in my previous email, the only way to make that actually run
at ~full framerate is to very carefully manually tune the timings of
both guest and host Weston instance.

If you deem the cost of gaining that performance too high, that's fine.

However, I do absolutely believe, that if you wanted it, any Wayland
compositor project that aims to be in any way a performant general
purpose compositor for desktops or more would be willing to follow you
in using new UAPI that would make life better with nested KMS drivers.
You just have to ask, instead of assume that userspace won't hear you.
After all, working better is in their interest as well.

I *am* trying to push you forward, not stop you.

Sorry, I'll try to keep quiet from now on since I don't have any stakes
here.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230504/17e24025/attachment.sig>