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

Thu Aug 23 11:39:05 UTC 2018

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.

> > 
> > 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.

> 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.

> > > 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.

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?

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?

> > >     
> > > > Secondly, when doing a modeset, the client requires a valid mode
> > > > (drmModeModeInfo).  I'm currently unsure how to pass this back to
> > > > the client.
> > > > The obvious type="object" interface="drmModeModeInfo" fails to link
> > > > and instead
> > > > I rely on the fact that a) the client can retrieve IDs from the
> > > > lease using
> > > > lease API (drmModeGetLease()) which gives a connector id -- 
> > > > or alternately can also use a wl_array to pass that,
> > > > and b) the client can use the leased fd to iterate over connectors.
> > > > Matching those two would allow to get a valid
> > > > drmModeModeInfo object to pass it when modesetting (for more info
> > > > see the client implementation provided at the end).
> > > > Question is, is this an acceptable way of doing it? Do note that
> > > > this
> > > > can only be "used" after the lease has been created.    
> > > 
> > > Can't the client query available modes on the passed connector via
> > > the
> > > leased fd?    
> > 
> > That's how the client does it now, it uses the leased fd to query
> > available modes. Presumably the client should have/receive all the data
> > from the compositor....    

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.

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).

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.

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.

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.

> 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.

On Thu, 23 Aug 2018 08:49:13 +0200
Philipp Zabel <p.zabel at pengutronix.de> wrote:

> On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
> [...]  
> > +  <interface name="zwp_kms_lease_request_v1" version="1">
> > +    <description summary="lease request object">
> > +      This interface is used for managing zwp_kms_lease_v1 object. It is used
> > +      to create a zwp_kms_lease_v1 object (the actual lease object) and also to
> > +      revoke the lease.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroys the lease request object">
> > +        This has no effect on any remaining objects created through this
> > +        interface.
> > +      </description>
> > +    </request>
> > +
> > +  <request name="create">
> > +    <description summary="create">
> > +      Create a lease using the connector output name received in zwp_kms_lease_manager_v1::connectors.
> > +      Any attempt to use an incorrect connector name will reswult in zwp_kms_lease_request_v1::failed.
> > +    </description>
> > +    <arg name="connector" type="string" summary="connector output name"/>    
> 
> 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.

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/20180823/656c2dee/attachment.sig>