[PATCH weston v5 14/36] libweston: new head-based output management API

[Derek Foreman]

On 2018-02-05 05:23 AM, Pekka Paalanen wrote:
> On Fri, 2 Feb 2018 15:00:09 -0600
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> On 2017-12-14 05:40 AM, Pekka Paalanen wrote:
>>> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>>
>>> Introduce the API for users (compositors) to create an output from a
>>> head, attach and detach heads, and destroy outputs created this way.
>>> This also adds the backend-facing API to libweston.
>>>
>>> In the new API design, a backend creates heads, and the compositor
>>> chooses one or more heads (clone mode) to be driven by an output.
>>> In the future backends will be converted to not create outputs directly
>>> but only in the new create_output hook.
>>>
>>> The user subscribes to a heads_changed hook and arranges heads into
>>> outputs from there.
>>>
>>> Adding the API this way will allow frontends (main.c) and backends to be
>>> converted one by one. This adds compatiblity paths in
>>> weston_compositor_create_output_with_head() and weston_output_destroy()
>>> so that frontends can be converted first to call these, and then
>>> backends can be converted one by one to the new design. Afterwards, the
>>> compatibility paths will be removed along with weston_output::head.
>>>
>>> Currently heads can be added to a disabled output only. This is less
>>> than ideal for clone mode hotplug and should be improved on later.
>>>
>>> v4: Remove the wl_output global on head detach if output is enabled.
>>>
>>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>> ---
>>>    libweston/compositor.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++---
>>>    libweston/compositor.h |  78 +++++++++++++++++++++
>>>    2 files changed, 256 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libweston/compositor.c b/libweston/compositor.c
> 
>>> @@ -4583,7 +4629,7 @@ weston_compositor_iterate_heads(struct weston_compositor *compositor,
>>>     *
>>>     * \memberof weston_output
>>>     */
>>> -static int
>>> +WL_EXPORT int
>>>    weston_output_attach_head(struct weston_output *output,
>>>    			  struct weston_head *head)
>>>    {
>>> @@ -4593,9 +4639,13 @@ weston_output_attach_head(struct weston_output *output,
>>>    	if (!wl_list_empty(&head->output_link))
>>>    		return -1;
>>>    
>>> -	/* XXX: no support for multi-head yet */
>>> -	if (!wl_list_empty(&output->head_list))
>>> +	if (output->attach_head) {
>>> +		if (output->attach_head(output, head) < 0)
>>> +			return -1;
>>> +	} else if (!wl_list_empty(&output->head_list)) {
>>> +		/* No support for clones in the legacy path. */
>>>    		return -1;
>>> +	}
>>>    
>>>    	head->output = output;
>>>    	wl_list_insert(output->head_list.prev, &head->output_link);
> 
>>> +/** Create an output for an unused head
>>> + *
>>> + * \param compositor The compositor.
>>> + * \param head The head to attach to the output.
>>> + * \return A new \c weston_output, or NULL on failure.
>>> + *
>>> + * This creates a new weston_output that starts with the given head attached.
>>> + * The output inherits the name of the head. The head must not be already
>>> + * attached to another output.
>>> + *
>>> + * An output must be configured before it can be enabled.
>>> + *
>>> + * \memberof weston_compositor
>>> + */
>>> +WL_EXPORT struct weston_output *
>>> +weston_compositor_create_output_with_head(struct weston_compositor *compositor,
>>> +					  struct weston_head *head)
>>> +{
>>> +	struct weston_output *output;
>>> +
>>> +	if (head->allocator_output) {
>>> +		/* XXX: compatibility path to be removed after all converted */
>>> +		output = head->allocator_output;
>>> +	} else {
>>> +		assert(compositor->backend->create_output);
>>> +		output = compositor->backend->create_output(compositor,
>>> +							    head->name);
>>> +	}
>>> +
>>> +	if (!output)
>>> +		return NULL;
>>> +
>>> +	if (weston_output_attach_head(output, head) < 0) {
>>> +		if (!head->allocator_output)
>>> +			output->destroy(output);
>>> +
>>> +		return NULL;
>>> +	}
>>> +
>>> +	return output;
>>> +}
>>> +
>>> +/** Destroy an output
>>> + *
>>> + * \param output The output to destroy.
>>> + *
>>> + * The heads attached to the given output are detached and become unused again.
>>> + *
>>> + * It is not necessary to explicitly destroy all outputs at compositor exit.
>>> + * weston_compositor_destroy() will automatically destroy any remaining
>>> + * outputs.
>>> + *
>>> + * \memberof weston_output
>>> + */
>>> +WL_EXPORT void
>>> +weston_output_destroy(struct weston_output *output)
>>> +{
>>> +	struct weston_head *head;
>>> +
>>> +	/* XXX: compatibility path to be removed after all converted */
>>> +	head = weston_output_get_first_head(output);
>>
>> This took me a couple of reads...  if I understand correctly, the full
>> list of outputs is going to be torn down in the backend
>> output->destroy()?  And we hit that path if we don't have the legacy
>> allocator_output.
> 
> Yes, the backend will call weston_output_release() which will detach
> all attached heads.
> 
> In the legacy backend case, the backend is in charge of creating and
> destroying weston_outputs, so we cannot let weston_output_destroy()
> call output->destroy(). With legacy frontend things are as they were,
> but a migrated frontend will call weston_output_destroy().
> 
> In the migrated backend case, weston_outputs are created and destroyed
> by the frontend's command.
> 
> If we have a migrated frontend and a legacy backend, we just pretend
> the frontend is in charge of the weston_output lifetime.
> 
> It is enough to check only the first head for the legacy backend case,
> because in that case there cannot be more heads attached.
> weston_output_attach_head() ensures that.
> 
>> I think the code all looks sound and the API looks reasonable to me, but
>> I have to concede that I don't know the output handling path enough to
>> understand weston_compositor_reflow_output's setting up of the
>> allocator_output.
> 
> Why do you mention weston_compositor_reflow_outputs()?

Wow, for really embarrassing reasons actually.

> allocator_output is a note in weston_head to say we have a legacy
> backend that created a weston_output automatically. So when a migrated
> frontend goes to weston_compositor_create_output_with_head() to create
> a new weston_output, we can return the (pending) weston_output that
> already exists.

That made sense to me...

>> Acked-by: Derek Foreman <derekf at osg.samsung.com>

After re-reading the patch a couple of times I think I can upgrade this to
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

Thanks,
Derek
> 
> Thanks,
> pq
> 
>>> +	if (head->allocator_output) {
>>> +		/* The old design: backend is responsible for destroying the
>>> +		 * output, so just undo create_output_with_head()
>>> +		 */
>>> +		weston_head_detach(head);
>>> +		return;
>>> +	}
>>> +
>>> +	output->destroy(output);
>>> +}
>>> +
>>>    /** When you need a head...
>>>     *
>>>     * This function is a hack, used until all code has been converted to become
>>> diff --git a/libweston/compositor.h b/libweston/compositor.h
>>> index f7b7050c..f9d034c3 100644
>>> --- a/libweston/compositor.h
>>> +++ b/libweston/compositor.h
>>> @@ -172,6 +172,8 @@ struct weston_head {
>>>    
>>>    	char *name;			/**< head name, e.g. connector name */
>>>    	bool connected;			/**< is physically connected */
>>> +
>>> +	struct weston_output *allocator_output;	/**< XXX: to be removed */
>>>    };