Hi Marius, On 25 January 2018 at 11:10, Marius-cristian Vlad <marius-cristian.v...@nxp.com> wrote: > >> +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.
If you're setting a flag in drm_output (see below), it might be easier to pass a pointer to the drm_output, and then take a list of drm_planes? > >> + if (!choosen_output) { > >> + weston_log("No valid output found to lease!\n"); Oh also, as another matter of general principle: until we have useful debugging logs, please don't log things which can be provoked by the client, like this. If you want to get more verbose information out, adding a string parameter to the 'failed' event would let you communicate errors back down to the client, which you could also see via the WAYLAND_DEBUG logs. > >> + weston_log("Lease leased_id = %u created, on output %s\n", > >> + lease->leased_id, choosen_output->base.name); Logging for failure definitely seems too verbose as well. ;) > >> + wl_list_insert(&leases, &lease->list); > >> + drm_output_destroy(&choosen_output->base); > >> + > >> + zwp_drm_lease_v1_send_created(resource, lease->leased_fd, > >> + lease->leased_id); } > > > > Rather than destroying the output, I'd be much more comfortable marking it > > as disabled or so. There is also a race here: in the DRM backend, output > > destruction can be deferred, when a pageflip has not yet completed. > > [mvlad] Assuming that you are referring to weston_output_disable, the > backends output disable callback > -- drm_output_disable will eventually disable the output. With the output > disabled the client will no longer be able to use the connector. It could > also be that my testing version of the client assumes that the output will > not be disabled (hence forth > it can question the leased fd for connector/crtc/plane). From my tests > destroying the output seem to be the most reliable. Right. What I mean is that if we schedule a pageflip from Weston and a client requests a lease before the pageflip completes, then drm_output_destroy() will perform a delayed destroy, potentially shutting down the CRTC. This can happen _after_ the resource has been leased to the client, which would probably be quite confusing. When I said 'marking it as disabled or so', what I meant was below: a flag in the drm_output struct which notes that the output has been leased to a client and should not be used. I think the most robust way would be if we did this (so the DRM compositor would know not to try to use the output), and also allowed lease creation to be deferred. This would allow us to only give the lease to the client after Weston has cleaned up all its use of those resources; it would also potentially allow other users to call out to an external policy manager (which may not be immediate) to determine if the lease should be allowed, and help prevent race conditions when two clients are trying to hand over a lease between themselves. > >> + wl_signal_emit(&compositor->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. Thanks again! Cheers, Daniel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel