[PATCH 01/10] libweston: Add more functionality for handling weston_output objects

[Pekka Paalanen]

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.

> 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: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160812/7337830d/attachment.sig>