On Thu, 1 Feb 2018 15:36:55 -0600 Derek Foreman <der...@osg.samsung.com> wrote:
> On 2017-12-14 05:40 AM, Pekka Paalanen wrote: > > From: Pekka Paalanen <pekka.paala...@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.paala...@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 <der...@osg.samsung.com> Very much appreciated. Thanks, pq
pgpa0RcaR6xyW.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel