[PATCH 01/10] libweston: Add more functionality for handling weston_output objects
Pekka Paalanen
ppaalanen at gmail.com
Fri Aug 12 13:02:14 UTC 2016
On Thu, 11 Aug 2016 17:33:56 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:
> This patch implements additional functionality that will be used
> for configuring, enabling and disabling weston's outputs. Its
> indended use is by the compositors or user programs that want to
> be able to configure, enable or disable an output at any time. An
> output can only be configured while it's disabled.
>
> The compositor and backend specific functionality is required
> for these functions to be useful, and those will come later in
> this series.
>
> All the new functions have been documented, so I'll avoid
> describing them here.
>
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
> libweston/compositor.c | 346 ++++++++++++++++++++++++++++++++++++++++++++-----
> libweston/compositor.h | 29 +++++
> 2 files changed, 340 insertions(+), 35 deletions(-)
Hi Armin,
this patch is one tough nut to review. I've been struggling with it for
over two hours now.
>
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 2eb3a3b..f60ebb8 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -4101,41 +4101,6 @@ weston_compositor_reflow_outputs(struct weston_compositor *compositor,
> }
>
> WL_EXPORT void
> -weston_output_destroy(struct weston_output *output)
> -{
> - struct wl_resource *resource;
> - struct weston_view *view;
> -
> - output->destroying = 1;
> -
> - wl_list_for_each(view, &output->compositor->view_list, link) {
> - if (view->output_mask & (1u << output->id))
> - weston_view_assign_output(view);
> - }
> -
> - wl_event_source_remove(output->repaint_timer);
> -
> - weston_presentation_feedback_discard_list(&output->feedback_list);
> -
> - weston_compositor_reflow_outputs(output->compositor, output, output->width);
> - wl_list_remove(&output->link);
> -
> - wl_signal_emit(&output->compositor->output_destroyed_signal, output);
> - wl_signal_emit(&output->destroy_signal, output);
> -
> - free(output->name);
> - pixman_region32_fini(&output->region);
> - pixman_region32_fini(&output->previous_damage);
> - output->compositor->output_id_pool &= ~(1u << output->id);
> -
> - wl_resource_for_each(resource, &output->resource_list) {
> - wl_resource_set_destructor(resource, NULL);
> - }
> -
> - wl_global_destroy(output->global);
> -}
> -
> -WL_EXPORT void
> weston_output_update_matrix(struct weston_output *output)
> {
> float magnification;
> @@ -4325,6 +4290,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
> output->global =
> wl_global_create(c->wl_display, &wl_output_interface, 3,
> output, bind_output);
> +
> + output->initialized = true;
> }
>
> /** Adds an output to the compositor's output list and
> @@ -4363,6 +4330,309 @@ weston_output_transform_coordinate(struct weston_output *output,
> *y = p.f[1] / p.f[3];
> }
>
> +/** Undoes changes to an output done by weston_output_init()
> + *
> + * \param output The weston_output object that needs the changes undone.
> + *
> + * Removes the repaint timer.
> + * Destroys the Wayland global assigned to the output.
> + * Destroys pixman regions allocated to the output.
> + * Deallocates output's ID and updates compositor's output_id_pool.
> + */
> +static void
> +weston_output_deinit(struct weston_output *output)
> +{
> + if (!output->initialized)
> + return;
> +
> + wl_event_source_remove(output->repaint_timer);
> +
> + wl_global_destroy(output->global);
Destroying the global on deinit instead of disable looks odd...
The same goes for repaint_timer too. It is only needed when the output
is enabled.
> +
> + pixman_region32_fini(&output->region);
> + pixman_region32_fini(&output->previous_damage);
Finalizing these regions is the opposite of
weston_output_init_geometry() which is called from
weston_output_init(). So far so good, but patch 10 should move these
into weston_output_disable(), so that it matches weston_output_enable()
initializing these regions.
> + output->compositor->output_id_pool &= ~(1u << output->id);
I wonder if output id would be needed only while enabled, but that's not
important.
Hmm. The three comments above are actually misguided. I wrote them
assuming the deinit is the opposite of init, but it's not. Deinit is the
opposite of enable, more or less.
> +
> + output->initialized = false;
> +}
> +
> +/** Sets the output scale for a given output.
> + *
> + * \param output The weston_output object that the scale is set for.
> + * \param scale Scale factor for the given output.
> + *
> + * It only supports setting scale for an output that
> + * is not initialized and it can only be ran once.
> + */
> +WL_EXPORT void
> +weston_output_set_scale(struct weston_output *output,
> + int32_t scale)
> +{
> + /* We can only set scale on an uninitialized output */
> + assert(!output->initialized);
> +
> + /* We only want to set scale once */
> + assert(!output->scale);
> +
> + output->scale = scale;
> +}
> +
> +/** Sets the output transform for a given output.
> + *
> + * \param output The weston_output object that the transform is set for.
> + * \param transform Transform value for the given output.
> + *
> + * It only supports setting transform for an output that is
> + * not initialized and it can only be ran once.
> + *
> + * Refer to wl_output::transform section located at
> + * https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_output
> + * for list of values that can be passed to this function.
> + */
> +WL_EXPORT void
> +weston_output_set_transform(struct weston_output *output,
> + uint32_t transform)
> +{
> + /* We can only set transform on an uninitialized output */
> + assert(!output->initialized);
> +
> + /* We only want to set transform once */
> + assert(output->transform == UINT32_MAX);
> +
> + output->transform = transform;
> +}
> +
> +/** Initializes a weston_output object with enough data so
> + ** an output can be configured.
> + *
> + * \param output The weston_output object to initialize
> + * \param compositor The compositor instance.
> + *
> + * Sets initial values for fields that are expected to be
> + * configured either by compositors or backends.
> + *
> + * Notifies the compositor that an output is pending for
> + * configuration.
> + *
> + * Note that this function may end up calling output configure
> + * functions, including weston_output_enable() or weston_output()
> + * disable before it returns.
> + */
> +WL_EXPORT void
> +weston_output_init_pending(struct weston_output *output,
> + struct weston_compositor *compositor)
> +{
> + output->compositor = compositor;
> + output->destroying = 0;
> +
> + /* Add some (in)sane defaults which can be used
> + * for checking if an output was properly configured
> + */
> + output->mm_width = 0;
> + output->mm_height = 0;
> + output->scale = 0;
> + /* Can't use -1 on uint32_t and 0 is valid enum value */
> + output->transform = UINT32_MAX;
> +
> + output->initialized = false;
> +
> + wl_list_init(&output->link);
> +
> + wl_list_insert(compositor->pending_output_list.prev, &output->link);
> + wl_signal_emit(&compositor->output_pending_signal, output);
> +}
> +
> +/* NOTE: Some documentation is copy/pasted from weston_output_init(), as this
> + is intended to replace it. */
> +
> +/** Constructs a weston_output object that can be used by the compositor.
> + *
> + * \param output The weston_output object that needs to be enabled.
> + *
> + * Output coordinates are calculated and each new output is by default
> + * assigned to the right of previous one.
> + *
> + * Sets up the transformation, zoom, and geometry of the output using
> + * the properties that need to be configured by the compositor.
> + *
> + * Establishes a repaint timer for the output with the relevant display
> + * object's event loop. See output_repaint_timer_handler().
> + *
> + * The output is assigned an ID. Weston can support up to 32 distinct
> + * outputs, with IDs numbered from 0-31; the compositor's output_id_pool
> + * is referred to and used to find the first available ID number, and
> + * then this ID is marked as used in output_id_pool.
> + *
> + * The output is also assigned a Wayland global with the wl_output
> + * external interface.
> + *
> + * Backend specific function is called to set up the output output.
> + *
> + * Output is added to the compositor's output list
> + *
> + * If the backend specific function fails, the weston_output object
> + * is returned to a state it was before calling this function and
> + * is added to the compositor's pending_output_list in case it needs
> + * to be reconfigured or just so it can be destroyed at shutdown.
> + *
> + * 0 is returned on success, -1 on failure.
> + */
> +WL_EXPORT int
> +weston_output_enable(struct weston_output *output)
> +{
> + struct weston_output *iterator;
> + int x = 0, y = 0;
> +
> + assert(output->enable);
> +
> + iterator = container_of(output->compositor->output_list.prev,
> + struct weston_output, link);
> +
> + if (!wl_list_empty(&output->compositor->output_list))
> + x = iterator->x + iterator->width;
> +
> + /* Make sure the width and height are configured */
> + assert(output->mm_width && output->mm_height);
> +
> + /* Make sure the scale is set up */
> + assert(output->scale);
> +
> + /* Make sure we have a transform set */
> + assert(output->transform != UINT32_MAX);
> +
> + /* Remove it from pending/disabled output list */
> + wl_list_remove(&output->link);
> +
> + /* TODO: Merge weston_output_init here. */
> + weston_output_init(output, output->compositor, x, y,
> + output->mm_width, output->mm_height,
> + output->transform, output->scale);
> +
> + /* Enable the output (set up the crtc or create a
> + * window representing the output, set up the
> + * renderer, etc)
> + */
> + if (output->enable(output) < 0) {
> + weston_log("Enabling output \"%s\" failed.\n", output->name);
> + weston_output_deinit(output);
> + wl_list_insert(output->compositor->pending_output_list.prev,
> + &output->link);
> + return -1;
> + }
> +
> + weston_compositor_add_output(output->compositor, output);
> +
> + return 0;
> +}
> +
> +/** Converts a weston_output object to a pending output state, so it
> + ** can be configured again or destroyed.
> + *
> + * \param output The weston_output object that needs to be disabled.
> + *
> + * See weston_output_init_pending() for more information on the
> + * state output is returned to.
> + *
> + * Calls a backend specific function to disable an output, in case
> + * such function exists.
> + *
> + * If the output is being used by the compositor, the following happens:
> + *
> + * 1. Presentation feedback is discarded.
> + * 2. Compositor is notified that outputs were changed and
> + * applies the necessary changes.
> + * 3. All views assigned to the weston_output object are
> + * moved to a new output if such exists. Otherwise,
> + * they are marked as dirty and are waiting for a new
> + * output to be assigned.
> + * 4. Signal is emited to notify all users of the weston_output
> + * object that the output is being destroyed.
> + * 5. Resources assigned to an output are destroyed.
> + *
> + * Output is returned to a state it was before weston_output_enable()
> + * was ran. See weston_output_deinit() for more information.
> + *
> + * Output is added to pending_output_list so it will get destroyed
> + * if the output does not get configured again when the compositor
> + * shuts down. If an output is to be used immediately, it needs to
> + * be manually removed from the list.
> + *
> + * If backend specific disable function returns negative value,
> + * this function will return too. It can be used as an indicator
> + * that output cannot be disabled at the present time.
But this function returns void so it cannot return a negative value.
Just remove the paragraph. A libweston user must never need to call
this function more than once. If an output cannot be disabled
completely immediately, libweston (or a backend) should handle the
delayed disablement automatically.
This also points out the problem that if weston_output_disable() is
being called from weston_output_destroy(), it must not postpone, but we
have no way to ensure that.
Also, the backend disable() hook must not be called as a part of
weston_output_destroy() call chain, because the DRM backend is supposed
to restore the original DRM output state. Right now what happens is
that the DRM backend restores the output state, then calls
weston_output_destroy(), which then ends up calling backend's disable()
which would turn the output off.
That is why calling weston_output_disable() from
weston_output_destroy() does not work. I think you need to split a new
function out of weston_output_disable() that you can then call from
weston_output_destroy(). That function would probably be
weston_output_enable_undo() I mention for patch 10.
I think weston_output_disable() should look more or less like this:
{
output->destroying = 1;
if (output->disable)
if (output->disable(output) < 0)
return; /* the backend will retry */
if (output->enabled)
weston_output_enable_undo(output);
}
That means lots of the other code needs rearrangement and how to keep
the old weston_output_init() working until all backends are converted
is yet another complication.
One important detail here is that if output->disable() returns -1
meaning that it needs to be postponed, the backend guarantees to call
weston_output_disable() again. I did say I don't like retries like
that, but maybe it's fine here. The alternative is to let the backend
internally call its own disable() again and let it also call
weston_output_enable_undo().
> + */
> +WL_EXPORT void
> +weston_output_disable(struct weston_output *output)
> +{
> + struct wl_resource *resource;
> + struct weston_view *view;
> +
> + /* Should we rename this? */
> + output->destroying = 1;
> +
> + if (output->disable)
> + if (output->disable(output) < 0)
> + return;
> +
> + /* It's initialized on enable, so it can be asumed it is
> + being used */
> + if (output->initialized) {
> + wl_list_for_each(view, &output->compositor->view_list, link) {
> + if (view->output_mask & (1u << output->id))
> + weston_view_assign_output(view);
> + }
> +
> + weston_presentation_feedback_discard_list(&output->feedback_list);
> +
> + weston_compositor_reflow_outputs(output->compositor, output, output->width);
> +
> + wl_signal_emit(&output->compositor->output_destroyed_signal, output);
> + wl_signal_emit(&output->destroy_signal, output);
> +
> + wl_resource_for_each(resource, &output->resource_list) {
> + wl_resource_set_destructor(resource, NULL);
> + }
> + }
> +
> + wl_list_remove(&output->link);
> +
> + weston_output_deinit(output);
> +
> + output->destroying = 0;
> +
> + /* We need to preserve it somewhere so it can be destroyed on shutdown
> + if nobody wants to configure it again */
> + wl_list_insert(output->compositor->pending_output_list.prev, &output->link);
> +}
> +
> +/** Emits a signal to indicate that there is an output waiting to be configured.
Any number of outputs, really.
> + *
> + * \param compositor The compositor instance
> + *
> + * This re-emits a signal for each pending output in case the listener was
> + * set up after the signals were intially sent.
This is incorrect: it emits the signals for the currently pending
outputs regardless when the listeners were set.
The documentation should just say: emits output_pending_signal for each
output currently pending (connected but unused).
> + */
> +WL_EXPORT void
> +weston_pending_output_coldplug(struct weston_compositor *compositor)
> +{
> + struct weston_output *output, *next;
> +
> + wl_list_for_each_safe(output, next, &compositor->pending_output_list, link)
> + wl_signal_emit(&compositor->output_pending_signal, output);
> +}
> +
> +WL_EXPORT void
> +weston_output_destroy(struct weston_output *output)
> +{
> + weston_output_disable(output);
This should perhaps be:
if (output->enabled)
weston_output_enable_undo(output);
> +
> + output->destroying = 1;
> +
> + wl_list_remove(&output->link);
> +
> + free(output->name);
> +}
> +
> static void
> destroy_viewport(struct wl_resource *resource)
> {
> @@ -4700,6 +4970,7 @@ weston_compositor_create(struct wl_display *display, void *user_data)
> wl_signal_init(&ec->hide_input_panel_signal);
> wl_signal_init(&ec->update_input_panel_signal);
> wl_signal_init(&ec->seat_created_signal);
> + wl_signal_init(&ec->output_pending_signal);
> wl_signal_init(&ec->output_created_signal);
> wl_signal_init(&ec->output_destroyed_signal);
> wl_signal_init(&ec->output_moved_signal);
> @@ -4735,6 +5006,7 @@ weston_compositor_create(struct wl_display *display, void *user_data)
> wl_list_init(&ec->plane_list);
> wl_list_init(&ec->layer_list);
> wl_list_init(&ec->seat_list);
> + wl_list_init(&ec->pending_output_list);
> wl_list_init(&ec->output_list);
> wl_list_init(&ec->key_binding_list);
> wl_list_init(&ec->modifier_binding_list);
> @@ -4779,6 +5051,10 @@ weston_compositor_shutdown(struct weston_compositor *ec)
> wl_list_for_each_safe(output, next, &ec->output_list, link)
> output->destroy(output);
>
> + /* Destroy all pending outputs associated with this compositor */
> + wl_list_for_each_safe(output, next, &ec->pending_output_list, link)
> + output->destroy(output);
> +
> if (ec->renderer)
> ec->renderer->destroy(ec);
>
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index 0133084..0cb04ad 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -240,6 +240,12 @@ struct weston_output {
> uint16_t *b);
>
> struct weston_timeline_object timeline;
> +
> + bool initialized;
This should probably be called "enabled" instead, because after patch
10, it means that weston_output_enable() has succeeded.
> + int scale;
> +
> + int (*enable)(struct weston_output *output);
> + int (*disable)(struct weston_output *output);
> };
>
> enum weston_pointer_motion_mask {
> @@ -752,6 +758,7 @@ struct weston_compositor {
> struct wl_signal update_input_panel_signal;
>
> struct wl_signal seat_created_signal;
> + struct wl_signal output_pending_signal;
> struct wl_signal output_created_signal;
> struct wl_signal output_destroyed_signal;
> struct wl_signal output_moved_signal;
> @@ -763,6 +770,7 @@ struct weston_compositor {
> struct weston_layer fade_layer;
> struct weston_layer cursor_layer;
>
> + struct wl_list pending_output_list;
> struct wl_list output_list;
> struct wl_list seat_list;
> struct wl_list layer_list;
> @@ -1793,6 +1801,27 @@ weston_seat_set_keyboard_focus(struct weston_seat *seat,
> int
> weston_compositor_load_xwayland(struct weston_compositor *compositor);
>
> +void
> +weston_output_set_scale(struct weston_output *output,
> + int32_t scale);
> +
> +void
> +weston_output_set_transform(struct weston_output *output,
> + uint32_t transform);
> +
> +void
> +weston_output_init_pending(struct weston_output *output,
> + struct weston_compositor *compositor);
> +
> +int
> +weston_output_enable(struct weston_output *output);
> +
> +void
> +weston_output_disable(struct weston_output *output);
> +
> +void
> +weston_pending_output_coldplug(struct weston_compositor *compositor);
> +
> #ifdef __cplusplus
> }
> #endif
Looking only at the header changes, the API looks quite fine. It's just
the internal details of which function undoes what, and what the call
chain is, is tangled.
After all that, are you left completely confused or are you perhaps
seeing a way to reorder things?
I suspect we are really close to a working design, but there are a few
details like naming and docs that throw me off, and the fact that patch
10 actually changes what weston_output_init() is and the docs don't
seem to follow.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160812/2cc1355c/attachment-0001.sig>
More information about the wayland-devel
mailing list