[PATCH wayland-protocols] Add the wl_drm protocol

[Pekka Paalanen]

On Tue, 14 Nov 2017 11:23:22 +0000
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> On 14 November 2017 at 08:18, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Mon, 13 Nov 2017 16:27:24 +0000
> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >  
> >> On 13 November 2017 at 14:52, Emil Velikov <emil.l.velikov at gmail.com> wrote:  
> >> > On 13 November 2017 at 14:21, Daniel Stone <daniel at fooishbar.org> wrote:  
> >> >> Hi Emil,
> >> >>
> >> >> On 2 November 2017 at 17:09, Emil Velikov <emil.l.velikov at gmail.com> wrote:  
> >> >>> Import latest version (v2) of the protocol from Mesa.
> >> >>>
> >> >>> From the README:
> >> >>>
> >> >>> Warning!
> >> >>> The goal is to share the protocol file across Mesa and other low-level
> >> >>> components graphics stack such as libva and Xwayland. File is moved to
> >> >>> wayland-protocols for sharing purposes _only_.
> >> >>>
> >> >>> The protocol is _not_ for public use. Furthermore Mesa and others consider
> >> >>> the protocol as deprecated over wl_dmabuf.  
> >> >>
> >> >> I suspect the sheer fact that we've recently added it as a stable
> >> >> published protocol would be enough for people to ignore that
> >> >> objection.
> >> >>
> >> >> I have to say that I'm still against this one, for a few reasons.
> >> >>
> >> >> Firstly, yes, VA-API got it wrong: there's no way it should be using
> >> >> unpublished private protocol. Luckily, newer versions of VA-API and
> >> >> the GStreamer integration now have both proper dmabuf integration as
> >> >> well as the notion of simply exporting buffers rather than trying to
> >> >> be a presentation _and_ decoding layer in one, so using a VA-API
> >> >> decoder piped to waylandsink is enough to get it working without
> >> >> wl_drm ever being used. Happy days. (Xwayland is in the same boat:
> >> >> lfrb's patches mean it can use dmabuf instead.)
> >> >>
> >> >> Secondly, apart from VA-API, it's not used generically. It gets
> >> >> published as a side effect of Mesa calling eglBindWaylandDisplayWL(),
> >> >> and the Mesa client code (both in libEGL.so, though the two versions
> >> >> can indeed get out of sync) uses it. Compositors don't explicitly
> >> >> advertise it, and again for the most part clients don't use it.
> >> >>
> >> >> What I worry is that, if we publish it, clients _will_ come to use it
> >> >> despite the warnings, and Mesa will never be able to stop advertising
> >> >> it. Having it be private/unpublished protocol is an implementation
> >> >> detail, but with a published protocol doc, the fact Mesa advertises it
> >> >> will come to be seen as ABI instead. I really, really, don't want to
> >> >> paint ourselves into that corner: wl_drm has always been an awkward
> >> >> special case, but I'd much prefer to kill it off than enshrine its
> >> >> position.
> >> >>
> >> >> There is one gap which zwp_linux_dmabuf_v1 doesn't fill, which is
> >> >> passing a device handle for the composition GPU, as well as maybe
> >> >> another for the scanout device. That would give the allocator enough
> >> >> information to determine correct buffer placement and so on. But if we
> >> >> ignore placement as we do today, just having format + modifiers would
> >> >> be enough for both EGL/Vulkan and VA-API clients which were aware of
> >> >> modifiers to work properly. Drivers without modifiers would still need
> >> >> the GPU device passed down so they can decide which magic tiling mode
> >> >> to use, but that'll go as they get converted over.
> >> >>
> >> >> I'd much prefer to see wl_drm go completely unused where possible on
> >> >> the client side first, then work out a transition plan for Mesa
> >> >> deprecating it, then we can say with a straight face that we're not
> >> >> publishing it because we expect it to disappear.
> >> >>  
> >> > Thanks for the elaborate answer Dan.
> >> >
> >> > Being the more paranoid person (me thinks), I doubt anyone outside of
> >> > existing users will use wl_drm.
> >> > I do see your concern though - let me see if I can figure out a way to
> >> > make it harder to misuse.
> >> >
> >> > What I am planning here is the de-facto deprecation of wl_drm. It's
> >> > somewhat evil, so I kept is half-hidden.
> >> > Here it is, in all its [evil] glory:
> >> >  - step 1: move wl_drm to wayland-protocols, throw as many obstacles
> >> > needed to prevent average Joe from using it
> >> >  - 2: switch users {Mesa...} to it
> >> >  - 3: in parallel with #2, propose/shout at people that wl_drm will be
> >> > gone soon (tm)
> >> >  - 4: after users have working wl_dmabuf implementation, kill off
> >> > wl_drm from wayland-protocols
> >> >  - 5: roll new wayland-protocols, bump the requirement in users
> >> >  - 6: drop 'dead' code from users
> >> >  - ...
> >> >  - Profit!
> >> >
> >> > How does that sound? I could add that to the commit message if you prefer.
> >> > FTR Pekka was asking why Mesa doesn't ship/install the file - because
> >> > it leads to a circular dependency.
> >> > Plus people are more likely to misuse it, in said case.
> >> >  
> >> Some more ideas that come to mind:
> >>
> >> * by moving the XML we do not make it stable, the protocol already is
> >>
> >> * users already depend on wayland-protocols ... to a degree
> >>
> >> * moving wl_drm serves as a lovely reminder, same as the version bump later on
> >> "We really want to update $user _away_ from use wl_drm"
> >> "We want to test the !wl_drm codepaths more extensively"  
> >
> > Hi Emil,
> >
> > sorry, I cannot see it doing that. What I would see from moving wl_drm
> > into wayland-protocols is "It is actually ok to start using wl_drm in
> > arbitrary programs, so we are making the protocol definition easily
> > accessible for everyone." Because that is the whole point of
> > wayland-protocols: promote extensions that everyone should use or at
> > least consider instead of making up custom ones. The protocols marked
> > as stable especially are that.
> >  
> The keywords we seems to be missing are public vs private.
> How about we add those notions and associate wl_drm with the latter?

Hi,

then I believe it cannot enter wayland-protocols really. Also it makes
no sense to publish a protocol in wayland-protocols and say no-one
should use it - then it should not be in wayland-protocols to start
with.

> > Another thing I would imagine user projects to think is: "Oh good, I
> > can stop carrying the copy I had." If they do that, and then you
> > remove, rename, or depracate wl_drm in wayland-protocols, the projects
> > will just get annoyed by the useless motion they were tricked into and
> > move back to having an internal copy of wl_drm.
> >
> > You can cause a lot of code churn by renaming wl_drm interfaces, but it
> > is superfluous. The same could be achieved by simply making Mesa not
> > advertise wl_drm at runtime.
> >  
> I'm not proposing any renaming, because as you mentioned - it doesn't help much.
> 
> Can you throw a quick plan, vaguely like the one I did earlier?
> One that covers deprecation and removal* of wl_drm.

I believe I already did below...

> >>
> >> * adding a single toggle in wayland-protocols instead of
> >> --disable-wl-drm in each of three projects
> >> --hack-hack-I-want-wl-drm-but-I-use-if-only-for-mesa-vaapu-xwayland
> >>  
> >
> > As explained above, that would be a useless side-step. We need such a
> > switch for Mesa runtime, not wayland-protocols build time.
> >  
> Why runtime? You mention it a few times throughout, but I'm failing to
> see any arguments behind it.
> There are a few other gotchas mentioned throughout tagged with *.

Because run time matters, build time is irrelevant - or you never
explained why build time is important to you.

It does not matter if anyone builds or does not build support for
wl_drm. What matter is, does it work. It cannot work, if Mesa does not
advertise it at runtime in the Wayland compositor extensions via
wl_registry.

Not adding wl_drm to wl_registry at run time is the only way to
actually stop it from working. We could do that by a Mesa configure
option or a runtime option. The main point is that it will not be
present *by default* at run time.

Any build time solution involving clients OTOH is circumventable by all
client projects, which is what they already do by having a copy of the
XML file or the generated code for wl_drm. Therefore there is no
benefit in trying to change where client projects get the wl_drm
definitions from, they will get it if they want it regardless. They
already did.

> 
> > The projects using wl_drm as a client must already deal with the fact
> > that wl_drm might not be advertised by the server at runtime. Then it
> > is client project's choice if they just seize to work at all or use
> > something else.
> >  
> That is correct - clients _should_ know how to deal with it, sadly
> they do _not_.

Are you sure there is not even an assert(wl_drm_found) or similar? If
so, that means that the clients would *already* explode in "mysterious"
ways when running against a Wayland server that a) does not use Mesa,
or b) is not using EGL and hardware acceleration. So we cannot make
that any worse than it already is.

