[PATCH weston v5 06/36] libweston: introduce weston_output::head_list

[Derek Foreman]

On 2018-02-02 02:32 AM, Pekka Paalanen wrote:
> On Thu, 1 Feb 2018 15:36:55 -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>
>>>
>>> The intention is that in the future backends will dynamically allocate
>>> weston_heads based on the resources they have. The lifetime of a
>>> weston_head will be independent of the lifetime of a weston_output it
>>> may be attached to. Backends allocate objects derived from weston_head,
>>> like they currently do for weston_output. Backend will choose when to
>>> destroy a weston_head.
>>>
>>> For clone mode, struct weston_output gains head_list member, which is
>>> the list of attached heads that will all show the same framebuffer.
>>> Since heads are growing out of weston_output, management functions are
>>> added.
>>>
>>> Detaching a head from an enabled output is allowed to accommodate
>>> disappearing heads. Attaching a head to an enabled output is disallowed
>>> because it may need hardware reconfiguration and testing, and so
>>> requires a weston_output_enable() call.
>>>
>>> As a temporary measure, we have one weston_head embedded in
>>> weston_output, so that backends can be migrated individually to the new
>>> allocation scheme.
>>>
>>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>> ---
>>>    libweston/compositor.c | 216 +++++++++++++++++++++++++++++++++++++++----------
>>>    libweston/compositor.h |   7 +-
>>>    2 files changed, 181 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>>> index c668aa28..efa961dc 100644
>>> --- a/libweston/compositor.c
>>> +++ b/libweston/compositor.c
> 
>>> @@ -4386,6 +4389,98 @@ weston_head_from_resource(struct wl_resource *resource)
>>>    	return wl_resource_get_user_data(resource);
>>>    }
>>>    
>>> +/** Initialize a pre-allocated weston_head
>>> + *
>>> + * \param head The head to initialize.
>>> + *
>>> + * The head will be safe to attach, detach and release.
>>> + *
>>> + * \memberof weston_head
>>> + * \internal
>>> + */
>>> +static void
>>> +weston_head_init(struct weston_head *head)
>>> +{
>>> +	/* Add some (in)sane defaults which can be used
>>> +	 * for checking if an output was properly configured
>>> +	 */
>>> +	memset(head, 0, sizeof *head);
>>> +
>>> +	wl_list_init(&head->output_link);
>>> +	wl_list_init(&head->resource_list);
>>> +}
>>> +
>>> +/** Attach a head to an inactive output
>>> + *
>>> + * \param output The output to attach to.
>>> + * \param head The head that is not yet attached.
>>> + * \return 0 on success, -1 on failure.
>>> + *
>>> + * Attaches the given head to the output. All heads of an output are clones
>>> + * and share the resolution and timings.
>>> + *
>>> + * Cloning heads this way uses less resources than creating an output for
>>> + * each head, but is not always possible due to environment, driver and hardware
>>> + * limitations.
>>> + *
>>> + * On failure, the head remains unattached. Success of this function does not
>>> + * guarantee the output configuration is actually valid. The final checks are
>>> + * made on weston_output_enable().
>>> + *
>>> + * \memberof weston_output
>>> + */
>>> +static int
>>> +weston_output_attach_head(struct weston_output *output,
>>> +			  struct weston_head *head)
>>> +{
>>> +	if (output->enabled)
>>> +		return -1;
>>
>> Would it be reasonable to make !output->enabled an assert()?
> 
> I forget if I actually had it like that, but then this function later
> becomes public API and I preferred to return an error rather than BOOM.
> 
> It happens in "libweston: new head-based output management API".
> 
> Is that ok?

Seems reasonable to me.

>>> +
>>> +	if (!wl_list_empty(&head->output_link))
>>> +		return -1;
>>> +
>>> +	/* XXX: no support for multi-head yet */
>>> +	if (!wl_list_empty(&output->head_list))
>>> +		return -1;
>>> +
>>> +	head->output = output;
>>> +	wl_list_insert(output->head_list.prev, &head->output_link);
>>> +
>>> +	return 0;
>>> +}
> 
> 
>>> @@ -5042,6 +5148,8 @@ weston_pending_output_coldplug(struct weston_compositor *compositor)
>>>    WL_EXPORT void
>>>    weston_output_release(struct weston_output *output)
>>>    {
>>> +	struct weston_head *head;
>>> +
>>>    	output->destroying = 1;
>>>    
>>>    	if (output->enabled)
>>> @@ -5050,9 +5158,35 @@ weston_output_release(struct weston_output *output)
>>>    	pixman_region32_fini(&output->region);
>>>    	pixman_region32_fini(&output->previous_damage);
>>>    	wl_list_remove(&output->link);
>>> +
>>> +	while (!wl_list_empty(&output->head_list)) {
>>> +		head = weston_output_get_first_head(output);
>>> +		weston_head_detach(head);
>>> +	}
>>
>> I feel like I'm missing something here, but... this function looks
>> multi-head aware, but depends on the "hacky"
>> weston_output_get_first_head() function?  Should this just be turned
>> into a regular for_each_safe?
>>
>> It seems like at the end of the series we're left with 4 callers to
>> weston_output_get_first_head()...  The cms-colord site seems potentially
>> non-trivial to resolve.  What else still needs to be made multi-head
>> aware before the function can be removed?
> 
> Yes, that's a good point. This site is actually the only legit use case
> for weston_output_get_first_head(). I agree it is somewhat confusing
> and is a result of lazyness. Will fix.
> 
> Aside from cms-colord.c, in the full clonemode series (this is a
> partial one), the only other use of weston_output_get_first_head() is
> in compositor-drm.c drm_output_set_mode().
> 
> In drm_output_set_mode(), the first head is used to get the "inherited
> mode", that is, the video mode on the output before weston took over.
> This video mode is fed into drm_output_choose_initial_mode(). The
> trouble is, if we are enabling for the first time an output with
> multiple heads, which head's original mode should be used? I suppose
> you'd filter out any heads that didn't have any mode set, but how to
> pick then? So I went for the easy way out for now.

(Is it just me or has drm_output_choose_initial_mode's doxy gotten a 
little stale?)

Not sure I'm ready to think that far ahead yet, but I'll try...

At some point I think it should be possible for all the heads in an 
output to have completely different resolutions as well as refresh 
rates.  On a windows PC here I can set my 4k monitor and 1920x1200 
monitor to be clones, and I can set the resolution to 3840x2160.

This looks poor with black bars top and bottom on the 1920x1200 monitor, 
but it does work, and I can see some people thinking this is a 
reasonable way to hook up a projector.

So, I think the weston_output resolution and weston_head resolution 
should be potentially independent (even when not cloned) of any 
modelines available from the head.

That makes picking a potential output mode from several heads a serious 
mess. :)

So, drifting slowly back to some kind of point...

Will simply picking the current mode on the first head be surprising to 
users?  Will it result in situations where switching two monitor cables 
will cause surprising behaviour?

I don't really know how we should define surprising in this context, but 
I suppose we should do our best to light up all the displays set as 
cloned, and also not exit.

I think we should be as lazy as possible in order to get that.  I 
suspect that level of lazy is somewhere between "just use the first 
head" and "savage stack of broken heuristics that fall apart in the dark 
corners".

The concept of the mode currently in use for an "output" at weston 
startup seems somewhat meaningless when an output is actually an 
internal canvas abstraction and all the heads in that output may have 
different modes at launch time. :/

(All that said, I'm not sure any of that actually matters in the context 
of the series currently under review.)

Thanks,
Derek

>> In any event, everything up to this point is still:
>> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> 
> Very much appreciated.
> 
> 
> Thanks,
> pq
>