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

[Marius-cristian Vlad]

[resending... maybe this time works]

On Wed, 2018-09-05 at 08:55 +0200, Philipp Zabel wrote:
> Hi Marius,
> 
> thank you for the update!
Thank you for taking the time to look over this.

> 
> Am Dienstag, den 04.09.2018, 17:39 +0300 schrieb Marius Vlad:
> > Simple protocol extension to manage DRM lease. Based on the work by
> > Keith
> > Packard in [1], respectively [2].
> > 
> > [1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fcgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc417153538
> > 9d72e9135c9615cecd07b346fd6d7edata=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055sdata=p
> > qLcIrKTlgSJ0fVyfdXOXO4Re%2BV6OrNQGccIhMSn0Os%3Dreserved=0
> > [2] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
> > linux.git%2Fcommit%2F%3Fh%3Dv4.15-
> > rc9%26id%3D62884cd386b876638720ef88374b31a84ca7ee5fdata=02%7C0
> > 1%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055sdata=A
> > 3EfXGUUJMl%2FFQHw8LVHPn9Ptu%2BXKAr90EIwnHpLhzw%3Dreserved=0
> > 
> > Changes since v4:
> 
> [...]
> > - removed 'revoked' event entirely as it adds complexity without
> > adding
> > too much benefit.
> 
> The client will notice this via the leased drm fd sooner or later
> anyway, so it seems that this event is not strictly necessary. I
> wonder
> if there is any value in letting the client know immediately, though.
> For example, if a client displays mostly static content (like a
> presentation running on a non-desktop projector), it could be a long
> while until the next page flip attempt.
> 
> > Signed-off-by: Marius Vlad 
> > ---
> > 
> > An implementation for this version can be found at
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fcommits%2Fdrm-
> > lease-advertise-each-connnector-object-
> > fixesdata=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C620b40c9cbc5430e8a4b08d612fc9d85%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636717273515580055sdata=M
> > UrWEApwiGgSvcz8yKiT9Tm8Ij3UpGs6Ai5ioTtuWDI%3Dreserved=0
> > 
> 
> I tried the current weston implementation with this protocol version,
> and I think the leased property and lease object have to be moved
> from
> output to head.
> Currently, all heads without enabled output are skipped, and trying
> to
> lease a disabled monitor with:
> 
> [output]
> name=HDMI-A-1
> leasable=on
> mode=off
> 
> crashes the compositor.

Yes, I've never taken this into consideration. I assume this is the
case for HMDs where by default the connector will be disconnected? The
output contains the scanout_plane which contains the plane id, and
obviously for a disconnected output this will not be the case. 

I fixed the crash by checking if the output is set, but most likely
this not the proper fix, if this is required. 

> 
> [...]
> > +  Upon connecting to the compositor all leasable connectors
> > will be
> > +  advertised to the client.
> 
> What happens if a client has received the full list of leasable
> connectors and then one of those becomes unavailable, either because
> it
> is physically unplugged, or because it is leased to another client?
> Is the client notified about this?
> 
> What happens if a new connector becomes available, either because it
> is
> physically plugged in, or because another client cancels its lease?
> This could be handled by sending the connector advertisement (again),
> in which case leasable connectors could appear any time, not only
> upon
> connecting.

Care to explain a bit how exactly does the client reaches that state? 


"connector_added", "connected_add_failed" are events that shrink the
window in which the advertised connectors might be different when
actually adding the connector. 

> 
> >  These connectors are represented by
> > +  zwp_kms_lease_connector_v1 interface, and have to be "added"
> > to a lease
> > +  request object before creating a lease object. This
> > instructs the
> > +  compositor to use that connector when creating a lease. The
> > client can
> > +  receive multiple events for multiple leasable connectors and
> > needs a way
> > +  to discern between various leasable connectors. When
> > receiving this
> > +  connector object, events will be sent gratuitously so that
> > the
> > +  client can properly choose which connector wants to use.
> > +
> > +  A lease request object is represented by
> > zwp_kms_lease_request_v1
> > +  interface and is used temporarily as a storage place for
> > objects
> 
> 
> s/objects/object/

It's a confusion of terminology. Objects in compositor and objects 
for the lease creation. 

> 
> > +  IDs. Once the client has a lease object it can freely call
> > its
> > +  

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

[Marius-cristian Vlad]

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Marius-cristian Vlad]

On Thu, 2018-08-30 at 14:38 +0200, Philipp Zabel wrote:
> On Thu, 2018-08-30 at 11:01 +0000, Marius-cristian Vlad wrote:
> [...]
> > > One interesting question is how to handle the situation when the 
> > > client deliberately, or not, holds the lease indefinitely.
> > 
> > The client implicitly cancels the lease by closing the lease fd (or
> > by
> > having it closed automatically when it is terminated). The
> > compositor
> > will get a hotplug event then, and it has to check the lessee list 
> 
> when this
> 
> > happens and revoke leases to lost lessees.
> > 
> > Mvlad: That's what I expected to see, but I did some brief tests
> > here,
> > and it could the our lease kernel version could a little bit older,
> > but when SIGKILL'ing the client, the DRM subsystem doesn't revoke
> > the
> > lease, or calls drm_lease_destroy. I suppose the following happens:
> > when creating the lease the compositor will hold a reference of the
> > leased fd -- well technically the DRM leases part does -- (and ofc
> > I
> > can't close it then because the client won't be able to use it),
> 
> The compositor has to close lease_fd after sending it to the client
> via
> zwp_kms_lease_v1_send_leased_fd. Otherwise there's two open
> references
> to the lease drm_master, and closing only the client's reference will
> not cause it to be destroyed.

Yes, you are correct. I mistakenly though I can only close it after the
client sends the revoke request.
 

> 
> >  we pass that thru weston (over the unix socket), the client will
> > also
> > hold a reference for that leased fd, so basically the lease will
> > only
> > be destroyed when that ref counter hits 0. In the implementation I
> > close the leased fd deliberately on the compositor side, when I get
> > the revoke request from the client.
> 
> If there is only one open lease_fd reference in the client, there is
> no
> need for the revoke request. The client can just close the fd and the
> compositor will revoke the lease object after gets the hotplug event
> and
> notices the lessee is gone.
> 
> Now if the client keeps the leased_fd open but requests to destroy
> the
> zwp_kms_lease_v1 object, the compositor must revoke the DRM lease,
> same
> as when a VT switch happens or when the connector gets unplugged.

Alright I'll re-work this part and send an update. 

> 
> >  There's no issue when the client behaves, but only when the client
> > dies abruptly, and it won't notify the compositor that it is no
> > longer
> > using the lease. I've only seen this once when you brought it up,
> > so I
> > need to test it further to be sure about this.
> 
> My understanding is that if the client dies, the last open fd
> referencing the lease drm_master is closed

Yep, I've tested it now and that's how it works. 

> 
> > >  This has multiple
> > > ramification like what happens when the client un-expectingly
> > > dies and 
> > > doesn't revoking the lease,
> > 
> > See above, a lease terminates when its last fd is closed.
> > 
> > Mvlad: I'm not really sure on this, see my above comment. 
> > 
> > >  or when hot-plugging the connector and weston tries to get a
> > > hold of 
> > > a connector previously leased?
> > 
> > I'd say hot-unplugging a connector that is part of a lease should
> > cause this lease to be canceled.
> > 
> > mvlad: Which means that the client would have to be notified when
> > that happens, so it can shut down cleanly. 
> 
> Could that be via the revoked event on the zwp_kms_lease_v1?

I'd say yes.

> 
> > Somehow the client (in its rendering part perhaps) has to check if
> > the lease is still valid. Guess we can also
> > verify errno, so it should not complicate the client that much.
> 
> Right. If at some point the client suddenly fails to pageflip, it
> should
> be able to cope.
> 
> [...]
> > Mvlad: This relates also the above comments as well. Need to verify
> > if
> > indeed it works like this. But a follow up question here regarding
> > planes: 
> > We've discussed this quite extensively, and I've concluded that it
> > is
> > much easier to gather the objects ids required to create the lease,
> > directly in the compositor as it already has that information. I
> > kind
> > of assume that the connector object will also provide the CRTC and
> > implicitly the planes it can drive.
> 
> I think the connector should bring its CRTC and a primary plane.
> 
> At least for devices that have overlay planes that can m

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

[Marius-cristian Vlad]

Excuse my MUA, but I have no other means to reply atm.

-Original Message-
From: Philipp Zabel  
Sent: Thursday, August 30, 2018 10:03 AM
To: Marius-cristian Vlad ; 
wayland-devel@lists.freedesktop.org
Cc: kei...@keithp.com; eu...@de.adit-jv.com; dan...@fooishbar.org; 
ppaala...@gmail.com; sardemff7+wayl...@sardemff7.net; p.za...@pengutronix.de
Subject: Re: [PATCH wayland-protocols v4] unstable/drm-lease: DRM lease 
protocol support

Hi Marius,

Disclaimer: I don't feel very qualified to comment on the protocol design, take 
this with a grain of salt.

mvlad: The more comments the better I say .

Am Mittwoch, den 29.08.2018, 14:00 +0300 schrieb Marius Vlad:
> Simple protocol extension to manage DRM lease. Based on the work by 
> Keith Packard in [1], respectively [2].
> 
> [1] 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgi
> t.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc4171535389d72e9135c
> 9615cecd07b346fd6d7edata=02%7C01%7Cmarius-cristian.vlad%40nxp.com
> %7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636712093917097791sdata=5xcj%2BmYAkgmahSPWIQOWgtjLpaOy
> jEzqAVEINvanfrc%3Dreserved=0 [2] 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%
> 2Fcommit%2F%3Fh%3Dv4.15-rc9%26id%3D62884cd386b876638720ef88374b31a84ca
> 7ee5fdata=02%7C01%7Cmarius-cristian.vlad%40nxp.com%7Ceba3e65e2d19
> 47d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6367
> 12093917097791sdata=s%2FUhj6o7JUe7Is1GO3NZUphEwXg1BJeGAvVhVbr9gaU
> %3Dreserved=0
> 
> Changes since v3:
> - instead of advertising all leasable connectors at once, advertise 
> each connector for each leasable output.  Use an object to represent a 
> leasable connector and provide a request to add that leasable 
> connector when creating the lease (Philipp Zabel)
> - add two events to handle add_connector request
> - add some comments in the manager interface to explain how the 
> protocol can/should be used
> 
> Changes since v2:
> - advertise connectors when creating a lease request object (Pekka 
> Paalanen)
> - when revoking the lease use the lease object instead of lessee id 
> (Pekka Paalanen)
> - various grammar/spelling fixes (Pekka Paalanen)
> 
> Changes since v1:
> - added manager: advertise lease capability and manage the lease 
> (Daniel Stone)
> - add request(s) for adding connector/crtc/plane to behave more like 
> dmabuf (Daniel Stone)
> 
> Signed-off-by: Marius Vlad 
> ---
> A (rather incomplete) implementation for this version can be found at
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> lab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Ftree%2Fdrm-lease-adverti
> se-each-connnector-objectdata=02%7C01%7Cmarius-cristian.vlad%40nx
> p.com%7Ceba3e65e2d1947d3c0eb08d60e46a50c%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C636712093917097791sdata=G4kPsx742yO4%2FY5swFgVLSk
> AMk5%2Fa9gnd7Z7%2BLSSgKU%3Dreserved=0
> 
> One interesting question is how to handle the situation when the 
> client deliberately, or not, holds the lease indefinitely.

The client implicitly cancels the lease by closing the lease fd (or by having 
it closed automatically when it is terminated). The compositor will get a 
hotplug event then, and it has to check the lessee list happens and revoke 
leases to lost lessees.

Mvlad: That's what I expected to see, but I did some brief tests here, and it 
could the our lease kernel version could a little bit older, but when 
SIGKILL'ing the client, the DRM subsystem doesn't revoke the lease, or calls 
drm_lease_destroy. I suppose the following happens: when creating the lease the 
compositor will hold a reference of the leased fd -- well technically the DRM 
leases part does -- (and ofc I can't close it then because the client won't be 
able to use it), we pass that thru weston (over the unix socket), the client 
will also hold a reference for that leased fd, so basically the lease will only 
be destroyed when that ref counter hits 0. In the implementation I close the 
leased fd deliberately on the compositor side, when I get the revoke request 
from the client. There's no issue when the client behaves, but only when the 
client dies abruptly, and it won't notify the compositor that it is no longer 
using the lease. I've only seen this once when you brought it up, so I need to 
test it further to be sure about this.

>  This has multiple
> ramification like what happens when the client un-expectingly dies and 
> doesn't revoking the lease,

See above, a lease terminates when its last fd is closed.

Mvlad: I'm not really sure on this, see my above comment. 

>  or when hot-plugging the connector and weston tries to get a hold of 
> a connector previously leas

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

[Marius-cristian Vlad]

On Fri, 2018-08-24 at 11:58 +0200, Philipp Zabel wrote:
> Hi Marius,
> 
> On Fri, 2018-08-24 at 09:21 +0000, Marius-cristian Vlad wrote:
> > On Fri, 2018-08-24 at 10:57 +0200, Philipp Zabel wrote:
> 
> [...]
> > > > 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.
> > > 
> > > And object would allow handling both cases the same way when
> > > building
> > > the lease request.
> > 
> > To make sure I understand this "object" leasing. Instead of
> > advertising
> > connectors this way:
> > 
> > 
> >   
> > This event advertises a leasable connector. This allows
> > the
> >    client to
> > choose which connector the compositor should lease. It can
> > either
> > use connector name, monitor name, or if that's not
> > sufficient
> > to parse
> > EDID blob.
> >   
> >    > />
> >   
> >   
> >   
> >   
> >   
> > 
> > 
> > We do something like this:
> > 
> > 
> >   
> > This event advertises a leasable connector object.
> >   
> >    > interface="zwp_kms_lease_connector_v1"
> >   summary="the new temporary" />
> > 
> > 
> > 
> > Then that interface is used to query info (like
> > EDID/conn_name/monitor
> > name)
> 
> Yes, that is my understanding as well.
> 
> I expect that for all currently known use cases the connector name
> (like
> xdg_output.name) and the monitor vendor / product id / serial string
> should be sufficient. Maybe the native resolution and refresh rate
> could
> be useful as well.
> 
> I would suggest leaving the raw EDID out for now, a request for it
> can
> be added later to zwp_kms_lease_connector_v1, if necessary. It won't
> be
> possible to pass otentially huge complete EDIDs in an array argument.
> 
> > and using that information to ask/revoke the lease (based on
> > either connector name, or based on EDID)?
> 
> I imagine that object to be passed to the lease request's
> add_connector
> request directly:
> 
>   
> ...
> 
>       interface="zwp_kms_lease_connector_v1"
>    summary="a leasable connector to be added to the lease
> request"/>
> 
>   
> 
> That way there is no need for separate requests
> "add_connector_by_name",
> "add_connnector_by_vendor_product_serial" etc.
> 
> The same could potentially be done later with a
> "zwp_kms_lease_plane_v1"
> object and "add_plane" request.

Got it. Will give it a test and see how it goes.


> 
> regards
> Philipp
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Marius-cristian Vlad]

On Fri, 2018-08-24 at 10:57 +0200, Philipp Zabel wrote:
> 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  wrote:
> > 
> > > Hi,
> > > 
> > > On Wed, 2018-08-22 at 11:12 +, 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.

To make sure I understand this "object" leasing. Instead of advertising
connectors this way:


  
This event advertises a leasable connector. This allows the
   client to
choose which connector the compositor should lease. It can
either
use connector name, monitor name, or if that's not sufficient
to parse
EDID blob.
  
  
  
  
  
  
  


We do something like this:


  
This event advertises a leasable connector object.
  
  



Then that interface is used to query info (like EDID/conn_name/monitor
name) and using that information to ask/revoke the lease (based on
either connector name, or based on EDID)?

Is this the right assumption?

