[PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support

[Philipp Zabel]

Hi Pekka,

On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote:
> Sorry for the potentially duplicate email, my MUA messed up the CC list
> on the first go.
> 
> 
> On Thu, 23 Aug 2018 08:41:30 +0200
> Philipp Zabel <p.zabel at pengutronix.de> wrote:
> 
> > Hi,
> > 
> > On Wed, 2018-08-22 at 11:12 +0000, Marius-cristian Vlad wrote:
> > [...]  
> > > > Why not just send the connectors one by one, a single event with all
> > > > relevant information for each?    
> 
> Hi,
> 
> yes, sending one event per connector is a good design, but see below if
> we actually might want to extend that to creating an object per
> connector with "per-attribute" events for pieces of information.

One reason for creating an object would be that in some cases (like VR)
the client will want to match on vendor/model/serial from EDID, but in
other scenarios EDID data might not even be available.
When leasing a DPI or LVDS panel without EDID information, the connector
would have to be identified via the connector name, for example.

And object would allow handling both cases the same way when building
the lease request.

> > > Hmm, okay, I'll try do that.     
> > 
> > I'm wondering what should be used to identify a connector to a
> > hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to
> > other APIs that may request leases on the application's behalf.
> > 
> > What could be passed into a Vulkan function to request the lease and
> > retrieve the corresponding VkDisplayKHR object? wl_display and
> > make/model/serial strings? wl_display and drm connector name?
> > Or is there need for an object describing a leasable connector,
> > similar to wl_output?  
> 
> One certainly cannot rely on wl_output, because that will not be
> present for outputs that are not currently used as part of the desktop.
> IOW, HMDs are likely never exposed as a wl_output.

Understood, it would have to be a new object. There could be more of
them than wl_outputs, and they wouldn't carry the same information:
output geometry obviously has no place here. But there is some overlap.
The wl_output geometry event also contains "make" and "model" arguments
(but is missing product id or serial). And I notice that zxdg_output_v1
already has a "name" event that likely contains the connector name.
Maybe the leasable connector object should report all of these as
events.

> > Or should the leasing be done by the application itself, outside of
> > Vulkan, and just the zwp_kms_lease_v1 object be passed in?  
> 
> These are good questions, I hope answers will be found for what the
> Vulkan and other APIs will actually need for advertising what they need
> to advertise.

I suppose this is something that should be discussed on the Vulkan side
first? Given we'd need a new extension there anyway, maybe the solution
is not making this wayland specific at all and just adding a way to
import the leased DRM fd.

> > > > Especially EDID info would be most useful for finding the right VR
> > > > headset.  
> 
> Sharing EDID could be tricky mechanically. If we assume that an average
> EDID blob is 256 bytes, it would be small enough to pass "inline" in
> Wayland, e.g. as a wl_array argument on an event. But since we want to
> provide it for all leasable connectors, the burst of data could grow
> bigger than the buffers in libwayland, and we start relying on the
> kernel socket buffers which are much larger usually.

I'm not sure if pushing the complete EDID is necessary. Maybe there
should be an EDID request if preparsed vendor/product/serial or
connector name are good enough to identify the to-be-leased connector in
most cases.

> Maybe it's ok to start with a wl_array while we are still in unstable
> protocol, but this question may need to be revisited in the future.
> 
> Is there a defined upper limit on EDID size?

EDID contains an 8-bit number of extension blocks, there should be an
upper limit of about 32 KiB.

> Btw. since we may need to share EDID, and we may need applications to
> parse EDID to dig out what they need, I believe there will be a demand
> for a library to do that. Does any such exist already?

I'm not sure if there is a commonly used one.

[...]
> The client should not receive "all" the data from the compositor
> via Wayland, but only the bits it needs to make the decision whether to
> attempt leasing a connector or not.

Seconded, the protocol should contain just enough to make a decision to
lease.

> Modes are part of EDID, so if we send EDID, I think that's good enough.
> After all, we want to describe the monitor/HMD foremost, not what the
> kernel or the compositor have decided it can do (e.g. add standard
> modes the EDID might miss, or prune modes that cannot be used).

What about legacy LVDS and DPI displays that have no EDID information?
I expect those would be matched by connector name, but maybe there are
also use cases where somebody would select a connector to lease
depending on its native resolution or refresh rate.

> That data does not need to be complete either, I think. It should allow
> coarse pruning, and yes, ideally it would allow choosing the right
> connectors immediately, but clients still have the opportunity to lease
> a connector and ask DRM for more details before making the decision.

Ok.

> In summary, I don't know if we need to send the whole EDID or just
> selected standard fields from EDID (make, model, serial, ...), and
> likewise I'm not sure there is need to send video mode info if that's
> not useful for choosing a connector/monitor.

For the use cases that I can currently imagine (VR, special fixed
displays, projectors on a special connector) either

  manufacturer id (make), product code (better than monitor name
  string) and monitor serial number
or
  connector name

should be good enough. But as said, I'm not sure if there could be
cases where just matching native resolution or refresh rate would
be required.

> After the client has chosen a connector and leased it, then it will
> receive everything it needs directly from the kernel, e.g. a video mode
> list.

Yes.

> > If there was a "leasable connector" object instead of just the connector
> > event, that could report modes and EDID data as events, like wl_output.
> > I'm not sure if that is a good idea or unnecessary complication.  
> 
> That depends on how much data we want to send. A single message has a
> quite limited size. libwayland buffering sets a hard limit at 4096
> bytes, but in practise we should use, say, 1024 bytes max. If that
> message includes EDID inline, that's already 128-512 bytes taken.
>
> From protocol specification point of view, if the ad is just one event
> that carries all per-connector information, it cannot be extended. If we
> later want to include more information, we need to duplicate the event
> to add new arguments to it, because existing events cannot be modified
> without breaking all consumers. OTOH, if each piece of information is
> sent as a separate event on a "leasable connector" object, adding new
> pieces of information is much cleaner, no need to duplicate the
> existing events.

If we had an object, we could start with an event that just carries
model/vendor/serial and add an edid event (possibly even with an edid
request) later, if necessary.

[...]
> > Instead of submitting the connector identification as an argument to the
> > create request, would adding a separate add_connector request be more
> > extensible?
> > 
> > That would allow to request a single lease for multiple connectors,
> > or to add a mechanism to request adding overlay planes to the lease.  
> 
> If there is any possibility that we might want to build a single lease
> request from additional bits (e.g. planes) in the future, then I would
> recommend the add_connector request on a tentative lease config object
> approach. The linux_dmabuf extension already uses the same pattern to
> have a variable number of things in the final request.
> 
> As said, one cannot add arguments to an existing request or event later
> - you have to replace the whole message if you want something added.
> 
> Sorry, I'm a bit too busy to do a proper review of the protocol for
> now, but I hope these comments push it in a good direction.

Thank you for the detailed initial feedback.

regards
Philipp