[PATCH weston v5 16/36] libweston: add weston_head_is_device_changed() API

[Derek Foreman]

On 2018-02-05 05:34 AM, Pekka Paalanen wrote:
> On Fri, 2 Feb 2018 16:25:56 -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>
>>>
>>> Reacting to DRM hotplug events is racy. It is theoretically possible to
>>> get hotplug events for a quick swap from one monitor to another and
>>> process both only after the new monitor is connected. Hence it is
>>> possible for display device information to change without going through
>>> a disconnected state for the head.
>>
>> I figured you'd have to address this somewhere. :)
>>
>>> To support such cases, add API to allow detecting it in the compositor.
>>>
>>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>> ---
>>>    libweston/compositor.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++----
>>>    libweston/compositor.h |  7 ++++
>>>    2 files changed, 89 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>>> index 86efcfad..0d3e185b 100644
>>> --- a/libweston/compositor.c
>>> +++ b/libweston/compositor.c
>>> @@ -4714,6 +4714,31 @@ weston_head_release(struct weston_head *head)
>>>    	wl_list_remove(&head->compositor_link);
>>>    }
>>>    
>>> +static void
>>> +weston_head_condition_device_changed(struct weston_head *head, bool condition)
>>> +{
>>> +	if (!condition)
>>> +		return;
>>
>> It feels odd to me to check the condition here instead of having the
>> callers do something like:
>> if (condition) weston_head_device_changed();
> 
> Yeah, it could certainly be done that way. ISTR to go through various
> variations, but not sure I tried exactly that though. The starting point
> was how weston_head_set_connection_status() was.
> 
> I could change that.

I'll concede that it saves a line of code at the call sites, it just 
"feels weird" to see constructs like that (in C code, to me). :)

Thanks,
Derek

>> But I guess that's a style thing, so my personal preference doesn't hold
>> much weight here...
>>
>> Either way,
>> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> 
> 
> Thanks,
> pq
> 
> 
>>> +
>>> +	head->device_changed = true;
>>> +
>>> +	if (head->compositor)
>>> +		weston_compositor_schedule_heads_changed(head->compositor);
>>> +}
>>> +
>>> +/** String non-equal comparison with NULLs being equal */
>>> +static bool
>>> +str_null_neq(const char *a, const char *b)
>>> +{
>>> +	if (!a && !b)
>>> +		return false;
>>> +
>>> +	if (!!a != !!b)
>>> +		return true;
>>> +
>>> +	return strcmp(a, b);
>>> +}
>>> +
>>>    /** Store monitor make, model and serial number
>>>     *
>>>     * \param head The head to modify.
>>> @@ -4724,6 +4749,8 @@ weston_head_release(struct weston_head *head)
>>>     * \param serialno The monitor serial number, a made-up string, or NULL for
>>>     * none.
>>>     *
>>> + * This may set the device_changed flag.
>>> + *
>>>     * \memberof weston_head
>>>     * \internal
>>>     */
>>> @@ -4733,6 +4760,11 @@ weston_head_set_monitor_strings(struct weston_head *head,
>>>    				const char *model,
>>>    				const char *serialno)
>>>    {
>>> +	weston_head_condition_device_changed(head,
>>> +				str_null_neq(head->make, make) ||
>>> +				str_null_neq(head->model, model) ||
>>> +				str_null_neq(head->serial_number, serialno));
>>> +
>>>    	free(head->make);
>>>    	free(head->model);
>>>    	free(head->serial_number);
>>> @@ -4748,6 +4780,8 @@ weston_head_set_monitor_strings(struct weston_head *head,
>>>     * \param mm_width Image area width in millimeters.
>>>     * \param mm_height Image area height in millimeters.
>>>     *
>>> + * This may set the device_changed flag.
>>> + *
>>>     * \memberof weston_head
>>>     * \internal
>>>     */
>>> @@ -4755,6 +4789,10 @@ WL_EXPORT void
>>>    weston_head_set_physical_size(struct weston_head *head,
>>>    			      int32_t mm_width, int32_t mm_height)
>>>    {
>>> +	weston_head_condition_device_changed(head,
>>> +					     head->mm_width != mm_width ||
>>> +					     head->mm_height != mm_height);
>>> +
>>>    	head->mm_width = mm_width;
>>>    	head->mm_height = mm_height;
>>>    }