[PATCH weston v5 06/36] libweston: introduce weston_output::head_list

Fri Feb 2 08:32:33 UTC 2018

On Thu, 1 Feb 2018 15:36:55 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 2017-12-14 05:40 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > The intention is that in the future backends will dynamically allocate
> > weston_heads based on the resources they have. The lifetime of a
> > weston_head will be independent of the lifetime of a weston_output it
> > may be attached to. Backends allocate objects derived from weston_head,
> > like they currently do for weston_output. Backend will choose when to
> > destroy a weston_head.
> > 
> > For clone mode, struct weston_output gains head_list member, which is
> > the list of attached heads that will all show the same framebuffer.
> > Since heads are growing out of weston_output, management functions are
> > added.
> > 
> > Detaching a head from an enabled output is allowed to accommodate
> > disappearing heads. Attaching a head to an enabled output is disallowed
> > because it may need hardware reconfiguration and testing, and so
> > requires a weston_output_enable() call.
> > 
> > As a temporary measure, we have one weston_head embedded in
> > weston_output, so that backends can be migrated individually to the new
> > allocation scheme.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >   libweston/compositor.c | 216 +++++++++++++++++++++++++++++++++++++++----------
> >   libweston/compositor.h |   7 +-
> >   2 files changed, 181 insertions(+), 42 deletions(-)
> > 
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index c668aa28..efa961dc 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c

> > @@ -4386,6 +4389,98 @@ weston_head_from_resource(struct wl_resource *resource)
> >   	return wl_resource_get_user_data(resource);
> >   }
> >   
> > +/** Initialize a pre-allocated weston_head
> > + *
> > + * \param head The head to initialize.
> > + *
> > + * The head will be safe to attach, detach and release.
> > + *
> > + * \memberof weston_head
> > + * \internal
> > + */
> > +static void
> > +weston_head_init(struct weston_head *head)
> > +{
> > +	/* Add some (in)sane defaults which can be used
> > +	 * for checking if an output was properly configured
> > +	 */
> > +	memset(head, 0, sizeof *head);
> > +
> > +	wl_list_init(&head->output_link);
> > +	wl_list_init(&head->resource_list);
> > +}
> > +
> > +/** Attach a head to an inactive output
> > + *
> > + * \param output The output to attach to.
> > + * \param head The head that is not yet attached.
> > + * \return 0 on success, -1 on failure.
> > + *
> > + * Attaches the given head to the output. All heads of an output are clones
> > + * and share the resolution and timings.
> > + *
> > + * Cloning heads this way uses less resources than creating an output for
> > + * each head, but is not always possible due to environment, driver and hardware
> > + * limitations.
> > + *
> > + * On failure, the head remains unattached. Success of this function does not
> > + * guarantee the output configuration is actually valid. The final checks are
> > + * made on weston_output_enable().
> > + *
> > + * \memberof weston_output
> > + */
> > +static int
> > +weston_output_attach_head(struct weston_output *output,
> > +			  struct weston_head *head)
> > +{
> > +	if (output->enabled)
> > +		return -1;  
> 
> Would it be reasonable to make !output->enabled an assert()?

I forget if I actually had it like that, but then this function later
becomes public API and I preferred to return an error rather than BOOM.

It happens in "libweston: new head-based output management API".

Is that ok?

> > +
> > +	if (!wl_list_empty(&head->output_link))
> > +		return -1;
> > +
> > +	/* XXX: no support for multi-head yet */
> > +	if (!wl_list_empty(&output->head_list))
> > +		return -1;
> > +
> > +	head->output = output;
> > +	wl_list_insert(output->head_list.prev, &head->output_link);
> > +
> > +	return 0;
> > +}

> > @@ -5042,6 +5148,8 @@ weston_pending_output_coldplug(struct weston_compositor *compositor)
> >   WL_EXPORT void
> >   weston_output_release(struct weston_output *output)
> >   {
> > +	struct weston_head *head;
> > +
> >   	output->destroying = 1;
> >   
> >   	if (output->enabled)
> > @@ -5050,9 +5158,35 @@ weston_output_release(struct weston_output *output)
> >   	pixman_region32_fini(&output->region);
> >   	pixman_region32_fini(&output->previous_damage);
> >   	wl_list_remove(&output->link);
> > +
> > +	while (!wl_list_empty(&output->head_list)) {
> > +		head = weston_output_get_first_head(output);
> > +		weston_head_detach(head);
> > +	}  
> 
> I feel like I'm missing something here, but... this function looks 
> multi-head aware, but depends on the "hacky" 
> weston_output_get_first_head() function?  Should this just be turned 
> into a regular for_each_safe?
> 
> It seems like at the end of the series we're left with 4 callers to 
> weston_output_get_first_head()...  The cms-colord site seems potentially 
> non-trivial to resolve.  What else still needs to be made multi-head 
> aware before the function can be removed?

Yes, that's a good point. This site is actually the only legit use case
for weston_output_get_first_head(). I agree it is somewhat confusing
and is a result of lazyness. Will fix.

Aside from cms-colord.c, in the full clonemode series (this is a
partial one), the only other use of weston_output_get_first_head() is
in compositor-drm.c drm_output_set_mode().

In drm_output_set_mode(), the first head is used to get the "inherited
mode", that is, the video mode on the output before weston took over.
This video mode is fed into drm_output_choose_initial_mode(). The
trouble is, if we are enabling for the first time an output with
multiple heads, which head's original mode should be used? I suppose
you'd filter out any heads that didn't have any mode set, but how to
pick then? So I went for the easy way out for now.

> In any event, everything up to this point is still:
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

Very much appreciated.

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180202/f18e4957/attachment-0001.sig>