On 2018-02-02 02:32 AM, Pekka Paalanen wrote:
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

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

Seems reasonable to me.

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

(Is it just me or has drm_output_choose_initial_mode's doxy gotten a little stale?)

Not sure I'm ready to think that far ahead yet, but I'll try...

At some point I think it should be possible for all the heads in an output to have completely different resolutions as well as refresh rates. On a windows PC here I can set my 4k monitor and 1920x1200 monitor to be clones, and I can set the resolution to 3840x2160.

This looks poor with black bars top and bottom on the 1920x1200 monitor, but it does work, and I can see some people thinking this is a reasonable way to hook up a projector.

So, I think the weston_output resolution and weston_head resolution should be potentially independent (even when not cloned) of any modelines available from the head.

That makes picking a potential output mode from several heads a serious mess. :)

So, drifting slowly back to some kind of point...

Will simply picking the current mode on the first head be surprising to users? Will it result in situations where switching two monitor cables will cause surprising behaviour?

I don't really know how we should define surprising in this context, but I suppose we should do our best to light up all the displays set as cloned, and also not exit.

I think we should be as lazy as possible in order to get that. I suspect that level of lazy is somewhere between "just use the first head" and "savage stack of broken heuristics that fall apart in the dark corners".

The concept of the mode currently in use for an "output" at weston startup seems somewhat meaningless when an output is actually an internal canvas abstraction and all the heads in that output may have different modes at launch time. :/

(All that said, I'm not sure any of that actually matters in the context of the series currently under review.)


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

Very much appreciated.


wayland-devel mailing list

Reply via email to