Re: [PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
On Fri, 2018-08-24 at 11:58 +0200, Philipp Zabel wrote: > Hi Marius, > > On Fri, 2018-08-24 at 09:21 +, 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
Hi Marius, On Fri, 2018-08-24 at 09:21 +, 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: ... 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. 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
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 these as > events. > > > > Or should the leasing be done by the application itself, outside > > > of > > > Vulkan, and just the zwp_kms_lease_v1 object be passed in? > > > > These are good questions, I hope answers will be found for what the > > Vulkan and other APIs will actually need for advertising what they > > need > > to advertise. > > I suppose this is something that should be discussed on the Vulkan > side > first? Given we'd need a new extension there anyway, maybe the > solution > is not making this wayland specific at all and just adding a way to > import the
Re: [PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
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. > > > Hmm, okay, I'll try do that. > > > > I'm wondering what should be used to identify a connector to a > > hypothetical Vulkan VK_EXT_acquire_wayland_display extension, or to > > other APIs that may request leases on the application's behalf. > > > > What could be passed into a Vulkan function to request the lease and > > retrieve the corresponding VkDisplayKHR object? wl_display and > > make/model/serial strings? wl_display and drm connector name? > > Or is there need for an object describing a leasable connector, > > similar to wl_output? > > One certainly cannot rely on wl_output, because that will not be > present for outputs that are not currently used as part of the desktop. > IOW, HMDs are likely never exposed as a wl_output. Understood, it would have to be a new object. There could be more of them than wl_outputs, and they wouldn't carry the same information: output geometry obviously has no place here. But there is some overlap. The wl_output geometry event also contains "make" and "model" arguments (but is missing product id or serial). And I notice that zxdg_output_v1 already has a "name" event that likely contains the connector name. Maybe the leasable connector object should report all of these as events. > > Or should the leasing be done by the application itself, outside of > > Vulkan, and just the zwp_kms_lease_v1 object be passed in? > > These are good questions, I hope answers will be found for what the > Vulkan and other APIs will actually need for advertising what they need > to advertise. I suppose this is something that should be discussed on the Vulkan side first? Given we'd need a new extension there anyway, maybe the solution is not making this wayland specific at all and just adding a way to import the leased DRM fd. > > > > Especially EDID info would be most useful for finding the right VR > > > > headset. > > Sharing EDID could be tricky mechanically. If we assume that an average > EDID blob is 256 bytes, it would be small enough to pass "inline" in > Wayland, e.g. as a wl_array argument on an event. But since we want to > provide it for all leasable connectors, the burst of data could grow > bigger than the buffers in libwayland, and we start relying on the > kernel socket buffers which are much larger usually. I'm not sure if pushing the complete EDID is necessary. Maybe there should be an EDID request if preparsed vendor/product/serial or connector name are good enough to identify the to-be-leased connector in most cases. > Maybe it's ok to start with a wl_array while we are still in unstable > protocol, but this question may need to be revisited in the future. > > Is there a defined upper limit on EDID size? EDID contains an 8-bit number of extension blocks, there should be an upper limit of about 32 KiB. > Btw. since we may need to share EDID, and we may need applications to > parse EDID to dig out what they need, I believe there will be a demand > for a library to do that. Does any such exist already? I'm not sure if there is a commonly used one. [...] > The client should not receive "all" the data from the compositor > via Wayland, but only the bits it needs to make the decision whether to > attempt leasing a connector or not. Seconded, the protocol should contain just enough to make a decision to lease. > Modes are part of EDID, so if we send EDID, I think that's good enough. > After all, we want to describe the monitor/HMD foremost, not what the > kernel or the compositor have decided it can do (e.g. add standard > modes the EDID might miss, or prune modes that cannot be used). What about legacy LVDS and DPI displays that have no EDID information? I expect those would be matched by connector name, but maybe there are also use cases where somebody would select a connector to lease depending on its native resolution or refresh rate. > That data does not need to be
Re: [PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
On Thu, 23 Aug 2018 15:12:03 + Marius-cristian Vlad wrote: > On Thu, 2018-08-23 at 14:39 +0300, Pekka Paalanen wrote: > > > > > 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? As I mentioned below, I don't know if we *need* to send EDID. It all depends on what the applications will need for making the decision to attempt leasing. A problem with sending just specific parsed fields of EDID instead of the blob is that we need to update the protocol if new very useful fields appear in EDID. It's a trade-off, both ways can work. For the record, I think on X11 EDID is already exposed to clients, so VR runtimes might expect it to be available. Of course, one can get it after creating the lease, too, so not including it in the leasing protocol will not prohibit clients from getting it. It just takes more effort on the client. Thanks, pq pgpwRIIgGFava.pgp Description: OpenPGP digital signature ___ 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
On Thu, 2018-08-23 at 08:41 +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? > > > > 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
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" after the lease has been created. > > > > > > > > Can't the client query available modes on the passed connector > > > > via > > > > the > > > > leased fd? > > > > > > That's how the client does it now, it uses the leased fd to query > > > available modes. Presumably the client should have/receive all > > > the data > > > from the compositor > > The client should not receive "all" the data from the compositor > via Wayland, but only the bits it needs to make the decision whether > to > attempt leasing a connector or not. > > Modes are part of EDID, so if we send EDID, I think that's good > enough. > After all, we want to describe the monitor/HMD foremost, not what the > kernel or the compositor have decided it can do
Re: [PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
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? > > > > > > > Secondly, when doing a modeset, the client requires a valid mode > > > > (drmModeModeInfo). I'm currently unsure how to pass this back to > > > > the client. > > > > The obvious type="object" interface="drmModeModeInfo" fails to link > > > > and instead > > > > I rely on the fact that a) the client can retrieve IDs from the > > > > lease using > > > > lease API (drmModeGetLease()) which gives a connector id -- > > > > or alternately can also use a wl_array to pass that, > > > > and b) the client can use the leased fd to iterate over connectors. > > > > Matching those two would allow to get a valid > > > > drmModeModeInfo object to pass it when modesetting (for more info > > > > see the client implementation provided at the end). > > > > Question is, is this an acceptable way of doing it? Do note that > > > > this > > > > can only be "used" after the lease has been created. > > > > > > Can't the client query available modes on the passed connector via > > > the > > > leased fd? > > > > That's how the client does it now, it uses the leased fd to query > > available modes. Presumably the client should have/receive all the data > > from the compositor The client should not receive "all" the data from the compositor via Wayland, but only the bits it needs to make the decision whether to attempt leasing a connector or not. Modes are part of EDID, so if we send EDID, I think that's good enough. After all, we want to describe the monitor/HMD foremost, not what the kernel or the compositor have decided it can do (e.g. add standard modes the EDID might miss, or prune modes that cannot be used). That data does not need to be complete either, I think. It should allow coarse pruning, and yes, ideally it would allow choosing the right connectors immediately, but clients still have the opportunity to lease a connector and ask DRM for more details before making the decision. In summary, I don't know if we need to send the whole EDID or just selected standard fields from EDID (make, model, serial, ...), and likewise I'm not sure there is need to send video mode info if that's not useful for choosing a connector/monitor. After the client has chosen a connector and leased it, then it will receive everything it needs directly from the kernel, e.g. a video mode list. > If there was a "leasable connector" object instead of just the connector > event, that could report modes and EDID data as events, like wl_output. > I'm not sure if that is a good idea or unnecessary
Re: [PATCH wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
On Mon, 2018-08-20 at 23:00 +0300, Marius Vlad wrote: [...] > + > + > + This interface is used for managing zwp_kms_lease_v1 object. It is used > + to create a zwp_kms_lease_v1 object (the actual lease object) and also > to > + revoke the lease. > + > + > + > + > +This has no effect on any remaining objects created through this > +interface. > + > + > + > + > + > + Create a lease using the connector output name received in > zwp_kms_lease_manager_v1::connectors. > + Any attempt to use an incorrect connector name will reswult in > zwp_kms_lease_request_v1::failed. > + > + Instead of submitting the connector identification as an argument to the create request, would adding a separate add_connector request be more extensible? That would allow to request a single lease for multiple connectors, or to add a mechanism to request adding overlay planes to the lease. 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
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? > > 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%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 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: 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
On Wed, 2018-08-22 at 09:24 -0700, Keith Packard wrote: > Marius-cristian Vlad writes: > > > > 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 > > Just so you know -- basic mode information isn't sufficient to identify > the target headset. Systems have been attempting to identify the headset > assuming the pixel size of the device was unique, but there are now > headsets reporting 'normal' desktop sizes and so something based on > EDID is likely to be necessary. I think the best way to identify the target headset would be serial number information from EDID, which should match the serial number information from USB for most of them. I have noticed that my Vive doesn't contain serial number information in its EDID block - it is impossible to differentiate between two of those if they are connected at the same time. In that case matching against manufacturer ID and product code from EDID should be good enough for most cases. 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 writes: >> 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 Just so you know -- basic mode information isn't sufficient to identify the target headset. Systems have been attempting to identify the headset assuming the pixel size of the device was unique, but there are now headsets reporting 'normal' desktop sizes and so something based on EDID is likely to be necessary. -- -keith signature.asc Description: PGP signature ___ 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
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 wayland-protocols v3] unstable/drm-lease: DRM lease protocol support
Hi Marius, 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://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=62884cd386b876638720ef88374b31a84ca7ee5f > > 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? 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? > A server/client implementation to match this protocol > can be found at > https://gitlab.freedesktop.org/marius.vlad0/weston/commits/new-drm-lease 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. > Makefile.am | 1 + > unstable/drm-lease/README| 4 + > unstable/drm-lease/drm-lease-unstable-v1.xml | 173 > +++ > 3 files changed, 178 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 6394e26..5d13dca 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -4,6 +4,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/text-input/text-input-unstable-v3.xml > \ > unstable/input-method/input-method-unstable-v1.xml > \ > diff --git a/unstable/drm-lease/README b/unstable/drm-lease/README > new file
