[PATCH 01/10] libweston: Add more functionality for handling weston_output objects
Armin Krezović
krezovic.armin at gmail.com
Fri Aug 12 13:33:27 UTC 2016
On 12.08.2016 15:02, Pekka Paalanen wrote:
> 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.
>
Without this, the output could be left partially enabled in weston_output_enable(),
see weston_output_enable() call to output->enable().
That means that (currently) weston_output_init() called from weston_output_enable()
will create a global, repaint timer and some pixman regions. Still, output->enable()
MAY fail for some reason, and we want to return the output that was passed to
weston_output_enable() in a state it was before that. Naturally, the same code needs
to be called on explicit disable, which is why this function has been introduced.
But, as noted in Patch 10 reply, this function won't be doing the thing it currently
is doing (as current weston_output_init() will be gone by then). It can be renamed,
no big deal.
>> +
>> + 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.
>
Hm, yeah ... That's an issue. I probably wanted to say that function
won't continue disabling the output. Maybe we do want a return value
for this function, see below.
> 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.
Hmm ... You are right. Still, weston_output_disable() is called by
backend specific disable function. I fixed this in DRM backend, so
it wouldn't call weston_output_destroy() if a page flip was pending.
I believe we could introduce a special return value for weston_output_disable(),
so weston_output_destroy() knows what to do (or not do).
>
> 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.
Can't we move output restore to drm_output_destroy() then? It would ease
a lot of things.
Also, as a precaution, we could introduce a new field to drm_output
structure that could be set in drm_output_destroy to indicate that
an output should not be turned off. That's just another solution.
>
> 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);
> }
>
I assume this is pseudo code, as this alone won't be enough. Still,
I'd like to keep it this way, otherwise I'd have to further complicate
(and possibly duplicate) backend specific disable/destroy functions.
> 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().
>
That works too. So far, what I've done so far was the best that came
to my mind when I stumbled upon drm and pageflip pending situation.
Before DRM backend port, the code was simpler, yes.
>> + */
>> +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).
>
OK.
>> + */
>> +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.
>
Makes sense.
>> + 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
>
Thanks for the review. I'm not confused, but I don't think I'd like to
further complicate the effort. Everything you outlined above can be done,
but doing so might (don't say that they will) introduce further complications
on the backend side, rather than libweston side.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 855 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160812/27d2e074/attachment.sig>
More information about the wayland-devel
mailing list