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

[Pekka Paalanen]

On Thu, 25 Jan 2018 19:07:43 +
Marius-cristian Vlad <marius-cristian.v...@nxp.com> wrote:

> -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 :/.  

Hi,

we can specify the protocol any way we want. If we specify that clients
must follow a certain sequence, we can expect them to follow that
sequence.

Sending the "release the lease" request, i.e. destroying the Wayland
protocol object representing the lease, is independent of actually
closing the DRM fd. If the leased DRM fd is revoked by the server,
would it mean that the lessee can no longer use the still open fd to
destroy FBs?

But, there must be a clean-up mechanism in the kernel as well, so maybe
there is no need for the lessee to explicitly destroy the last FB as
closing the DRM fd and subsequently the server setting its own FB to
the CRTC should cause the now orphaned lessee FB to be
garbage-collected.

For client initiated release, we could require the sequence: client
sends the release the lease request, client waits for the server to
process the release (produces an event), only then client closes the
DRM fd. The client would never rmfb the last FB.

My question is: is there any need for that? Would it make sense or not?

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

Right.

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

I would assume that's just a race, or a side-effect of the delayed
drm_output destruction as Daniel explained. I presume that in fact the
CRTC is still running with the server FB when the lessee sets its own.
I would expect there to be flicker if you actually guaranteed the
server's FB has been destroyed and the destruction handled by the
kernel (cross a vblank), before giving the lease to the client.

I believe one should guar

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.

[Pekka Paalanen]

On Thu, 25 Jan 2018 11:33:34 +
Daniel Stone  wrote:

> Hi Marius,
> 
> On 25 January 2018 at 11:10, Marius-cristian Vlad
>  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?

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?

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?

What about leaking fb contents between the lessor and lessee?

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?


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.

[Daniel Stone]

Hi Marius,

On 25 January 2018 at 11:10, Marius-cristian Vlad
 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(, >list);
> >> +   drm_output_destroy(_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(>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

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 weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Daniel Stone]

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.

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

On 24 January 2018 at 19:11, Marius Vlad  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.

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

This should be under drm_backend.

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

> +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'.

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

> +#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.

> +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;
> +   }
> +   }
> +
> +   if (!choosen_output) {
> +   weston_log("No valid output found to lease!\n");
> +   zwp_drm_lease_v1_send_failed(resource);
> +   return;
> +   }
> +
> +   drm_lease_save_objects(, objects, );
> +   lease = zalloc(sizeof(*lease));
> +
> +   /* create lease */
> +   lease->leased_fd = drmModeCreateLease(drm_fd, objects, nobjects, 0,
> + >leased_id);
> +   if (lease->leased_fd < 0) {
> +   weston_log("Failed to create the lease!");
> +   free(lease);
> +   zwp_drm_lease_v1_send_failed(resource);
> +   return;
> +   }
> +
> +   weston_log("Lease leased_id = %u created, on output %s\n",
> +  lease->leased_id, choosen_output->base.name);
> +
> +   wl_list_insert(, >list);
> +   drm_output_destroy(_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.

> +static void
> +drm_lease_revoke(struct wl_client *client, struct wl_resource *resource, 
> uint32_t id)
> +{
> +   struct drm_lease *lease,