> 
> > > > 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 name="connector"><   
>   
>   < 
> This event advertises a leasable connector. This allows the
> client to<
> choose which connector the compositor should lease. It can
> either<
> use connector name, monitor name, or if that's not sufficient
> to parse<   
> EDID
> blob.<   
> 
>   <
>  
>    /><
>   <  
>   <
>   <
>   < 
>   <
> <
>  
> (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 thes

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

[Marius-cristian Vlad]

On Thu, 2018-08-23 at 08:41 +0200, Philipp Zabel 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?
> > 
> > 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?
> 
> Or should the leasing be done by the application itself, outside of
> Vulkan, and just the zwp_kms_lease_v1 object be passed in?
> 
> > > Especially EDID info would be most useful for finding the right
> > > VR
> > > headset.
> > > 
> > > > 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
> 
> 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.
> 
> > > > A server/client implementation to match this protocol 
> > > > can be found at https://emea01.safelinks.protection.outlook.com
> > > > /?ur
> > > > l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%
> > > > 2Fco
> > > > mmits%2Fnew-drm-leasedata=02%7C01%7Cmarius-
> > > > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C68
> > > > 6ea1
> > > > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187sda
> > > > ta=G
> > > > 4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3Dreserved=0
> > > 
> > > This crashed for me in drm_lease_manager_create_lease_req with a
> > > NULL
> > > pointer dereference because head->output == NULL for an
> > > unconnected
> > > head.
> > > Also I noticed zwm_kms_lease_request_v1_implementation is missing
> > > the
> > > .destroy request callback.
> > 
> > Good catch, fixed. You need to fetch it again.
> 
> Thank you, it works now. I've poked at it a bit, and noticed that the
> compositor crashes if the same lease is requested again before it was
> improperly revoked. This happened in three cases:
> 
> - if a second weston-egl-simple-lease is started while the first one
>   is still running,
> - if weston-egl-simple-lease is stopped by a VT switch (because the
>   next pageflip fails) and started again after switching back to
> weston,
> - if weston-egl-simple-lease is killed with SIGTERM and started
> again.
> 
> In the last case the last frame the last frame of the already killed
> application was still visible on the output.
> 
> I've also tried pulling and replugging the cable on the leased
> connector, which ends with a compositor assertion after replugging:

Yes, all of those have to be fixed. I'll send an updated version of
the implementation for a new version of the protocol. 

Thanks for testing this!

> 
> weston: compositor/main.c:1726: drm_process_layoutput: Assertion
> `output->output->enabled' failed.
> 
> regards
> Philipp
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Marius-cristian Vlad]

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  wrote:
> 
> > Hi,
> > 
> > On Wed, 2018-08-22 at 11:12 +, 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" aft

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

[Marius-cristian Vlad]

On Wed, 2018-08-22 at 09:17 +0200, Philipp Zabel wrote:
> Hi Marius,

Hi,

> 
> On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote:
> > Simple protocol extension to manage DRM lease. Based on the work by
> > Keith
> > Packard in [1], respectively [2].
> > 
> > [1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fcgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc417153538
> > 9d72e9135c9615cecd07b346fd6d7edata=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187sdata=M
> > gPbYibU3CTjKcSMJaiqBSP7FfAJD2h%2BLPYiE%2FXJ8z0%3Dreserved=0
> > [2] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
> > linux.git%2Fcommit%2F%3Fh%3Dv4.15-
> > rc9%26id%3D62884cd386b876638720ef88374b31a84ca7ee5fdata=02%7C0
> > 1%7Cmarius-
> > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187sdata=S
> > h%2B1TbPeqbUnaYF%2FwenCZIKTcR9P7l585Ellujk0Bq8%3Dreserved=0
> > 
> > Signed-off-by: Marius Vlad 
> > 
> > Changes since v2:
> > - advertise connector names when creating a lease request object
> > (Pekka Paalanen)
> > - when revoking the lease use the lease object instead of lessee id
> > (Pekka Paalanen)
> > - various grammar/spelling fixes (Pekka Paalanen)
> > 
> > Changes since v1:
> > - added manager: advertise lease capability and manage the lease
> > (Daniel Stone)
> > - add request(s) for adding connector/crtc/plane to behave more
> > like dmabuf (Daniel Stone)
> > ---
> > Some caveats while at it. This is just in RFC form adapted from the
> > comments
> > I've  mostly got from Pekka Paalanen. It most certainly doesn't
> > address all the issues
> > brought up: like multi-node card environments/system, doing a
> > TEST_ONLY
> > commit before giving out a lease, or takes hot-plugging into
> > consideration. 
> > As I was side-tracked to other things and
> > being on hiatus for a while, I wanted first to get some impressions
> > first if
> > indeed this is the right approach and do some incremental updates
> > afterwards.
> > 
> > The are some issues which I'd like to point out from the beginning,
> > which
> > require some further comments. In no particular order:
> > 
> > I've found that I can't pass a wl_array as a way to advertise
> > various
> > information to the client. (wl_array works fine with integers not
> > with
> > allocated data). It would probably be better to advertise also
> > monitor name, other EDID info or available modes, but at the moment
> > I only 
> > join-split a delimiter between connector names and send it out as a
> > string.  
> > While it is ugly I'm not aware of a way to send this information
> > back to the
> > client in the form of a list. 
> > 
> > The client is aware before hand of this delimiter and has the
> > number of entries:
> > can easily choose or pick one of the connectors. It could be that
> > there are
> > some other ways this can be achieved, I welcome any kind of
> > feedback here.
> 
> Why not just send the connectors one by one, a single event with all
> relevant information for each?

Hmm, okay, I'll try do that. 

> 
> Especially EDID info would be most useful for finding the right VR
> headset.
> 
> > 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


> 
> > A server/client implementation to match this protocol 
> > can be found at https://emea01.safelinks.protection.outlook.com/?ur
> > l=https%3A%2F%2Fgitlab.freedesktop.org%2Fmarius.vlad0%2Fweston%2Fco
> > mmits%2Fnew-drm-leasedata=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7C44bf17ed24d748c059fb08d607ff5fda%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636705190740857187sdata=G
> > 4P%2F9BhTT8i0RI3bRpYsXPJQQ0uJjmp9TL8UrboMgDI%3Dreserved=0
> 
> This crashed for me in drm_lease_manager_create_lease_req with a NULL
> pointer 

Re: [PATCH weston 1/2] desktop-shell: fix output removal for background/panel

[Marius-cristian Vlad]

On Thu, 2018-06-21 at 15:53 +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> When the compositor has multiple outputs (not clones) and one of them
> is
> removed, the ones remaining to the right will be moved to close the
> gap.
> Because reflowing the remaining outputs happens before removing the
> wl_output global, we get the new output x,y before the removal. This
> causes us to consider the remaining output immediately to the right
> of
> the removed output to be a clone of the removed output whose x,y
> don't
> get updated. That will then hit the two assertions this patch
> removes.
> 
> The reason the assertions were not actually hit is because of a
> compositor bug which moved the remaining outputs in the wrong
> direction.
> The next patch will fix the reflow, so we need this patch first to
> avoid
> the asserts.
> 
> Remove the assertions and hand over the background and panel if the
> "clone" does not already have them. If the clone already has them, we
> destroy the unnecessary background and panel.
> 
> Signed-off-by: Pekka Paalanen 
> ---
>  clients/desktop-shell.c | 37 -
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> index 6d19d029..fcc0b657 100644
> --- a/clients/desktop-shell.c
> +++ b/clients/desktop-shell.c
> @@ -1337,19 +1337,30 @@ output_remove(struct desktop *desktop, struct
> output *output)
>   }
>  
>   if (rep) {
> - /* If found, hand over the background and panel so
> they don't
> -  * get destroyed. */
> - assert(!rep->background);
> - assert(!rep->panel);
> -
> - rep->background = output->background;
> - output->background = NULL;
> - rep->background->owner = rep;
> -
> - rep->panel = output->panel;
> - output->panel = NULL;
> - if (rep->panel)
> - rep->panel->owner = rep;
> + /* If found and it does not already have a
> background or panel,
> +  * hand over the background and panel so they don't
> get
> +  * destroyed.
> +  *
> +  * We never create multiple backgrounds or panels
> for clones,
> +  * but if the compositor moves outputs, a pair of
> wl_outputs
> +  * might become "clones". This may happen
> temporarily when
> +  * an output is about to be removed and the rest are
> reflowed.
> +  * In this case it is correct to let the
> background/panel be
> +  * destroyed.
> +  */
> +
> + if (!rep->background) {
> + rep->background = output->background;
> + output->background = NULL;
> + rep->background->owner = rep;
> + }
> +
> + if (!rep->panel) {
> + rep->panel = output->panel;
> + output->panel = NULL;
> + if (rep->panel)
Guess copy-pasta from above, but is the test necessary? 

Reviewed-by: Marius Vlad 

> + rep->panel->owner = rep;
> + }
>   }
>  
>   output_destroy(output);
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 0/2] Document review and commit access requirements

[Marius-cristian Vlad]

On Mon, 2018-06-18 at 16:42 +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> Hi,
> 
> for years we have relied on unwritten traditions on how to review
> patches. Gaining commit access has been a secret rite no-one really
> knew
> what was required for to ask or grant it. I would dare claim that
> this
> has been partially the reason for why there are so few people who
> routinely review and land patches. At least I hope so, because
> "unwritten" is something we can fix.
> 
> Let's try to write down the existing conventions and criteria we use
> to
> review patches. These will not be rules to be followed to the letter
> but
> to the spirit.
> 
> Once we have documented guidelines for quality assurance on patch
> review, we can set up rules for granting commit rights. The movement
> to
> document commit rights requirements started in the kernel DRM
> commmunity
> as a tool to give out commits rights to more people and get more
> people
> involved and reviewing patches. I believe we would certainly want
> more
> people involved with Wayland and Weston, but it won't work if we
> don't
> also get more reviewers and committers.
> 
> So here goes. Documenting what is expected from reviewers and commmit
> rights holders should make everyone's lives easier. These patches are
> my
> first take on it, and build on others' as referenced. I want to
> ensure
> that I am replaceable. That everyone is.

And now the bus factor will considerably diminish :-).

> 
> The guidelines will not be perfect from the start. They should we
> honed
> over time.
> 
> 
> Thanks,
> pq
> 
> 
> Pekka Paalanen (2):
>   contributing: add review guidelines
>   contributing: commit rights
> 
>  CONTRIBUTING.md | 82
> +
>  1 file changed, 82 insertions(+)
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Marius-cristian Vlad]

On Tue, 2018-05-29 at 17:10 +0300, Pekka Paalanen wrote:
> On Mon, 12 Feb 2018 16:51:51 +0200
> Marius Vlad  wrote:
> 
> > Simple protocol extension to manage DRM lease. Based on the work by
> > Keith
> > Packard in [1], respectively [2].
> > 
> > [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72
> > e9135c9615cecd07b346fd6d7e
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.
> > git/commit/?h=v4.15-rc9=62884cd386b876638720ef88374b31a84ca7ee5f
> > 
> > Signed-off-by: Marius Vlad 
> > 
> > Changes since v1:
> > - added manager: advertise lease capability and manage the lease
> > (Daniel Stone)
> > - add request(s) for adding connector/crtc/plane to behave more
> > like dmabuf (Daniel Stone)
> > ---
> >  Makefile.am  |   1 +
> >  unstable/drm-lease/README|   4 +
> >  unstable/drm-lease/drm-lease-unstable-v1.xml | 150
> > +++
> >  3 files changed, 155 insertions(+)
> >  create mode 100644 unstable/drm-lease/README
> >  create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml
> 
> Hi Marius,
> 
> it's great to have someone working on this!
> 
> I finally got a chance to look at it. Comments are inline as usual.
> Most
> of my questions call for an answer in the spec text.

Thanks for taking the time to go over this. I have some minor follow-up
questions bellow.

> 
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 4b9a901..4f6a874 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -2,6 +2,7 @@ unstable_protocols =
> > \
> >     unstable/pointer-gestures/pointer-gestures-unstable-v1.xml
> > \
> >     unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml
> > \
> >     unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml  
> > \
> > +   unstable/drm-lease/drm-lease-unstable-v1.xml
> > \
> >     unstable/text-input/text-input-unstable-v1.xml  
> > \
> >     unstable/input-method/input-method-unstable-v1.xml  
> > \
> >     unstable/xdg-shell/xdg-shell-unstable-v5.xml
> > \
> > diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
> > new file mode 100644
> > index 000..a25600c
> > --- /dev/null
> > +++ b/unstable/drm-lease/README
> > @@ -0,0 +1,4 @@
> > +Linux DRM lease
> > +
> > +Maintainers:
> > +Marius Vlad 
> > diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml
> > b/unstable/drm-lease/drm-lease-unstable-v1.xml
> > new file mode 100644
> > index 000..907efb0
> > --- /dev/null
> > +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> > @@ -0,0 +1,150 @@
> > +
> > +
> > +
> > +  
> > +Copyright 2018 NXP
> > +
> > +Permission is hereby granted, free of charge, to any person
> > obtaining a
> > +copy of this software and associated documentation files (the
> > "Software"),
> > +to deal in the Software without restriction, including without
> > limitation
> > +the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > +and/or sell copies of the Software, and to permit persons to
> > whom the
> > +Software is furnished to do so, subject to the following
> > conditions:
> > +
> > +The above copyright notice and this permission notice
> > (including the next
> > +paragraph) shall be included in all copies or substantial
> > portions of the
> > +Software.
> > +
> > +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > KIND, EXPRESS OR
> > +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > OTHERWISE, ARISING
> > +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER
> > +DEALINGS IN THE SOFTWARE.
> > +  
> > +
> > +  
> > +
> > +  This interface makes use of DRM lease written by Keith
> > Packard.
> > +
> > +  A DRM master can create another DRM master and ``lease''
> > resources it has
> > +  control over to the new DRM master. Once leased, resources
> > can not be
> > +  controlled by the owner unless the owner cancels the lease,
> > or the new
> > +  DRM master is closed.
> > +
> > +  A lease is a contract between the Lessor (DRM master which
> > has leased out
> > +  resources to one or more other DRM masters) and a Lessee
> > +  (DRM master which controls resources leased from another DRM
> > master). This
> > +  contract specifies which resources may be controlled by the
> > Lessee.
> > +
> > +  The Lessee can issue modesetting/page-flipping atomic
> > operations etc.,
> > +  just like a Lesor using the leased file-descriptor passed by
> > the Lesor.
> 
> 

Re: [PATCH weston v9 8/9] weston: support clone mode on DRM-frontend

[Marius-cristian Vlad]

Hi, 

This no longer applies, due to man/weston-drm changes. 

Fixing that I get "atomic: get couldn't commit new state: Invalid
argument". Does that mean that HW doesn't support CRTC sharing you
mention in the description? Am I using it properly?

One more thing I found that using the following config:

[output]
name=HDMI-A-1
mode=current
same-as=HDMI-A-2

[output]
name=HDMI-A-2
mode=current

I get ``Output 'HDMI-A-2' enabled with head(s) HDMI-A-1, HDMI-A-2.''

I would've expected  to see ``Output 'HDMI-A-1' enabled with head(s)
HDMI-A-1, HDMI-A-2.'' I don't know of this matters or not... the other
way around is the same (using in HDMI-A-2 section same-as=HDMI-A-1). 



On Thu, 2018-04-19 at 15:09 +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> Add a new output section key "same-as" for configuring clone mode. An
> output marked "same-as" another output will be configured identically
> to
> the other output.
> 
> The current implementation supports only CRTC sharing for clone mode.
> Independent CRTC clone mode cannot be supported until output layout
> logic is moved from libweston into the frontend and libweston's
> damage
> tracking issues stemming from overlapping outputs are solved.
> 
> Quite a lot of infrastructure is needed to properly configure clone
> mode. The implemented logic allows easy addition of independent CRTC
> clone mode once libweston supports it. The idea is that wet_layoutput
> is
> the item to be laid out and all weston_outputs a wet_layoutput
> contains show exactly the same area of the desktop.
> 
> The configuration logic attempts to automatically fall back to
> creating
> more weston_outputs when all heads do not work under the same
> weston_output. For now, the fallback path ends with an error message.
> 
> Enabling a weston_output is bit complicated, because one needs to
> first
> collect all relevant heads, try to attach them all to the
> weston_output,
> and then back up head by head until enabling the weston_output
> succeeds.
> A new weston_output is created for the left-over heads and the
> process
> is repeated.
> 
> CRTC-sharing clone mode is the most efficient clone mode, offering
> synchronized scanout timings, but it is not always supported by
> hardware.
> 
> v9:
> - replace weston_compositor_set_heads_changed_cb() with
>   weston_compositor_add_heads_changed_listener()
> - remove workaround in simple_head_enable()
> 
> v6:
> - Add man-page note about cms-colord.
> - Don't create an output just to turn it off.
> 
> Fixes: https://emea01.safelinks.protection.outlook.com/?url=https%3A%
> 2F%2Fphabricator.freedesktop.org%2FT7727=02%7C01%7Cmarius-
> cristian.vlad%40nxp.com%7C2382a4e34bb74b66b07c08d5a5ee862a%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636597366236269669=xoi2xcBR3
> YD9gBAUidQ%2BmoO6oH0QhX3HIvGbGYqySU0%3D=0
> 
> Signed-off-by: Pekka Paalanen 
> Acked-by: Derek Foreman 
> ---
>  compositor/main.c  | 492
> ++---
>  man/weston-drm.man |  12 ++
>  2 files changed, 484 insertions(+), 20 deletions(-)
> 
> diff --git a/compositor/main.c b/compositor/main.c
> index 85c4d338..fe36e69d 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -70,11 +70,41 @@ struct wet_output_config {
>  };
>  
>  struct wet_compositor;
> +struct wet_layoutput;
>  
>  struct wet_head_tracker {
>   struct wl_listener head_destroy_listener;
>  };
>  
> +/** User data for each weston_output */
> +struct wet_output {
> + struct weston_output *output;
> + struct wl_listener output_destroy_listener;
> + struct wet_layoutput *layoutput;
> + struct wl_list link;/**< in
> wet_layoutput::output_list */
> +};
> +
> +#define MAX_CLONE_HEADS 16
> +
> +struct wet_head_array {
> + struct weston_head *heads[MAX_CLONE_HEADS]; /**<
> heads to add */
> + unsigned n; /**< the number
> of heads */
> +};
> +
> +/** A layout output
> + *
> + * Contains wet_outputs that are all clones (independent CRTCs).
> + * Stores output layout information in the future.
> + */
> +struct wet_layoutput {
> + struct wet_compositor *compositor;
> + struct wl_list compositor_link; /**< in
> wet_compositor::layoutput_list */
> + struct wl_list output_list; /**< wet_output::link */
> + char *name;
> + struct weston_config_section *section;
> + struct wet_head_array add;  /**< tmp: heads to add as
> clones */
> +};
> +
>  struct wet_compositor {
>   struct weston_compositor *compositor;
>   struct weston_config *config;
> @@ -83,6 +113,7 @@ struct wet_compositor {
>   struct wl_listener heads_changed_listener;
>   int (*simple_output_configure)(struct weston_output
> *output);
>   bool init_failed;
> + struct wl_list layoutput_list;  /**<
> wet_layoutput::compositor_link */
>  };
>  
>  static FILE *weston_logfile = NULL;
> @@ -1094,12 +1125,6 @@ simple_head_enable(struct wet_compositor *wet,
> struct weston_head *head)
>   struct 

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

[Marius-cristian Vlad]

Ping? Daniel?

Have submitted a v4 for the server/client example. Would be nice to
know if the protocol is at least OK or needs more work.

On Mon, 2018-02-12 at 16:51 +0200, Marius Vlad wrote:
> Simple protocol extension to manage DRM lease. Based on the work by
> Keith
> Packard in [1], respectively [2].
> 
> [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9
> 135c9615cecd07b346fd6d7e
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.gi
> t/commit/?h=v4.15-rc9=62884cd386b876638720ef88374b31a84ca7ee5f
> 
> Signed-off-by: Marius Vlad 
> 
> Changes since v1:
> - added manager: advertise lease capability and manage the lease
> (Daniel Stone)
> - add request(s) for adding connector/crtc/plane to behave more like
> dmabuf (Daniel Stone)
> ---
>  Makefile.am  |   1 +
>  unstable/drm-lease/README|   4 +
>  unstable/drm-lease/drm-lease-unstable-v1.xml | 150
> +++
>  3 files changed, 155 insertions(+)
>  create mode 100644 unstable/drm-lease/README
>  create mode 100644 unstable/drm-lease/drm-lease-unstable-v1.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 4b9a901..4f6a874 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2,6 +2,7 @@ unstable_protocols =  
>   \
>   unstable/pointer-gestures/pointer-gestures-unstable-v1.xml  
>   \
>   unstable/fullscreen-shell/fullscreen-shell-unstable-v1.xml  
>   \
>   unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml  
>   \
> + unstable/drm-lease/drm-lease-unstable-v1.xml
>   \
>   unstable/text-input/text-input-unstable-v1.xml  
>   \
>   unstable/input-method/input-method-unstable-v1.xml  
>   \
>   unstable/xdg-shell/xdg-shell-unstable-v5.xml
>   \
> diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README
> new file mode 100644
> index 000..a25600c
> --- /dev/null
> +++ b/unstable/drm-lease/README
> @@ -0,0 +1,4 @@
> +Linux DRM lease
> +
> +Maintainers:
> +Marius Vlad 
> diff --git a/unstable/drm-lease/drm-lease-unstable-v1.xml
> b/unstable/drm-lease/drm-lease-unstable-v1.xml
> new file mode 100644
> index 000..907efb0
> --- /dev/null
> +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml
> @@ -0,0 +1,150 @@
> +
> +
> +
> +  
> +Copyright 2018 NXP
> +
> +Permission is hereby granted, free of charge, to any person
> obtaining a
> +copy of this software and associated documentation files (the
> "Software"),
> +to deal in the Software without restriction, including without
> limitation
> +the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> +and/or sell copies of the Software, and to permit persons to
> whom the
> +Software is furnished to do so, subject to the following
> conditions:
> +
> +The above copyright notice and this permission notice (including
> the next
> +paragraph) shall be included in all copies or substantial
> portions of the
> +Software.
> +
> +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR OTHER
> +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER
> +DEALINGS IN THE SOFTWARE.
> +  
> +
> +  
> +
> +  This interface makes use of DRM lease written by Keith
> Packard.
> +
> +  A DRM master can create another DRM master and ``lease''
> resources it has
> +  control over to the new DRM master. Once leased, resources can
> not be
> +  controlled by the owner unless the owner cancels the lease, or
> the new
> +  DRM master is closed.
> +
> +  A lease is a contract between the Lessor (DRM master which has
> leased out
> +  resources to one or more other DRM masters) and a Lessee
> +  (DRM master which controls resources leased from another DRM
> master). This
> +  contract specifies which resources may be controlled by the
> Lessee.
> +
> +  The Lessee can issue modesetting/page-flipping atomic
> operations etc.,
> +  just like a Lesor using the leased file-descriptor passed by
> the Lesor.
> +
> +  Besides the leased file-description, an integer is used to
> uniquely
> +  identify a Lessee within the tree of DRM masters descending
> from a single
> +  Owner. Once the Lessee has finished with the resources it had
> used, the
> +  Lessee ID can be used to revoke that lease.
> +
> +  Warning! The protocol described in this file is experimental
> and
> +  

Re: [PATCH weston] libweston/compositor-drm: Reset repaint scheduled status when setting DPMS level to off

[Marius-cristian Vlad]

On Fri, 2018-03-23 at 21:30 +0300, Greg V wrote:
> > On 7 March 2018 at 17:36, Marius Vlad  > nxp.com 
> >  > Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fwayland-
> > devel=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7Cf1acbc5280754c8936a908d590ec293a%7C686ea1
> > d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636574266340740999=Q60NW
> > AAtygRpT0s55ToG%2Fkz5DloXqOM5Bhy5xIEO6PM%3D=0>> wrote:
> > /
> > > /Otherwise when setting dpms level DPMS_ON, 
> > > weston_output_schedule_repaint() //will bail out early and never
> > > get a chance to wake up the output. Arguably this could also
> > > be done in drm_set_dpms() when setting 
> > > dpms_off_pending //but I figure it better to do it when
> > > deferred./
> > 
> > /
> > Thanks, that's a good catch, but I do wonder how it wasn't getting
> > tripped up before. In previous releases, we'd call drm_set_dpms()
> > from
> > anywhere, which would block on the update completing and then set
> > the
> > DPMS level. I wonder if it's because we would receive a flip-done
> > event anyway and call weston_output_finish_frame() after the DPMS
> > had
> > completed, which would drive us into repaint-idle.
> 
> Hi! I think I might have tripped that up before.
> 
> Or a different DPMS bug?
> Does "wake up the output" here mean DPMS wake, or starting drawing to
> that output again?
Hi,

When the monitor wakes up it will eventually start drawing. From your
description seems to be a different issue. This (patch) relates to
Daniels' basic atomic modesetting patches that have landed in a few
weeks ago, and to me it seems you are (still) using the legacy KMS page
flip ioctl.


> One of my monitors, which is quite slow to wake up (AOC U2879VF)
> pretty often becomes frozen after waking up from DPMS.
> While on the other monitor (an old NEC MultiSync) this problem never
> happened.
> So like I could move the mouse and see the picture change on that
> monitor, but the first one was still displaying the frozen picture
> from before going to sleep.
> 
> The log lines were:
> 
> > [00:42:10.740] queueing pageflip failed: m [00:42:10.740] Couldn't
> > apply 
> 
> state for output DP-2|

Yes, failing to queue page flips will lead a frozen image on the
screen. 

> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] libweston/compositor-drm: Reset repaint scheduled status when setting DPMS level to off

[Marius-cristian Vlad]

On Fri, 2018-03-09 at 09:35 +, Daniel Stone wrote:
> Hi Marius,
> 
> On 7 March 2018 at 17:36, Marius Vlad 
> wrote:
> > Otherwise when setting dpms level DPMS_ON,
> > weston_output_schedule_repaint()
> > will bail out early and never get a chance to wake up the output.
> > 
> > Arguably this could also be done in drm_set_dpms() when setting
> > dpms_off_pending
> > but I figure it better to do it when deferred.
> 
> Thanks, that's a good catch, but I do wonder how it wasn't getting
> tripped up before. In previous releases, we'd call drm_set_dpms()
> from
> anywhere, which would block on the update completing and then set the
> DPMS level. I wonder if it's because we would receive a flip-done
> event anyway and call weston_output_finish_frame() after the DPMS had
> completed, which would drive us into repaint-idle.

Indeed, I've submitted a previous patch because for timeline
instrumentation because of this. weston_output_finish_frame() is being
called with a NULL timestamp.

> 
> I suspect we should be calling finish_frame() to match that
> behaviour,
> to indicate that the previous update has completed, and then
> synchronously applying DPMS state. Pretending that
> disable/destroy/DPMS-off can be called at any time is really biting
> though. :(

I don't know enough about the repaint machinery, should this be done
from drm_set_dpms() or from drm_output_update_complete()?. I'm hitting
some asserts in both cases, maybe I'm doing it wrong :/.



> Cheers,
> Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Weston refresh rate?

[Marius-cristian Vlad]

On Wed, 2018-02-28 at 16:24 +, Vanhauwaert Wouter wrote:
> Hello all,
> 
> I seem to have an issue, but don't really know what/who to address.
> Whatever I try to run on my weston installation, I never seem to
> exceed +- 40fps. 
> That can also be seen by running the weston-simple-egl test.
> When I run weston-simple-egl -f, so fullscreen, I get upto 27 fps.
> But, when I add -b, so weston-simple-egl -f -b (Don't sync to
> compositor redraw (eglSwapInterval 0)), I get a stunning 175 fps!
> So what is limiting the weston refresh rate? I'm running on a NXP
> IMX6D, industrial grade, morty yocto, weston/wayland 1.11. 
> 
> My fb settings are:
>  
> mode "1920x1080-60"
> # D: 148.500 MHz, H: 67.500 kHz, V: 60.000 Hz
> geometry 1920 1080 1920 2160 16
> timings 6734 148 88 36 4 44 5
> accel false
> rgba 5/11,6/5,5/0,0/0
> endmode
> 
> EGL is provided by vivante
> 
> Any thaughts?

People pointed to this patch [1]. It's for weston-2.0 so I don't knowif still 
applies.  

[1] https://source.codeaurora.org/external/imx/weston-imx/commit/?h=wes
ton-imx-2.0=3c51b529bd7d177bbb8b0c88df96636967075499

> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fli
> sts.freedesktop.org%2Fmailman%2Flistinfo%2Fwayland-
> devel=02%7C01%7Cmarius-
> cristian.vlad%40nxp.com%7C8073acdc2385433e273108d5827bf050%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636558391180394327=DCEn4L%2B
> KD1cqohFDyf8s2pQ%2BRYkmIJEje%2B8iHbZUhr8%3D=0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 1/2 v3] compositor-drm: Add support for DRM lease

[Marius-cristian Vlad]

Hi Philipp, Pekka,

On Thu, 2018-02-22 at 10:29 +0200, Pekka Paalanen wrote:
> On Wed, 21 Feb 2018 17:16:07 +0100
> Philipp Zabel  wrote:
> 
> > Hi Marius,
> > 
> > On Wed, 2018-02-21 at 16:49 +0200, Marius Vlad wrote:
> > > Signed-off-by: Marius Vlad 
> > > ---
> > >  Makefile.am|   2 +
> > >  compositor/main.c  |   9 ++
> > >  configure.ac   |   4 +
> > >  libweston/compositor-drm.c | 272
> > > +
> > >  libweston/compositor.c |   1 +
> > >  libweston/compositor.h |   2 +
> > >  6 files changed, 290 insertions(+)
> > > 
> > > diff --git a/Makefile.am b/Makefile.am
> > > index b5c29c0..439fa73 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -163,6 +163,8 @@ nodist_libweston_@LIBWESTON_MAJOR@_la_SOURCES
> > > = \
> > >   protocol/viewporter-server-protocol.h   \
> > >   protocol/linux-dmabuf-unstable-v1-protocol.c\
> > >   protocol/linux-dmabuf-unstable-v1-server-protocol.h 
> > >   \
> > > + protocol/drm-lease-unstable-v1-protocol.c   \
> > > + protocol/drm-lease-unstable-v1-server-protocol.h
> > > \
> > >   protocol/relative-pointer-unstable-v1-protocol.c
> > > \
> > >   protocol/relative-pointer-unstable-v1-server-protocol.h 
> > >   \
> > >   protocol/pointer-constraints-unstable-v1-protocol.c 
> > >   \
> > > diff --git a/compositor/main.c b/compositor/main.c
> > > index 18810f2..020553f 100644
> > > --- a/compositor/main.c
> > > +++ b/compositor/main.c
> > > @@ -1092,6 +1092,15 @@ drm_backend_output_configure(struct
> > > wl_listener *listener, void *data)
> > >   api->set_seat(output, seat);
> > >   free(seat);
> > >  
> > > + char *lease;
> > > + weston_config_section_get_string(section, "lease",
> > > , "off");
> > > + if (!strncmp(lease, "on", 2)) {
> > > + output->lease = true;  
> > 
> > Should this be enabled by default for non-desktop outputs?
> > 
> > I suppose this is out of scope for this patchset, but I'd be
> > curious
> > if adding a non_desktop property to struct weston_output would be
> > appropriate.
> 
> That property would belong in struct weston_head instead, introduced
> by
> this series:
> https://patchwork.freedesktop.org/series/32898/
> 
> I also think "leasable" would be orthogonal to "non-desktop" as well.
> "leasable" applies more to the KMS resources, particularly the CRTC,
> for which we don't currently have an object in the DRM-backend, but
> we
> will need one to help with the handling of unused crtcs. "leasable"
> would go well in that object. 


Marking "leasable" an output using "non-desktop" can remove the need to
have a "lease" option in the config file. I see this working nicely
with HMDs. "non-desktop" property can also remove the need to iterate
over all connected connectors and request a lease (or at least the need
to talk to weston). The client still needs to go over each connectors
but this time, it requests the lease directly. 

On the other hand for Auto, the EDID quirk might not be available so 
we still need a method which can tell (hint) which output can be
leased. The configuration file allows a way to control that. (i.e, 
in under no circumstance I want this output to be disabled/leased)

Can a combination of both be more suitable to fit both use-cases?

- Mark "leasable" any "non-desktop" property but also the ones found in
configuration file.

- For the client, if it finds a "non-desktop" property for that
connector it can use it directly, other-wise fall-backs to request a
lease for each connected connector. 



> 
> 
> Thanks,
> pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH weston] desktop-shell: fix shell_output_destroy_move_layer unused variable

[Marius-cristian Vlad]

Thanks for spotting this. 

Reviewed-by: Marius-Vlad 

-Original Message-
From: wayland-devel [mailto:wayland-devel-boun...@lists.freedesktop.org] On 
Behalf Of Pekka Paalanen
Sent: Tuesday, February 13, 2018 4:22 PM
To: wayland-devel@lists.freedesktop.org
Cc: Pekka Paalanen 
Subject: [PATCH weston] desktop-shell: fix shell_output_destroy_move_layer 
unused variable

From: Pekka Paalanen 

/home/pq/git/weston/desktop-shell/shell.c: In function 
‘shell_output_destroy_move_layer’:
/home/pq/git/weston/desktop-shell/shell.c:4718:24: warning: unused variable 
‘output’ [-Wunused-variable]
  struct weston_output *output = data;

Since the data pointer is not used for anything, decided to also set it to NULL 
in the caller. This caused another variable to become unused.

Signed-off-by: Pekka Paalanen 
---
 desktop-shell/shell.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c index 
1c35d18a..ceb45c74 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4715,7 +4715,6 @@ shell_output_destroy_move_layer(struct desktop_shell 
*shell,
struct weston_layer *layer,
void *data)
 {
-   struct weston_output *output = data;
struct weston_view *view;
 
wl_list_for_each(view, >view_list.link, layer_link.link) @@ 
-4727,10 +4726,9 @@ handle_output_destroy(struct wl_listener *listener, void 
*data)  {
struct shell_output *output_listener =
container_of(listener, struct shell_output, destroy_listener);
-   struct weston_output *output = output_listener->output;
struct desktop_shell *shell = output_listener->shell;
 
-   shell_for_each_layer(shell, shell_output_destroy_move_layer, output);
+   shell_for_each_layer(shell, shell_output_destroy_move_layer, NULL);
 
if (output_listener->panel_surface)
wl_list_remove(_listener->panel_surface_listener.link);
--
2.13.6

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fwayland-devel=02%7C01%7Cmarius-cristian.vlad%40nxp.com%7Cd904556c45e9356a08d572ed2530%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636541285220933551=d3nvfNMxsx48ty4mqVkoahmkc9WlSelxbcWa1EkTT3M%3D=0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 0/3 v2] DRM lease support

[Marius-cristian Vlad]

Hi,

The sample client I've posted seems to be using the overlay plane.
Also, Daniel Stone confirmed a while back that they can be used.

However it seems that page flipping on the overlay plane can not
be used if the connector is used in the same time (and I'm not 
treating that case -- I disable that connector before handling out the
lease). Or primary plane -> page flipping, overlay -> wait for vblank?

Looking briefly over weston code I see that for overlay planes they use
wait for vblank mechanism. Maybe Daniel/Pekka can confirm this, or
maybe this is an issue with legacy API and atomic doesn't suffer from
this problem. I'm not 100% sure. 

I guess it would be interesting to see if we can use it in the same
time.

On Mon, 2018-02-12 at 15:54 +, Ucan, Emre (ADITG/ESB) wrote:
> Hi,
> 
> Is it possible to only lease an overlay plane, so that lessor and
> lessee share the same connector ?
> 
> Best regards
> 
> Emre Ucan
> Engineering Software Base (ADITG/ESB)
> 
> Tel. +49 5121 49 6937
> 
> > -Original Message-
> > From: wayland-devel [mailto:wayland-devel-
> > boun...@lists.freedesktop.org] On Behalf Of Marius Vlad
> > Sent: Montag, 12. Februar 2018 15:55
> > To: wayland-devel@lists.freedesktop.org
> > Cc: pekka.paala...@collabora.co.uk; kei...@keithp.com; Marius Vlad;
> > dani...@collabora.com
> > Subject: [PATCH weston 0/3 v2] DRM lease support
> > 
> > Patch series that adds support for DRM leases.
> > 
> > DRM leases is a method developed by Keith Packard to allow other
> > application
> > manage the output of a display/VR, while a DRM master is already
> > owning
> > the outputs resources. A more thorough explanation and terminology
> > can
> > be found at [1]. libdrm [2] and kernel [3] have already gained
> > support.
> > 
> > Initially this was a single patch but decided to split into series
> > to
> > accommodate a demo client and another change as to allow a smoother
> > transition
> > of the output from weston to a DRM leased client.
> > 
> > Marius Vlad (3):
> >   compositor-drm: Add support for drm-lease.
> >   compositor-drm: Do not perform a modeset when the output has been
> > leased
> >   clients/simple-egl-lease: A demo client for DRM leases
> > 
> > [1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2
> > F%2Fkeithp.com%2Fblogs%2FDRM-lease%2F=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7Cb40b120d26074b6c3bea08d57230fe4c%7Cbd8a2a
> > 2207224ec7b35f1c4f0497e341%7C0%7C1%7C636540477114485732=twp10
> > TCbXPHqW5Xw8ZFigw1UIY2vuE1hp32tFeBmx3E%3D=0
> > [2]
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > git.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinu
> > x.git%2Fcommit%2F=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7Cb40b120d26074b6c3bea08d57230fe4c%7Cbd8a2a
> > 2207224ec7b35f1c4f0497e341%7C0%7C1%7C636540477114485732=Q5lra
> > qIXACr109zP62gYM3aNsc4vaIOmpcPqr5jz%2F8g%3D=0?
> > h=v4.15-rc9=62884cd386b876638720ef88374b31a84ca7ee5f
> > [3]
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > cgit.freedesktop.org%2Fmesa%2Fdrm%2Fcommit%2F%3Fid%3Dc4171535389d72
> > e9135c=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7Cb40b120d26074b6c3bea08d57230fe4c%7Cbd8a2a
> > 2207224ec7b35f1c4f0497e341%7C0%7C1%7C636540477114485732=08pKA
> > s4xpNtWPCGMpXGJjtl4heCfb9Blk9MbjJzcuUM%3D=0
> > 9615cecd07b346fd6d7e
> > 
> > 
> >  Makefile.am|  13 +
> >  clients/simple-egl-lease.c | 880
> > +
> >  clients/simple-egl-lease.h |  99 +
> >  compositor/main.c  |   9 +
> >  configure.ac   |  21 ++
> >  libweston/compositor-drm.c | 250 -
> >  libweston/compositor.c |   1 +
> >  libweston/compositor.h |   2 +
> >  8 files changed, 1273 insertions(+), 2 deletions(-)
> >  create mode 100644 clients/simple-egl-lease.c
> >  create mode 100644 clients/simple-egl-lease.h
> > 
> > --
> > 2.9.3
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > lists.freedesktop.org%2Fmailman%2Flistinfo%2Fwayland-
> > devel=02%7C01%7Cmarius-
> > cristian.vlad%40nxp.com%7Cb40b120d26074b6c3bea08d57230fe4c%7Cbd8a2a
> > 2207224ec7b35f1c4f0497e341%7C0%7C1%7C636540477114485732=gSvIW
> > bGVSXkvq9Z6jf36Evw3EO5kGB4mew4O6NXE8Jc%3D=0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Marius-cristian Vlad]

-Original Message-
From: Pekka Paalanen [mailto:pekka.paala...@collabora.co.uk] 
Sent: Thursday, January 25, 2018 2:06 PM
To: Daniel Stone <dan...@fooishbar.org>
Cc: Marius-cristian Vlad <marius-cristian.v...@nxp.com>; Keith Packard 
<kei...@keithp.com>; wayland-devel@lists.freedesktop.org
Subject: Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for 
drm-lease.

On Thu, 25 Jan 2018 11:33:34 +
Daniel Stone <dan...@fooishbar.org> wrote:

> Hi Marius,
> 
> On 25 January 2018 at 11:10, Marius-cristian Vlad 
> <marius-cristian.v...@nxp.com> wrote:

> > >> +   wl_signal_emit(>wake_signal, 
> > >> compositor);
> > >> +   
> > >> + wl_event_source_timer_update(compositor->idle_source,
> > >> +
> > >> + compositor->idle_time * 1000);
> > >
> > > I assume this is just to force a repaint. If the existing 
> > > compositor API doesn't quite work for this, we should create API 
> > > which does, or make sure enabling the output does the right thing. 
> > > Are you using desktop-shell, or ... ?
> >
> > [mvlad] Indeed. What I've observed is that it could be some time 
> > until the repaint fires and in that time the fb of the client can 
> > still be present on that output. Forcing a repaint seems to fix 
> > that. There's also a longer explanation: If the client destroyes the 
> > fb this would cause the connector to be disabled. If weston can 
> > reclaim the connector after it has been disabled there's no issue.
> > I will need to check this once more, it might not be needed after 
> > all.
> 
> Right. If we create/enable a new Weston output, this should result in 
> repaint happening by itself: just like it does with hotplug now.

Still, should we have the client wait for the compositor to have actually 
posted a repaint of the output before the client destroys its fb?

[mvlad] I would assume that a client will detroy its fb and, afterwards will 
revoke the lease. You won't be able the access the leased fd after revoking it. 
Weston will not be unable to repaint anything if there's currently no ouput to 
repaint on. I hope I understand your question correctly :/.  

Do I understand right that the client destroying the fb would cause the CRTC 
and connector to be turned off immediately? Do we want to avoid that flicker if 
Weston is to take that output back into use?

[mvlad] Yes that is correct. RMFB will lead the CRTC and connector to be turned 
off. If there's no FB present the helpers in DRM atomic commit part will 
disable that CRTC.  

That brings to my mind the opposite question: if weston stops using an output 
so that it can lease it out, how's the flicker avoidance in that case?

[mvlad] There seems to be no flicker when destroying the output with 
drm_output_destroy. This is rather blunt, but that's what I see happening. 

What about leaking fb contents between the lessor and lessee?

[mvlad] The client has its own fbs. What could happen is that the ouput 
"shared" by lessor/lessee can have inter-leaved fbs if the
lesor is still using the output, but I see that happening only on purpose. 

I'm kind of guessing that avoiding flicker is out of scope and it may well 
happen, and that preventing fb content leaking is more important.
Is that right?

Does this result in toggling the CRTC off and on again on every hand-over? 
Given Weston's opportunistical usage of KMS resources, would it not create a 
risk of the lessee not being able to turn the CRTC back on if Weston manages to 
take more KMS resources (e.g. memory bandwidth via use of overlay planes) into 
use first?

[mvlad] Yes on every "contract" the CRTC will go thru that on/off stage. As 
long as weston is no longer using/owning that output (connector) I don't see 
how that's possible. How can it "take" more resources if it is not aware of 
that connector? Right?  


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Marius-cristian Vlad]

-Original Message-
From: Daniel Stone [mailto:dan...@fooishbar.org] 
Sent: Thursday, January 25, 2018 12:07 AM
To: Marius-cristian Vlad <marius-cristian.v...@nxp.com>
Cc: wayland-devel@lists.freedesktop.org; Pekka Paalanen 
<pekka.paala...@collabora.co.uk>; Keith Packard <kei...@keithp.com>
Subject: Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for 
drm-lease.

Hi Marius,
The protocol changes I suggested would require a fair bit of work here, but 
I've enclosed a few comments on the implementation.

[mvlad] No problem. Thanks for taking the time to review it. I'll adjust it 
accordingly. 

Also, do you have a client you're using for this somewhere, that we could use 
to test?

[mvlad] Indeed, would've make sense to add a test client for it. Will add it.  

On 24 January 2018 at 19:11, Marius Vlad <marius-cristian.v...@nxp.com> wrote:
> @@ -1208,6 +1209,15 @@ drm_backend_output_configure(struct wl_listener 
> *listener, void *data)
>
> api->set_seat(output, seat);
> free(seat);
> +#ifdef HAVE_DRM_LEASE
> +   char *lease;
> +   weston_config_section_get_string(section, "lease", , "off");
> +   if (!strncmp(lease, "on", 2)) {
> +   output->lease = true;
> +   weston_log("Enabling lease on output %s\n", output->name);
> +   }
> +   free(lease);
> +#endif

Hm, doing this in generic code seems a bit odd.

[mvlad] Not sure what you mean. Checking for "lease" in config file or the 
ifdefs? 

> +#ifdef HAVE_DRM_LEASE
> +/* hold current leases given */
> +static struct wl_list leases;

This should be under drm_backend.

[mvlad] Initially I wanted to have this isolated from the drm compositor, then 
I realized I needed some way
to disable/destroy the output. Having them inside drm_backend makes much more 
sense. 

> +struct drm_display {
> +   uint32_t connector_id;
> +   uint32_t crtc_id;
> +   uint32_t plane_id;
> +};

This seems to get stored but not used after that?

[mvlad] A helper struct to store the objects we want to pass. Easier to pass as 
a whole.  

> +struct drm_lease {
> +   int leased_fd;
> +   uint32_t leased_id;
> +   struct wl_list list;
> +};

The convention for embedded struct wl_list which is an element of a list, 
rather than a list head, is to call it 'link'.

[mvlad] OK, will fix that. 

> +struct drm_lease_data {
> +   struct drm_backend *drm_backend;
> +   struct udev_device *drm_device; };

This is a bit odd as we only have one udev device in the backend. Like the 
lease list though, any data for leases should just live directly in the 
drm_backend.

[mvlad] Understood.

> +#ifdef HAVE_DRM_LEASE
> +static void
> +drm_lease_destroy(struct drm_backend *b) {
> +   struct drm_lease *lease, *lease_iter;
> +
> +   wl_list_for_each_safe(lease, lease_iter, , list) {
> +   if (drmModeRevokeLease(b->drm.fd, lease->leased_id) < 0) {
> +   weston_log("Failed to revoke lease id %u\n",
> +  lease->leased_id);
> +   continue;
> +   }
> +
> +   weston_log("Lease id %u revoked\n", lease->leased_id);
> +   wl_list_remove(>list);
> +   free(lease);
> +   }
> +}
> +#endif

If individual leases were a separate object, you could have drmModeRevokeLease 
inside the resource destruction handler; this would then get called when either 
the client or the compositor was destroyed.

[mvlad] This sounds nice. Will give it a try. 

> +static void
> +drm_lease_create(struct wl_client *client, struct wl_resource 
> +*resource) {
> +   struct drm_lease_data *user_data = 
> wl_resource_get_user_data(resource);
> +   struct weston_compositor *compositor = 
> user_data->drm_backend->compositor;
> +   int drm_fd = user_data->drm_backend->drm.fd;
> +
> +   struct drm_output *output = NULL;
> +   struct drm_output *choosen_output = NULL;
> +
> +   uint32_t objects[3];
> +   uint32_t nobjects;
> +
> +   struct drm_lease *lease;
> +   struct drm_display display = {};
> +
> +   wl_list_for_each(output, >output_list, base.link) {
> +   struct weston_output *wet_output = >base;
> +   if (wet_output->lease) {
> +   display.crtc_id = output->crtc_id;
> +   display.connector_id = output->connector_id;
> +   display.plane_id = 
> + output->scanout_plane->plane_id;
> +
> +   choosen_output = output;
> +   break;
> +   }
> + 

RE: [PATCH wayland-protocols] RFC: unstable: DRM lease support

[Marius-cristian Vlad]

Apologies for inline answers. But there's no other way :(. I'll add my replies 
prefixed with my name.

-Original Message-
From: Daniel Stone [mailto:dan...@fooishbar.org] 
Sent: Wednesday, January 24, 2018 11:42 PM
To: Marius-cristian Vlad <marius-cristian.v...@nxp.com>
Cc: wayland-devel@lists.freedesktop.org; Pekka Paalanen 
<pekka.paala...@collabora.co.uk>; Keith Packard <kei...@keithp.com>
Subject: Re: [PATCH wayland-protocols] RFC: unstable: DRM lease support

Hi Marius,
Thanks a lot for taking this on! It would be great to get this merged.


On 24 January 2018 at 19:09, Marius Vlad <marius-cristian.v...@nxp.com> wrote:
> +  
> +
> +  This interface makes use of DRM lease written by Keith Packard.
> +  It requires libdrm at least 2.4.89 and a recent (4.15) kernel.

A serious nitpick, but we can leave the versions out of this, especially as 
people will backport support to older versions.

[mvlad] will remove it. 

> +  Events:
> +
> +  - created -- event sent when the lease has been created succesfully
> +  - revoked -- event sent when the lease has been revoked succesfully
> +  - failed -- event sent when create/revoke requests failed.
> +
> +  Requests:
> +
> +  - create -- asks the server to create a lease. The server decides which
> +  ouput to lease to the client. In case of success, the client receives
> +  an event with a DRM capable-fd. The client can then issue libdrm
> +  ioctls (i.e., modesetting).  The client also receives a lessee_id which
> +  has be used in revoke request. In case of failure, a failed event will
> +  be sent.
> +  - revoke -- asks the server to revoke a previously leased fd, using the
> +  lessee_id.
> +  A revoke event will be sent in case of success or failed event 
> otherwise.

Also, the events/requests are already described in the actual protocol. A 
couple of paragraphs about what DRM leases actually are, and why you 
would/wouldn't want to use them, would be more useful I think.

[mvlad] Sure, I'll add more detailed info. 

> +
> +  
> +   This asks for the creation of a lease.
> +  
> +
> +
> +
> +
> +   This event indicates that the lease has been created. It provides the
> +   leased fd which the client can use to perform modesetting and a lessee
> +   id to revoke the lease when it has finished with it.
> +
> +
> +
> +
> +
> +
> +
> +   This event indicates that the lease could not be created/revoked.
> +
> +
> +
> +
> +  
> +   This asks to revoke a lease using the lessee id previously given in 
> event
> +   created.
> +  
> +  
> +
> +
> +
> +
> +   This event indicates that the leased has been revoked.
> +
> +

This interface seems a little idiosyncratic. Essentially, the client asks for 
creation of one lease (any lease), and the server returns it a lease with an 
ID. After that, the client destroys all the leases through the same interface. 
There are a couple of things I would suggest to improve this protocol, and make 
it more like other Wayland
protocols:

Most Wayland protocols carry lots of small objects, since creating them is 
lightweight and straightforward. I think this protocol could be improved by 
doing something similar to the dmabuf protocol, which carries three objects: 
one global which is pretty much just for extension advertisement, one temporary 
object used in the creation of a buffer, and then the buffer object itself. 
Applied to leases, this would be a zwp_kms_lease_manager_v1 global which only 
had two
requests: one destroy, and the other to create a wp_kms_lease_request object. 
zwp_kms_lease_request_v1 would be analagous to
zwp_linux_dmabuf_params_v1: it would have requests to lease particular parts of 
the device (e.g. HDMI-2 output as well as two planes), and successful/failed 
events.

[mvlad] I have some doubts that we can lease just parts of the DRM chain. 
drmModeCreateLease wants all the objects (planes,crtc,connector) to be passed 
in one go. Per my understanding we can lease the connector and the CRTC. The 
encoder for instance can't be leased. It used to, but after some re-designed of 
the lease it seemed no longer necessasry. One more thing: I haven't tried 
leasing an overlay plane, only universal planes, so I'm not sure the kernel 
interface allows to do that, need to check that.

The successful event would carry a zwp_kms_lease_v1 object, which would 
represent the actual lease itself, only having a 'destroy'
method. Essentially, this object would do nothing but represent the lifetime of 
a lease. This is more idiomatic: as a rule of thumb, any time a request/event 
takes an 'id' parameter which you have to look up in