[PATCH 01/10] libweston: Add more functionality for handling weston_output objects
Armin Krezović
krezovic.armin at gmail.com
Fri Aug 12 16:17:38 UTC 2016
On 12.08.2016 18:10, Pekka Paalanen wrote:
> On Fri, 12 Aug 2016 15:33:27 +0200
> Armin Krezović <krezovic.armin at gmail.com> wrote:
>
>> 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(-)
>
>>>> +/** 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.
>
> Right.
>
>
>>>> +/** 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.
>
> No, it shouldn't return a value. weston_output_disable() is an API
> intended foremost to the compositor. A compositor won't do anything
> with a return value, nor can it probably do anything to recover from a
> failed disabling, and it certainly should not have to deal with retries.
>
> This is the "master API" towards the compositor. You call it once and
> the output gets disabled, period. If hw disabling needs postponing or
> something, that's a libweston or backend internal detail that the
> compositor should not care about.
>
> Just make sure that an output in the process of being disabled is not
> added back to the pending_output_list until it actually is disabled, if
> it was previously enabled.
>
>>> 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 had not imagined such a change. That sounds bad as you have a
> recursion weston_output_disable() -> output->disable() ->
> weston_output_disable() -> output->disable() ...
>
> I would not expect to see recursion there, it's confusing.
>
Ooops, hang on. I didn't get that right. There's no recursion.
It should have said "weston_output_destroy() is called by backend specific destroy
function". The second part of the sentence above speaks about weston_output_destroy().
>> 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.
>
> It is already there to begin with. Unless your DRM porting patch moved it...
>
> Yes, it seems the DRM backend in upstream is not fully dealing with a
> pending pageflip while tearing down an output. Unfortunately, there are
> two different cases where that might happen:
>
> - Output hot-unplug; in this case the compositor main loop keeps
> running and we can just return and wait for the page flip event to
> arrive, and finish the work there.
>
> - Compositor shutdown; the main loop has already been stopped, we must
> finish with tearing down the output right now. IOW, if we need to
> wait for something, we need to block right there. The main loop won't
> deliver any more callbacks.
>
> The DRM backend just raises its hands and hopes for the final page flip
> event which I am not sure will ever come.
>
>> 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.
>
> When should it not be... oh, you're probably talking about your ported
> code.
>
> That reminds me of an another complication, but I think we have enough
> already. :-)
>
>>> 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.
>
> Keep it which way?
>
> There is no duplicate, there is just refactoring into smaller
> 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.
>
> Right. Dealing with the delayed destruction or disabling is a pain.
>
>>>> + */
>>>> +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);
>>>> +}
>>>> +
>
>>> 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 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.
>
> We shall see. We did have a nice irc chat. :-)
>
> I think that if a backend needs its "disable" called also for "destroy",
> it should do it itself. If the core does it, you get in trouble if a
> backend does not want it. It is more straightforward to do what you
> want, rather than work around the core doing something you don't want,
> even if it's a few more lines of code.
>
>
> Thanks,
> pq
>
-------------- 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/e7bc5abf/attachment-0001.sig>
More information about the wayland-devel
mailing list