Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wayland support (wlr_gamma_control_unstable_v1) #663

Closed
wants to merge 1 commit into from

Conversation

minus7
Copy link

@minus7 minus7 commented Sep 1, 2018

This continues the work from #568, replacing gamma_control with the newer wlr_gamma_control_unstable_v1. This newer protocol changes how gamma ramps are passed to support some graphics card + monitor combinations that have large gamma ramps. The old protocol was limited to 4096 bytes for gamma ramps which the new one avoids by passing ramps via a file descriptor. A snippet of MIT-licensed code from Weston has been added for that.

I'd like to get this merged upstream to not have it drag on longer. I can squash the changes if you want.

wlroots-based compositors already support this.

@jonls jonls self-requested a review September 1, 2018 20:16
@minus7
Copy link
Author

minus7 commented Sep 1, 2018

Looks like the backported pkg.m4 I reverted breaks the build on Ubuntu 14.04. Meh.

@ddevault ddevault mentioned this pull request Sep 18, 2018
@ddevault ddevault mentioned this pull request Oct 14, 2018
@zetorian
Copy link

I can confirm this is working on sway 1.0 with a dual monitor setup, using both internal intel graphics on i915 and nvidia 970m with nouveau, while using prime. great job guys!

@colemickens
Copy link

I'm using this successfully as well, on an XPS 9370 with Intel integration i915.

I do have one small bit of feedback -- Would it be possible to sniff sway/wlroots and default to using the wayland method of adjusting gamma if it knows it is running under Wayland?

@minus7
Copy link
Author

minus7 commented Nov 7, 2018

@colemickens I tried to make the autodetect work for Wayland but at least when running Wayland on one tty and X11 on another it doesn't work on X11 anymore

@colemickens
Copy link

@minus7, just realized my own package updated and I didn't notice. Turns out your detection change works well for me (but I also don't run X11 and Wayland simultaneously. Presumably that's a rare scenario.) Thanks for this PR, would be great to see it merged! 👍

@xantoz
Copy link

xantoz commented Dec 7, 2018

I've tried this on latest sway and wlroots master (swaywm/sway@cf6edaf and swaywm/wlroots@cf6edaf) and I noticed the following problems:

  1. One-shot mode doesn't work. It just flashes briefly.
  2. Tearing when setting the gamma ramps. This is relatively visible during the fade-in and fade-out. Maybe this is more of a problem in wlroots, or a limitation in DRM itself, however.

Otherwise it's working pretty good.

@emersion
Copy link

emersion commented Dec 7, 2018

One-shot mode doesn't work. It just flashes briefly.

Yes. This is by design. Instead, the oneshot mode should be replaced by a constant mode. I've heard macOS has the same issue.

Tearing when setting the gamma ramps. This is relatively visible during the fade-in and fade-out. Maybe this is more of a problem in wlroots, or a limitation in DRM itself, however.

No idea what could cause this. I can't reproduce. Maybe related to your graphics driver.

@xantoz
Copy link

xantoz commented Dec 7, 2018

@emersion The tearing is even more visible if running using oneshot mode (which just blinks briefly).
I am running on Intel Skylake with a dual head setup.

By tearing I mean only the gamma itself is affected, not the applications themselves. There will be a tearline with differing color temperatures above/under. Likely because changes in the gamma ramps are not synchronized with vblank like everything else?

Perhaps discussion of this issue in particular should move to the wlroots issue tracker, though?

@minus7
Copy link
Author

minus7 commented Apr 17, 2019

I "rebased and squashed" this. That makes it look a lot saner. Honestly, it was a nightmare of branches and merges. I had to resort to git diff $(git merge-base upstream/master HEAD)..HEAD and applying that to the upstream master (which oddly enough worked flawlessly then).
I tried attributing everyone that contributed code and mentioning the important changes in the commit message.

CC @Lourens-Rich @martinetd @breznak @t-8ch @giucam: Most of you have code in this :D

