update freedesktop api #264
Conversation
|
Hi! Thanks for this PR! 🎆 |
src/modules/pm.c
Outdated
| static void acquire_lock(void) { | ||
| sd_bus_message *msg = NULL; | ||
| sd_bus_error error = SD_BUS_ERROR_NULL; | ||
| sd_bus *sysbus = get_system_bus(); |
There was a problem hiding this comment.
I'd avoid exposing the system bus; instead, we should rely on the already existent way to do that (async):
SYSBUS_ARG_REPLY(...)
Basically, the only line that should need a change is:
USERBUS_ARG_REPLY(pm_args, parse_bus_reply, &pm_inh_token,
"org.freedesktop.PowerManagement.Inhibit",
"/org/freedesktop/PowerManagement/Inhibit",
"org.freedesktop.PowerManagement.Inhibit",
"Inhibit");
That should become
SYSBUS_ARG_REPLY(pm_args, parse_bus_reply, &pm_inh_token, "org.freedesktop.login1", "/org/freedesktop/login1", "org.freedesktop.login1.Manager", Inhibit);
and properly manage the reply message in the parse_bus_reply callback :)
There was a problem hiding this comment.
See https://github.com/FedeDP/Clight/blob/master/src/modules/upower.c#L97 for an example :)
src/modules/bus.c
Outdated
| @@ -206,3 +206,7 @@ static int proxy_async_request(struct sd_bus_message *m, void *userdata, sd_bus_ | |||
| sd_bus *get_user_bus(void) { | |||
| return userbus; | |||
| } | |||
|
|
|||
| sd_bus *get_system_bus(void) { | |||
There was a problem hiding this comment.
Following what i wrote below, there is no need to expose sysbus.
|
I've been working on this a bit as a separate custom module so I could try and get a clear idea of what we want it to do. You can see it here. I will push a few commits to be more inline with this and update with your comments. |
|
Thank you man :) and btw thanks for the effort! |
|
Okay, I believe I have updated it so it correctly uses your macros and will report to the login1.manager when Clight is inhibited, and will also inhibit Clight when some other exterenal program reports to login1.manager. |
|
Hi! I will review as soon as i got some spare time, thank you!
Session idle is already addressed by Clightd: https://github.com/FedeDP/Clightd/blob/master/src/modules/idle.c, and should work everywhere(tty, xorg and wayland). Isn't it working for you? Moreover, i am a bit wary about adding system-specific code in Clight (like Xorg specific, wl specific, kde specific and so on). |
|
Just a side note: can you actually keep the bare minimum diff from master? There are multiple changes that are just linting/coding style related. |
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
350c47a
to
545ef91
Compare
Sorry about that. I was trying to use a .clang-format to match what you
Well what I observe as not "working" is this. Applications like mpv, and edit: Swayidle does not report wayland specific idling events to the |
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
545ef91
to
9b233c4
Compare
Oh, so you are talking about org.freedesktop.ScreenSaver API then? But i think you are referring to the Clight Inhibited message, that is run when an external application asks the DE to inhibit its power management features, to avoid eg: dimming while a movie is being watched. I found this: https://www.reddit.com/r/linuxquestions/comments/t03c4q/how_do_i_lock_the_screen_with_sway/. |
There was a problem hiding this comment.
This is a first attempt at a review.
I am not getting all the ext_inhibit related code; i don't think it should belong here as it has nothing to do with power management, right?
It should perhaps being part of the inhibition logic in interface.c (or inhibit.c)?
If i am correct, i think we can work on that in another PR.
src/modules/pm.c
Outdated
| } | ||
|
|
||
| if (locks != NULL) { | ||
| DECLARE_HEAP_MSG(suspend_req, SUSPEND_REQ); |
There was a problem hiding this comment.
I think we are leaking the suspend_req here, if we are not going to publish it!
Moreover, i'd just call publish_susp_req(); there.
There was a problem hiding this comment.
On a second thought, do we really need to suspend if any logind inhibition is ON? I mean, we should just eventually suspend:
- when session is not active (checked below)
- when laptop goes to sleep
There was a problem hiding this comment.
the logic behind this was to publish the message so any other subscribed modules would know. I think this was the only way I could get autocalibration to disable without calling it specifically. Is there a better way to handle this?
There was a problem hiding this comment.
There is an inhibiton option (https://github.com/FedeDP/Clight/blob/master/Extra/clight.conf#L45) to automatically inhibit backlight calibratin on inhibition!
src/modules/pm.c
Outdated
| @@ -122,8 +148,14 @@ static int on_session_change(UNUSED sd_bus_message *m, UNUSED void *userdata, UN | |||
|
|
|||
| static int parse_bus_reply(sd_bus_message *reply, const char *member, void *userdata) { | |||
| int r = -EINVAL; | |||
| if (!strcmp(member, "Inhibit")) { | |||
| if (strcmp(member, "Inhibit") != 0) { | |||
There was a problem hiding this comment.
Mmmh it was == 0 before; is this right?
There was a problem hiding this comment.
Moreover, note that since parse_bus_reply callback is only called when acquiring the lock, i think we can drop the old branch of the if else statement.
There was a problem hiding this comment.
No, I dont think it was ==0 although, it should have been that or != since strcmp returns an int. But your larger point stands, I'll remove this.
There was a problem hiding this comment.
if (!strcmp(member, "Inhibit")) {
means
if (strcmp(member, "Inhibit") == 0) {
:P
Thanks btw!
There was a problem hiding this comment.
ahh I thought zero and null were different. Good to know. I should probably get a book on the language...
src/modules/pm.c
Outdated
| @@ -108,6 +110,30 @@ static int on_new_suspend(sd_bus_message *m, UNUSED void *userdata, UNUSED sd_bu | |||
|
|
|||
| /* Listener on logind session.Active for current session */ | |||
There was a problem hiding this comment.
Can you update the comment here explaining what are we watching now?
There was a problem hiding this comment.
Yes I'll update this once I fully understand how you want to approach this.
Yes, you are correct. Sorry I wasn't clear about this this before.
I don't believe that API is specifc to wayland, but to specific desktop I'm not certain the dbus api should be An excerpt from mpv's Man page
Gnome and Kde are the only wayland compositors to my knowledge that I don't know which direction you want to go, but it would be great to be To your comment about the
However, this is from the ScreenSaver wiki page. I do think the |
|
I think we should:
I am still not convinced that much about adding specific code in Clight; i think that the freedesktop standard should be followed as much as we can from sway and other wayland compositors (and other apps too!). EDIT: |
|
https://bugzilla.mozilla.org/show_bug.cgi?id=1587360#c3 :/ it's incredible how hard it is sometimes to have standards on linux... |
|
Yeah it is a very convoluted, the linux power management space. I think I imagine if you ran i3 on X, or any other window manager without a But overall I'm happy just distilling this PR to simply the logind Sending a inhibition lock to logind must have a 'Why' and it can be any |
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
2 times, most recently
from
976d9a0
to
523ba9b
Compare
|
I've got this to just replacing org.freedesktop.PowerManagement.Inhibit |
src/modules/pm.c
Outdated
| @@ -59,14 +60,14 @@ static void receive(const msg_t *const msg, UNUSED const void* userdata) { | |||
| case PM_REQ: { | |||
| pm_upd *up = (pm_upd *)MSG_DATA(); | |||
| if (VALIDATE_REQ(up)) { | |||
| on_pm_req(up->new); | |||
| on_pm_req(up->new, up->old); | |||
There was a problem hiding this comment.
Note: VALIDATE_REQ already takes care of rejecting requests if up->new == current state; we can avoid passing the additional param here, reducing the diff.
See: https://github.com/FedeDP/Clight/blob/master/src/pubsub/validations.c#L184
src/modules/pm.c
Outdated
|
|
||
| static void on_pm_req(const bool new, const bool old) { | ||
| if (new && !old) { | ||
| if (acquire_lock()) { |
There was a problem hiding this comment.
I really like the split acquire_lock and release_lock.
I think we could support both /org/freedesktop/PowerManagement/Inhibit, therefore just adding new:
acquire_logind_lockacquire_freedesktop_lock
and their release_foo_lock.
WDYT?
Supporting both should work on more systems (ie: non systemd ones!).
|
I'm happy to include the PowerManagement as a fallback for non-systemd systems. However, I don't have any way to test it, so you may need to make some changes to the PR. Currently, the only spec for the PowerManagement API I can find is this for xfce. It doesn't seem like the call to This makes things particularly difficult for the Systemd fallback to PowerManagementstatic bool release_lock(void) {
if (close(pm_inh_token) == 0) {
DEBUG("Released systemd-inhibit inhibition.\n");
pm_inh_token = -1;
return true;
} else {
USERBUS_ARG(pm_args, "org.freedesktop.PowerManagement.Inhibit",
"/org/freedesktop/PowerManagement/Inhibit",
"org.freedesktop.PowerManagement.Inhibit", "UnInhibit");
if (call(&pm_args, "u", pm_inh_token) == 0) {
DEBUG("Released PowerManagement inhibition.\n");
pm_inh_token = -1;
return true;
}
}
return false;
}Systemd
Non-systemd
PowerManagement fallback to Systemdstatic bool release_lockv2(void) {
USERBUS_ARG(pm_args, "org.freedesktop.PowerManagement.Inhibit",
"/org/freedesktop/PowerManagement/Inhibit",
"org.freedesktop.PowerManagement.Inhibit", "UnInhibit");
if (call(&pm_args, "u", pm_inh_token) == 0) {
DEBUG("Released PowerManagement inhibition.\n");
pm_inh_token = -1;
return true;
} else if (close(pm_inh_token) == 0) {
DEBUG("Released systemd-inhibit inhibition.\n");
pm_inh_token = -1;
return true;
}
return false;
}Systemd
Non-systemd
|
force pushed to avoid whitespace changes
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
61d7fef
to
f7bec22
Compare
|
Thanks for updating this! Aside from this, let me state my gratefulness for this discussion and for your work! :) |
|
No problem at all. I've learned a lot about C and programming in general through this exercise and am happy to contribute how I can. A question about the variable. Would it be enough to use a boolean such as |
|
A bool is ok! Or you can use enum values like { NONE, SYSTEMD, FREEDESKTOP }. |
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
026a3ff
to
84a1d5b
Compare
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
f448630
to
e92a973
Compare
Moreover, updated todo. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
|
Oh I forgot to let you know I’ve made the changes you requested, if you want to have another look |
|
Hi! I made some really small changes, and opened a PR against this branch in your fork! |
…ack. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
|
okay, I've accepted your PR and tested on my machine, everything looks |
tpeacock19
force-pushed
the
feat/freedesktop-inhibit
branch
from
e144005
to
0996b13
Compare
|
Thank you for the great patience and work! :) |
| SYSBUS_ARG_REPLY(pm_args, parse_bus_reply, &pm_inh_token, | ||
| "org.freedesktop.login1", "/org/freedesktop/login1", | ||
| "org.freedesktop.login1.Manager", "Inhibit"); | ||
| int ret = call(&pm_args, "ssss", "idle", "Clight", "Idle inhibitor.", "block"); |
There was a problem hiding this comment.
@tpeacock19 i just noticed we are inhibiting idle here; i think we should be inhibiting shutdown, sleep instead.
At the same time, idle should be inhibited by the inhibit module IMHO, like here: https://github.com/FedeDP/Clight/tree/master/src/modules#L60
What do you think?
There was a problem hiding this comment.
I can do a PR in that direction myself btw ;) i want to just gather your feedback since you implemented it!
There was a problem hiding this comment.
Sorry, i meant here: https://github.com/FedeDP/Clight/blob/master/src/modules/display.c#L95
There was a problem hiding this comment.
Seven distinct inhibitor lock types may be taken, or a combination of them:
- sleep inhibits system suspend and hibernation requested by (unprivileged) users
- shutdown inhibits high-level system power-off and reboot requested by (unprivileged) users
- idle inhibits that the system goes into idle mode, possibly resulting in automatic system suspend or shutdown depending on configuration.
From the Freedesktop Docs. I had assumed that idle would cover the cases for automatic suspend/sleep/shutdown and if it was explicitly called, then it should allow for it. That is how I understand the documentation anyway. Do you think differently?
However, since it does default to idle;sleep;shutdown, perhaps it wouldn't be a bad idea?
There was a problem hiding this comment.
I think you are right indeed!
Thank you!
The PowerManagement api seems to be deprecated,
https://www.freedesktop.org/wiki/Specifications/power-management-spec/
This commit updates the current pm module to use the
org.freedesktop.login1.Manager.Inhibit api. This also has the
benefit of working with swayidle.
a bit unrelated, but I'm thinking of working on a wayland
specific pm module that will rely on
https://wayland.app/protocols/idle-inhibit-unstable-v1
Would you be interested in this?