Re: [PATCH wayland-protocols] RFC: unstable: DRM lease support
Hi Marius, On 25 January 2018 at 10:17, Marius-cristian Vlad wrote: > > 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. Right. What I meant is that we would build the list of objects incrementally, rather than requiring them to be passed all at once, e.g.: req = zwp _kms_lease_manager_v1_get_request(mgr); zwp_kms_lease_request_v1_add_output(req, output_hdmi1); /* adds CRTC + connector + primary plane */ zwp_kms_lease_request_v1_add_plane(req); /* adds overlay plane */ lease = zwp_kms_lease_request_v1_create(req); /* only here is drmModeCreateLease called */ zwp_kms_lease_request_v1_destroy(lease); This would give a little bit more flexibility and avoids hardcoding specific setups. It also makes it very clear what the life cycle is for each stage. > [mvlad] 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. I think you mean 'scanout plane' here: 'universal planes' refers to being able to treat scanout/cursor planes like overlay planes (i.e. enumerate them through GetResources, and configure them outside of SetCrtc/SetCursor/MoveCursor). I just checked now, and any kind of plane can be leased. There is one issue I spotted though: it's theoretically possible for Weston to not enable universal planes but enable leasing; this will result in us passing a plane ID of 0 (from our fake scanout plane) in the lease request. Probably best to just not enable leasing if universal planes isn't enabled; you'd have to have a very weird contorted partial backport of libdrm/kernel to do that, but people will absolutely try to do that, I'm sure. :) Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: [PATCH wayland-protocols] RFC: unstable: DRM lease support
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 Cc: wayland-devel@lists.freedesktop.org; Pekka Paalanen ; Keith Packard 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 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 a list, this means you should probably be using
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 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. > + 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. > + > + > + 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. 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 a list, this means you should probably be using an object instead. Other than that, I think it looks really good, and I'd be really happy to merge it in. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland-protocols] RFC: unstable: DRM lease support
Simple protocol extension for DRM leases, based on the work done by Keith Packard in libdrm [1] and in the Linux kernel [2]. There are two requests (create/revoke) and three events (created/revoked/failed). The server is responsible for choosing which output to lease. Another patch will follow that makes use of this procotol extension. [1] https://cgit.freedesktop.org/mesa/drm/commit/?id=c4171535389d72e9135c9615cecd07b346fd6d7e [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.15-rc9&id=62884cd386b876638720ef88374b31a84ca7ee5f Signed-off-by: Marius Vlad --- Makefile.am | 1 + unstable/drm-lease/README| 4 ++ unstable/drm-lease/drm-lease-unstable-v1.xml | 98 3 files changed, 103 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..ec67456 --- /dev/null +++ b/unstable/drm-lease/drm-lease-unstable-v1.xml @@ -0,0 +1,98 @@ + + + + +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. + It requires libdrm at least 2.4.89 and a recent (4.15) kernel. + + 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. + + Warning! The protocol described in this file is experimental and + backward incompatible changes may be made. Backward compatible changes + may be added together with the corresponding interface version bump. + Backward incompatible changes are done by bumping the version number in + the protocol and interface names and resetting the interface version. + Once the protocol is to be declared stable, the 'z' prefix and the + version number in the protocol and interface names are removed and the + interface version number is reset. + + + + + This asks for the creation of a lease. + +
