magnifier: fix high power consumption even with magnifier disabled

[tokyo4j]

Fixes the high power consumption issue reported by @droc12345.

My fix is based on my understanding that the output state should be committed when the cursor has been moved or magnifier on/off state is changed.

I confirmed the CPU consumption reduced from around 5% to lower than 1% with this fix.

[tokyo4j]

Link: #1832

[droc12345]

As I mentioned in #1822, it does fix the increased power draw I was seeing.
Looks good from my end, others can check that the magnification still works,
between this and the wlroots pixman fix, maybe this will fix all the problems.

[Consolatis]

I think this is correct.
However, I am not sure if we need the last_mag state in the first place:

When enabling the magnifier (either via magnify_toggle() or magnify_set_scale()) we do call wlr_output_schedule_frame() which should set wlr_output->needs_frame and thus already skip the early out.

[droc12345]

I was also pondering whether we need the last_mag, all we really care about is
do they want magnification and is the mag flag set (these conditions should always match, IMO, but who knows), I don't see the need to care about last_mag.

[droc12345]

As a side note, while looking at scene-helper I saw the fixme comment, and that MR was done 10 months ago, Do we need to look at this?

 * FIXME: Remove the following line as soon as
 * https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4253
 * is merged. At that point wlr_scene handles damage tracking internally
 * again.
 wlr_damage_ring_rotate(&scene_output->damage_ring);

[ahesford]

Magnification seems to work fine without last_mag on my end.

[Consolatis]

As a side note, while looking at scene-helper I saw the fixme comment, and that MR was done 10 months ago, Do we need to look at this?

Only relevant once we start to track wlroots 0.18.x. That wlroots MR didn't make it into 0.17.

[droc12345]

Sorry, I thought they had backported it.

[tokyo4j]

I removed last_mag.

I also found removing wants_magnification doesn't make any difference in my testing.
@Consolatis Could you explain why it's necessary?

EDIT:
Based on my observation of wlroots, in wlr_output_cursor_move(), output_cursor_damage_whole() damages the cursor area in the output for software cursor, or drm_connector_move_cursor() forces a new frame for hardware cursor.

[Consolatis]

I also found removing wants_magnification doesn't make any difference in my testing.

For software cursors that should indeed not make a difference as we don't early out as the damage region is not empty.
For hardware cursors I am not sure if wlroots actually schedules a new frame (and thus sets wlr_output->needs_frame).
There is also the wayland backend which might yet again behave slightly different in regards to the cursor details.

Note while testing this: movement of the cursor on top of some wayland surface or SSD button may / will cause damage by the application / SSD and thus it may look as if its actually working properly while it isn't. Moving the cursor on top of the empty desktop area should not cause any application / SSD damage or change the cursor shape and thus is a better test here.

Another note while testing: once the magnifier is shown, the area of the magnifier will be added to the damage region for the next frame so from that point on we will never early out until the magnification is disabled again.

[droc12345]

Not sure if wants_magnification is necessary, but it does call is_magnify_on, and returns true/false on that.
But that did indeed make last_mag/is_magnify_on unnecessary as it's already being called.

Edit to add: wants_magnification, calls output_wants_magnification, which returns false for either
!magnify_on or the cursor not having been moved. Though I'm not sure that function is 100% accurate.
Something doesn't seem right about it, but it's not clear why I feel that way.

ETA2: in output_wants_magnification, it checks for cursor not having moved, do we really care about that.
If not, then the whole function could go away, and be replaced with the simple call is_magnify_on
which simply checks the flag.

[tokyo4j]

For software cursors that should indeed not make a difference as we don't early out as the damage region is not empty.
For hardware cursors I am not sure if wlroots actually schedules a new frame (and thus sets wlr_output->needs_frame).
There is also the wayland backend which might yet again behave slightly different in regards to the cursor details.

OK. I'm not sure removing wants_magnification is fine for all backends, either. In addition to wayland backend, I have no idea how cursor is handled in headless backend.

ETA2: in output_wants_magnification, it checks for cursor not having moved, do we really care about that.
If not, then the whole function could go away, and be replaced with the simple call is_magnify_on
which simply checks the flag.

I think this is correct, given that the early return never happens when the magnifier is shown. I replaced wants_magnification with is_magnify_on() in the fixup commit.

