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
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?
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
*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.
(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.)
Thanks,
Derek
In any event, everything up to this point is still:
Reviewed-by: Derek Foreman <der...@osg.samsung.com>
Very much appreciated.
Thanks,
pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