It is also not a problem. Your plan, my plan, they all break the same
eventually.

To me it just seems that you want break their builds first, and runtime
later. Why bother in trying to get all client projects to integrate
changes whose only purpose is to break their builds soon after? Why
would they ever agree to that?

The very definition of deprecating and removing wl_drm is that we break
everything relying on it. It has never been promised to be a public or
stable interface. Moving it into wayland-protocols would make both
promises.

> Even if we fix the current instances we cannot retroactively fit older
> releases ;-(
> 
> Now combine that with the fact that wayland client/server libs (+
> headers) will gladly deref NULL pointers and things get extra 'fun'.
> 
> Looking from the POV of the average Joe - the above breakages are hard
> to debug and will lead to a lot of noise/bugreports.

There are absolutely no consequences to libwayland-server users. The
server side is totally safe if it decides to not register a wl_drm
global in wl_registry. There just cannot be any unexpected function
calls or invalid pointers.

The only thing libwayland-client users can do wrong is pass a NULL or
uninitialized wl_drm pointer to protocol functions. If it is NULL, they
get an explosion in libwayland-client that is very obvious to debug. If
it is uninitialized, using an uninitialized pointer is something that
cannot be guarded against anyway and is hard to debug if it does not
explode immediately, but also has a high chance to explode similarly to
NULL.

I repeat that the latter is already triggerable right now by running
the client against a Wayland compositor that does not use Mesa EGL. Try
'weston --use-pixman' and see what happens to the clients.

> By using wayland-protocols, builders/packagers will effectively pair
> components that can work together.
> Additionally they will ensure an extra baseline testing, before
> shipping to users.

And they wouldn't otherwise?

> With the runtime toggle:
>  - the pairing will be missing - trying to use A with (or w/o) wl_foo with crash
>  - different parties will test different codepaths ... not ideal

If distributors do any baseline testing, they will see that
everything that relied on wl_drm is now broken with this new version of
Mesa, with my proposal to have Mesa by default not advertise wl_drm in
wl_registry, and regardless of where the wl_drm XML file lives or is
copied to.

> > What we need is to stop Mesa from advertising wl_drm on the Wayland
> > server side, with perhaps an opt-in "yes, I as a human user really do
> > use and need programs that rely on it" for the transition period.

...here, this was my proposal summary for the deprecation plan. In more
words:

1. Announce, that on <date>, Mesa will stop supporting wl_drm.

2. On that date, make Mesa not add the wl_drm global by default.
   Optionally, one can add a build or run time switch to get it back
   for a while.

3. Remove the switch if added.


> >  
> This proposal might work but we're looking at deprecation period a lot
> larger* than with my proposal.

Why?

Your proposal relies on getting explicit changes into client projects,
before it can proceed to a next phase. My proposal has no such
additional delay before throwing the switch.

> >> * explicit and annoying enable ^^ ensures
> >> a) people are less likely to misuse
> >> b) people use wl_dmabuf by default  
> >
> > If the enable was at Mesa runtime, not wayland-protocols build time,
> > yes. Otherwise no.
> >  
> Please elaborate*.

Explained above why a build time thing is not enough. Client projects
can circumvent it by doing literally nothing starting today.
Distributors just build and distribute them, and to the end user it
will never matter if the build required wayland-protocols or not.

The "explicit and annoying" would come from "Oh but you need to start
your compositor with MESA_ENABLE_DEPRECATED_WL_DRM=1 environment
variable for this client to work."


> >> * combinatoric explosion
> >> With separate toggles we have - mesa with wl_drm & vaapi w/o wl_drm etc
> >> Bump wayland-protocols req. across the board - everything is
> >> consistently built w/o wl_drm ;-)  
> >
> > I can't see why build options matter, when Wayland already requires all
> > clients to deal with the availability of an extension at runtime. A
> > project cannot use wl_drm, if a Wayland server does not advertise it.
> >  
> Original assumption was that each user will have their own toggle -

Why? There is really only one toggle: it's in Mesa, in the Wayland
server side part.

> from that POV it does not matter if it's a build or runtime one.
> Even if the client correctly handles wl_drm presence* we end up with
> combinatoric explosion. Which tends to be harder and slower to debug.

I just cannot see any combinatorial explosion here. Can you explain
exactly what combinations emerge? Why would your proposal make it any
different?


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: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171114/c2ac1ab5/attachment.sig>