However, I'm feeling the current situation where the screen is always redrawn when the magnifier is shown is not optimal.
Is there any way to improve this? Is this the limitation of scene-graph API?

[droc12345]

cripes, what a convoluted mess.

This is about output_wants_magnification ie. wants_magnification

It's set to return false for !is_magnify_on
it then checks for cursor position not having moved (using seat's view) if not moved then return false
if it passes the above, then it sets a new position,
though the last line is ... weird ... I thiink that should just be replaced with "true"

This would allow the early bail if the cursor hasn't moved
the flip side is if the cursor is not moved, then mag won't kick in, ie early return.

I'm not sure how to solve this though.

maybe the original patch was for the best and leave everything else alone (as long as everything works)

            &scene_output->damage_ring.current) && !wants_magnification
-           && last_mag != is_magnify_on()) {
+           && last_mag == is_magnify_on()) {
        return false;
    }```

[Consolatis]

However, I'm feeling the current situation where the screen is always redrawn when the magnifier is shown is not optimal.

Agree.

Is there any way to improve this? Is this the limitation of scene-graph API?

We somehow need to mark the magnifier region damaged but maybe we could do so before the early out check based on moved cursor || non-empty damage ring rather than always after modifying the buffer. That should then theoretically allow us to use the early out branch. What complicates this is that the whole function is called with different outputs so we can't just make the additional_damage box static as its output specific.

though the last line is ... weird ... I thiink that should just be replaced with "true"

Then we would update a output without cursor when the magnifier is shown on another monitor.

This would allow the early bail if the cursor hasn't moved
the flip side is if the cursor is not moved, then mag won't kick in, ie early return.

No, it would not return early because the damage region is not empty due to us adding the damage after modifying the output buffer. See above.

Edit:
Overall the most important thing here is to get rid of the extra rendering with the magnifier disabled.
The performance improvement when the magnifier is enabled is a somewhat different story and it would be great if we can also fix that but it has lower priority IMHO.

[droc12345]

Consolatis, then we do want wants_magnification for its checks on flags, movement and output.

The extra rendering with mag disabled is easy, (the original patch worked)
either change
last_mag != is_magnify_on()
to
last_mag == is_magnify_on()

OR just check !is_magnify_on() and ignore last_mag.
either one will return early (if allowed by other conditions)

Not sure how the performance of mag on would be, but as you mention it's a lower priority
and many might never use it so it makes little difference to spend lots of time on it.

[tokyo4j]

Overall the most important thing here is to get rid of the extra rendering with the magnifier disabled.
The performance improvement when the magnifier is enabled is a somewhat different story and it would be great if we can also fix that but it has lower priority IMHO.

Agree. I feel fixing this problem is out of my reach. We can just leave this issue open and revisit later.
However, I also feel this is something we must fix someday, since labwc claims to be simple and performant compositor.

I updated the PR to simply remove last_mag and added a FIXME comment.

[Consolatis]

Thanks for taking care of this regression.

[droc12345]

Ok, added mag stuff to my config, and with the latest changes, it seems to work alright.
Power consumption is like it was before the magnifier merge, and even with mag on
the power consumption seems low. I think that the original problem with power usage
was due to the fact of using != instead of == for the check.

[tokyo4j]

Power consumption is like it was before the magnifier merge, and even with mag on the power consumption seems low.

Really? In my test, htop shows CPU usage goes up from 0% to 5% when I enable magnifier.

[droc12345]

You're right, I did re-check, and the power usage does go up, but it's only for the time the magnification is in effect.

With the fix, without mag, the power usage is a before.
with mag on, the power does increase, though it's still low, IMO, in my case going from ~3.2 watts to ~5.5
watts, but as soon as mag is toggled off, the power usage drops to normal (all this on an idle system)
This doesn't mean that the power usage will always ~double, but it will increase, mine just happens
to be a new low power system I bought, so I was monitoring the power anyway.

So bottom line, IMO, it's good enough for general usage, if you're going to use magnification, expect it to
use a little more power. At this point, I don't think it's worth putting in a lot of work to make it better.
If there's an easy fix, to the power problem fine, otherwise I'd just live with it, at least put it on the bottom
of the stack of things to do, ie not as important as other things.

Edit to add: the power usage I mention it true watts being used to drive the system.
But I did pull up top and watch labwc, normal ~1% cpu, mag on ~5%, Seeing what you're seeing.