[PATCH weston v4 1/3] compositor: If the output is inhibited, don't idle it off
Derek Foreman
derekf at osg.samsung.com
Tue Aug 16 14:51:01 UTC 2016
On 15/08/16 09:33 PM, Bryce Harrington wrote:
> On Mon, Aug 15, 2016 at 06:57:52PM -0500, Derek Foreman wrote:
>> On 10/08/16 07:25 PM, Bryce Harrington wrote:
>>> Adds a helper routine weston_output_inhibited_outputs() which returns a
>>> mask of outputs that should inhibit screen idling.
>>>
>>> Use this routine to check for inhibiting outputs for handling of idle
>>> behaviors in core: In sleep mode, only halt repainting outputs that
>>> don't have valid inhibits. Don't send these monitors DPMS off commands
>>> either, if the system would otherwise be powering them down.
>>>
>>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
>>> ---
>>> libweston/compositor.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------
>>> libweston/compositor.h | 10 +++++++++
>>> 2 files changed, 61 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>>> index b17c76d..aa14504 100644
>>> --- a/libweston/compositor.c
>>> +++ b/libweston/compositor.c
>>> @@ -485,6 +485,8 @@ weston_surface_create(struct weston_compositor *compositor)
>>>
>>> wl_list_init(&surface->pointer_constraints);
>>>
>>> + surface->inhibit_idling = false;
>>> +
>>> return surface;
>>> }
>>>
>>> @@ -2320,15 +2322,42 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
>>> TL_POINT("core_repaint_exit_loop", TLP_OUTPUT(output), TLP_END);
>>> }
>>>
>>> +/** Retrieves a mask of outputs that should inhibit screensaving
>>> + *
>>> + * \param compositor The compositor instance.
>>> + * \return An output mask indicating the ids of all inhibiting outputs
>>> + *
>>> + * Checks for surfaces whose clients have requested that they
>>> + * disable the screenserver and display powersaving. Note
>>> + * the output ids for these surfaces.
>>> + */
>>> +WL_EXPORT uint32_t
>>> +weston_compositor_inhibited_outputs(struct weston_compositor *compositor)
>>> +{
>>> + struct weston_view *view;
>>> + uint32_t inhibited_outputs_mask = 0;
>>> +
>>> + wl_list_for_each(view, &compositor->view_list, link) {
>>> + /* Does the view's surface inhibit this output? */
>>> + if (!view->surface->inhibit_idling)
>>> + continue;
>>> +
>>> + inhibited_outputs_mask |= view->output_mask;
>>> + }
>>> + return inhibited_outputs_mask;
>>> +}
>>> +
>>> static int
>>> output_repaint_timer_handler(void *data)
>>> {
>>> struct weston_output *output = data;
>>> struct weston_compositor *compositor = output->compositor;
>>> + uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>>>
>>> if (output->repaint_needed &&
>>> - compositor->state != WESTON_COMPOSITOR_SLEEPING &&
>>
>> I don't like that this patch changes the meaning of
>> "WESTON_COMPOSITOR_SLEEPING" - should at least make an effort to update
>> all relevant comments such as the one immediately before
>> "weston_compositor_sleep" and the one in the enum definition in
>> compositor.h...
>
> I'm not sure what you mean?
Well, as a quick example, in compositor.h, the comment after
WESTON_COMPOSITOR_SLEEPING is:
/* same as offscreen, but also set dpms to off */
This isn't accurate anymore as it's possible for us to be
WESTON_COMPOSITOR_SLEEPING and not actually have any displays with dpms
off at all, isn't it?
It becomes very confusing to figure out what's going on when "sleeping"
doesn't necessarily mean anything is asleep at all. Maybe a new state
should be introduced, but at a minimum all of the comments related to
sleeping should be updated to be in sync with the new behaviour.
>>> compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
>>> + (compositor->state != WESTON_COMPOSITOR_SLEEPING
>>> + || inhibited_outputs_mask & (1 << output->id)) &&
>>> weston_output_repaint(output) == 0)
>>> return 0;
>>>
>>> @@ -2450,9 +2479,15 @@ weston_output_schedule_repaint(struct weston_output *output)
>>> {
>>> struct weston_compositor *compositor = output->compositor;
>>> struct wl_event_loop *loop;
>>> + uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>>>
>>> - if (compositor->state == WESTON_COMPOSITOR_SLEEPING ||
>>> - compositor->state == WESTON_COMPOSITOR_OFFSCREEN)
>>> + /* If we're offscreen, or if we're sleeping and the monitor
>>> + * isn't currently being inhibited by an active surface, then
>>> + * skip repainting.
>>> + */
>>> + if (compositor->state == WESTON_COMPOSITOR_OFFSCREEN ||
>>> + (compositor->state == WESTON_COMPOSITOR_SLEEPING &&
>>> + !(inhibited_outputs_mask & (1 << output->id))))
>>> return;
>>>
>>> if (!output->repaint_needed)
>>> @@ -3859,10 +3894,20 @@ weston_compositor_dpms(struct weston_compositor *compositor,
>>> enum dpms_enum state)
>>> {
>>
>> The doxy block before this function has now become wrong, hasn't it?
>
> Not sure exactly what you mean by 'wrong', but I suppose it could
> benefit from being updated to mention that idled outputs won't be
> toggled off. Is that what you're referring to?
Well, the doxy says "Set a DPMS mode on all of the compositor's outputs"
After the patch it doesn't do that, it conditionally turns off monitors
that aren't being governed by a blanker inhibitor.
>>> struct weston_output *output;
>>> + struct weston_view *view;
>>> + uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>>>
>>> - wl_list_for_each(output, &compositor->output_list, link)
>>> - if (output->set_dpms)
>>> - output->set_dpms(output, state);
>>> + wl_list_for_each(output, &compositor->output_list, link) {
>>> + if (!output->set_dpms)
>>> + continue;
>>> +
>>> + /* If output is idle-inhibited, don't toggle to any DPMS state except ON. */
>>> + if (state != WESTON_DPMS_ON &&
>>> + inhibited_outputs_mask & (1 << output->id))
>>> + continue;
>>> +
>>> + output->set_dpms(output, state);
>>> + }
>>> }
>>>
>>> /** Restores the compositor to active status
>>> diff --git a/libweston/compositor.h b/libweston/compositor.h
>>> index 0133084..3a9a098 100644
>>> --- a/libweston/compositor.h
>>> +++ b/libweston/compositor.h
>>> @@ -1140,6 +1140,14 @@ struct weston_surface {
>>>
>>> /* An list of per seat pointer constraints. */
>>> struct wl_list pointer_constraints;
>>> +
>>> + /*
>>> + * Indicates the surface prefers no screenblanking, screensaving,
>>> + * or other automatic obscurement to kick in while the surface is
>>> + * considered "active" by the shell.
>>> + */
>>> + bool inhibit_idling;
>>
>> I find this a bit unclear - "idling" doesn't feel properly descriptive,
>> and the comment gives me the impression that an "idle inhibit surface"
>> will be redrawn even if it's obscured - does that mean surface frame
>> events for the surface will be generated even if it's not visible in any
>> way?
>
> Suggestions? If the surface in in the view_list and assigned to an
> output, then the idle inhibition will be honored.
I guess I'm fine with it, no need to bike shed this point.
>> Also, what's the bit about the surface being considered active by the
>> shell mean?
>
> That functionality got axed in an earlier review, this bit of doc was
> missed. Just ignore everything after "while".
Ah, ok, thought that might be the case. Would be good to see a patch
with the text fixed.
Thanks,
Derek
> Thanks,
> Bryce
>
>> Thanks,
>> Derek
>>
>>> +
>>> };
>>>
>>> struct weston_subsurface {
>>> @@ -1316,6 +1324,8 @@ void
>>> weston_output_schedule_repaint(struct weston_output *output);
>>> void
>>> weston_output_damage(struct weston_output *output);
>>> +uint32_t
>>> +weston_compositor_inhibited_outputs(struct weston_compositor *compositor);
>>> void
>>> weston_compositor_schedule_repaint(struct weston_compositor *compositor);
>>> void
>>>
More information about the wayland-devel
mailing list