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

Thu Aug 23 15:12:03 UTC 2018

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

Isn't this overkill? Asking the client to parse that blob?

Wouldn't make more sense to add the fields Philipp mentions
(manufacturer ID/product code) fields in drm_edid? The compositor
already parses the EDID to provide/supply a serial number/monitor name.

If the client can ask the kernel (through the leased fd) and chooses
what mode it wants, then what would be the point to send the whole
EDID? 

Regarding the size, I've modified the proto to include both drm_edid
fields we have currently and the EDID blob. The size depends on the
monitor/VR. My monitors have either 128 or 256-bytes EDIDs.

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