@minus7
Copy link
Author

minus7 commented Apr 17, 2019

Would be cool to see this merged, @jonls. Seems like a few people already use this branch through the AUR.

@xantoz: The oneshot issue has been addressed by @martinetd by making it behave like on macOS/Quartz and simply blocking. There's a better solution under discussion in #690

@atheriel
Copy link

What is the relationship between this PR, #704, and #568? They seem largely identical. It would probably be a lot easier for both users and the maintainer if there was an authoritative branch to pull from/review.

@emersion
Copy link

TBH at this point it might be better to establish a proper fork.

@atheriel
Copy link

Only if the maintainers of that fork commit resources to keeping it up to date with the upstream. Otherwise we'll just end up with a further proliferation of these working-at-some-point branches and PRs. (Although I commend @minus7 for recognizing all the previous authors for their work in this particular patch.)

The wlroots project seems like the obvious place for such a fork, if you have the bandwidth for it.

@emersion
Copy link

Considering that there were ~15 commits during last year, I don't think keeping up with upstream is really an issue. @jonls just seems to be lacking time to review PRs. Unfortunately there's no other maintainer, so this project is kind of stuck.

@minus7
Copy link
Author

minus7 commented Apr 28, 2019

What is the relationship between this PR, #704, and #568? They seem largely identical. It would probably be a lot easier for both users and the maintainer if there was an authoritative branch to pull from/review.

Oh, I didn't know about #704; someone made the effort of squashing the changes there already.
#568 uses an older protocol (see this PR's description) that's no longer used by wlroots-based compositors at least. I don't really know who uses which Wayland gamma interface.

I'm using Redshift on sway, so I'll keep maintaining my branch.

@TheTumultuousUnicornOfDarkness

(To follow up my comment in #704)

I use redshift-wlr-gamma-control-git package, which uses minus7's wayland branch, and it works like a charm with AMDGPU driver on Sway now.

@e00E Does it work for you too with minus7's wayland branch?
Here is the list of versions I use:
sway 1.0-8
wlroots 0.5.0-1
redshift-wlr-gamma-control-git 1.12.r25.geecbfed-1

@minus7
Copy link
Author

minus7 commented May 25, 2019

Works since Linux 5.1 (I noticed since I downgraded to 5.0 for a moment)

@TheTumultuousUnicornOfDarkness

I'm using Linux 5.1.4 with Mesa 19.0.5. Yes, it looks like it was a bug in Linux 5.0.
Thank you very much for your work @minus7!

@freed00m
Copy link

freed00m commented Jul 5, 2019

I use redshift-wlr-gamma-control-git package, which uses minus7's wayland branch, and it works like a charm with AMDGPU driver on Sway now.

@x0rg How? The AUR package fails to build, didn't touch the pkgbuild or the source.

./configure: 15410: ./configure: Syntax error: word unexpected (expecting ")")

@TheTumultuousUnicornOfDarkness

@freed00m: IDK, it works with makepkgfor me.

$ echo $PWD && LC_ALL=C makepkg -sif                                                                                                                                   git:(master)
/tmp/redshift-wlr-gamma-control-git
==> Making package: redshift-wlr-gamma-control-git 1.12.r25.geecbfed-1 (Fri Jul  5 20:11:13 2019)
==> Checking runtime dependencies...
==> Checking buildtime dependencies...
==> Retrieving sources...
  -> Updating redshift git repo...
Fetching origin
==> Validating source files with md5sums...
    redshift ... Skipped
==> Extracting sources...
  -> Creating working copy of redshift git repo...
Reset branch 'makepkg'
==> Starting pkgver()...
==> Removing existing $pkgdir/ directory...
==> Starting build()...
Copying file po/Makefile.in.in
autoreconf: Entering directory `.'
autoreconf: running: intltoolize --automake --copy --force
autoreconf: running: aclocal --force -I m4
autoreconf: configure.ac: tracing
autoreconf: running: libtoolize --copy --force
libtoolize: putting auxiliary files in '.'.
libtoolize: copying file './ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'.
libtoolize: copying file 'm4/libtool.m4'
libtoolize: copying file 'm4/ltoptions.m4'
libtoolize: copying file 'm4/ltsugar.m4'
libtoolize: copying file 'm4/ltversion.m4'
libtoolize: copying file 'm4/lt~obsolete.m4'
autoreconf: running: /usr/bin/autoconf --force
autoreconf: running: /usr/bin/autoheader --force
autoreconf: running: automake --add-missing --copy --force-missing
configure.ac:56: warning: The 'AM_PROG_MKDIR_P' macro is deprecated, and its use is discouraged.
configure.ac:56: You should use the Autoconf-provided 'AC_PROG_MKDIR_P' macro instead,
configure.ac:56: and use '$(MKDIR_P)' instead of '$(mkdir_p)'in your Makefile.am files.
configure.ac:14: installing './compile'
configure.ac:9: installing './missing'
src/Makefile.am: installing './depcomp'
autoreconf: Leaving directory `.'
checking for a BSD-compatible install... /usr/bin/install -c
[...]

@minus7
Copy link
Author

minus7 commented Jul 11, 2019

There's still two things bothering me with this patch:

  • After having monitors on standby for a while (over night works well), redshift fails. Still gotta figure out what's wrong there. The file descriptors/handles for the gamma ramps leak and will hit 65k after a few days or running and crash. Not exactly sure why they aren't cleaned up though. Fixed
  • Sometimes a single monitor flickers. I guess a gamma update is spread over two frames there.

@freed00m
Copy link

@x0rg

Thx, I really must find what is the missing dep or what is causing my setup to fail.

This is my log.

redshift-wlr-gamma-control-git-1.12.r25.geecbfed-1-x86_64-build.log

@TheTumultuousUnicornOfDarkness

@freed00m: I have found this: bitlbee/bitlbee-facebook#49 (comment)
Did you install the base-devel group? It is required to use makepkg.

@maximbaz
Copy link

maximbaz commented Apr 6, 2020

Try wlroots & Sway master. A fix got merged about gamma ramps recently.

It doesn't reproduce there, thank you very much!

@clarfonthey
Copy link

This has been open since 2018, and it seems like this repo hasn't been maintained much since then. Does anyone have the time/resources to maintain a fork? Or is the maintainer just slow? Not really sure what's up.

@CameronNemo
Copy link
Contributor

@clarfon I may have some cycles available. See the master branch here https://github.com/CameronNemo/redshift

@asadoll
Copy link

asadoll commented Jun 13, 2020

@clarfon I may have some cycles available. See the master branch here https://github.com/CameronNemo/redshift

Works as expected with both sway and wayfire master branches.

@vperilla
Copy link

Tested with Hikari and works fine

@TheChymera
Copy link

@CameronNemo is there no chance to get this merged?

@martinetd
Copy link

@CameronNemo is there no chance to get this merged?

This isn't CameronNemo's repo here. He made fork where it is merged (and redshift got renamed to gammastep after removing windows/mac support, with readme pointing to a gitlab repo that doesn't exist and no way to create issues...), but the only one who can make changes here is @jonls when he can find time to get around it.
He did get around to review the dbus PR recently so all hope isn't lost yet, maybe :)

@CameronNemo
Copy link
Contributor

If I cleaned up my repo and marked a release, would anyone be willing to package it for:

  • Arch (AUR)
  • Nix
  • Debian
  • Gentoo
  • anything else

?

@TheChymera
Copy link

If I cleaned up my repo and marked a release, would anyone be willing to package it for:

* Gentoo

?

Would this be a “permanent” fork? I.e. do you have the interest and time availability to merge future work by the original upstream, or continue to develop it your own way? I would much prefer if we could somehow convince the principal (@jonls ?) upstream to merge this so that development efforts are consolidated in one place. Forks are great when upstream becomes unresponsive, but they're annoying to maintain in parallel and have a bad habit of running out of steam and becoming outdated once the original issue was satisfied...

@CameronNemo
Copy link
Contributor

CameronNemo commented Jun 22, 2020

The original intention was to make it a permanent fork, but seeing as @jonls has been active in the past few weeks perhaps I need to adjust my trajectory. My biggest concern with a permanent fork would be missing out on the dbus service work that is going on in another fork (https://github.com/dkondor/redshift/tree/dbus_new).

But I cannot wait any longer for a release with wayland support. I will fork by the end of the month and mark a release -- it is just a matter of whether that fork intends to stay in sync with upstream or not. I would hope some distros pick it up, because I know many people want wayland support, but that happening is not a blocker to me forking.

I do appreciate @jonls activity in the past few weeks, responding to issues and reviewing PRs. I have a few questions for him:

  • Are there any blockers for this PR being merged?
  • When does he think a new release would be made, and how likely is it that the new release contain the work done in this PR?
  • What does he see as his role in the project moving forward? Does he have anyone in the pipeline to be added as a maintainer?

@lheckemann
Copy link

@CameronNemo yes, I'm happy to take care of the nix package.

@c4rlo
Copy link

c4rlo commented Jul 18, 2020

For Arch Linux, I've just submitted @CameronNemo's fork (gammastep) to the AUR as gammastep-git.

@CameronNemo
Copy link
Contributor

CameronNemo commented Jul 18, 2020

What a coincidence. I just put some finishing touches on the branch, made the gitlab repository public, and tagged a release. I did most of it last night but wanted to sleep on it.

@c4rlo
Copy link

c4rlo commented Jul 18, 2020

Sweet! I've pushed it to the AUR as gammastep, and have removed gammastep-git from the AUR.

Copy link
Contributor

@CameronNemo CameronNemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to apply a couple of commits on top of this.

int create_tmpfile_cloexec(char *tmpname) {
int fd;
mode_t prev_umask = umask(0066);
#ifdef HAVE_MKOSTEMP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever be defined? There is no AC_CHECK_FUNCS([mkostemp]) in the configure.ac file.

https://gitlab.com/chinstrap/gammastep/-/commit/47502deb942dff764d9f896f1004eaaa9c3cf4ef

return -1;
}

#ifdef WLR_HAS_POSIX_FALLOCATE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be HAVE_POSIX_FALLOCATE, and see above

return -1;
}

void *ptr = mmap(NULL, total_bytes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if gamma_size is 0 (i.e. when the output does not support setting gamma), the total_bytes will be zero and mmap will error with EINVAL.

https://gitlab.com/chinstrap/gammastep/-/commit/fdfec2bfb4ee8a44ae503a7c63a073453a17f176

rwanyoike pushed a commit to rwanyoike/dotfiles_archived that referenced this pull request Jul 29, 2020
… gopass

Add intel-undervolt, kubectx, wf-recorder

gammastep ticket: jonls/redshift#663
rwanyoike added a commit to rwanyoike/dotfiles_archived that referenced this pull request Jul 29, 2020
… gopass

Add intel-undervolt, kubectx, wf-recorder

gammastep ticket: jonls/redshift#663
@nbraud
Copy link

nbraud commented Dec 12, 2020

FYI, I wrote a patch that updates redshift's AppArmor profile to make it work with this PR.

@rustycl0ck
Copy link

ping @jonls , would be great to have this reviewed/merged

@archseer
Copy link

@rustycl0ck We've all migrated over to the https://gitlab.com/chinstrap/gammastep fork

@minus7
Copy link
Author

minus7 commented Jun 16, 2021

Another alternative (with less features) is [wlsunset|https://sr.ht/~kennylevinsen/wlsunset/]. I'll close this MR since I don't use nor maintain it anymore.

@minus7 minus7 closed this Jun 16, 2021
Artturin added a commit to Artturin/nixpkgs that referenced this pull request Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.