Re: [systemd-devel] [PATCH] backlight: let the administrator override clamping

2015-01-17 Thread Topi Miettinen 
On 01/17/15 11:38, David Herrmann wrote:
 Hi
 
 On Sat, Jan 17, 2015 at 12:28 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On my computer, the minimum brightness enforced by clamping in
 backlight is too bright.
 
 How can 5% of the backlight be too bright? Can you give some
 information on your backlight device? (type, max_brightness,
 actual_brightness and so on).

Well, my eyes start to hurt with level 1, 0 is OK. Max_brightness is 9,
actual_brightness always matches brightness. This is cheap old Acer
Aspire 8530 laptop with Mobility Radeon HD 3200, I don't know beyond
that what handles backlight.

 
 Btw., we use gnu-getopt_long() style arguments with two dashes. And we
 try to avoid negations in option names.

Ok. How about --disable-clamping or --allow-low-brightness, or would you
have better ideas?

 
 Thanks
 David
 
 Add a new option to override clamping in unit file. While at it, describe
 the clamping in documentation.
 ---
  man/systemd-backli...@.service.xml | 23 +--
  src/backlight/backlight.c  | 11 ++-
  2 files changed, 27 insertions(+), 7 deletions(-)

 diff --git a/man/systemd-backli...@.service.xml 
 b/man/systemd-backli...@.service.xml
 index 453afbf..26a2437 100644
 --- a/man/systemd-backli...@.service.xml
 +++ b/man/systemd-backli...@.service.xml
 @@ -48,7 +48,17 @@

  refsynopsisdiv
  parafilenamesystemd-backlight@.service/filename/para
 -
 parafilename/usr/lib/systemd/systemd-backlight/filename/para
 +cmdsynopsis
 +commandsystemd-backlight/command
 +arg choice=plainsave/arg
 +arg 
 choice=optreplaceablePATH/replaceable/arg
 +/cmdsynopsis
 +cmdsynopsis
 +commandsystemd-backlight/command
 +arg choice=plainload/arg
 +arg 
 choice=optreplaceablePATH/replaceable/arg
 +arg 
 choice=optreplaceable-no-clamp/replaceable/arg
 +/cmdsynopsis
  /refsynopsisdiv

  refsect1
 @@ -58,7 +68,16 @@
  is a service that restores the display backlight
  brightness at early boot and saves it at shutdown. On
  disk, the backlight brightness is stored in
 -filename/var/lib/systemd/backlight//filename./para
 +filename/var/lib/systemd/backlight//filename. During
 +loading, unless option
 +replaceable-no-clamp/replaceable is specified, the
 +brightness is clamped to at least value
 +literal1/literal or 5% of maximum
 +brightness./para
 +
 +parareplaceablePATH/replaceable identifies the
 +display brightness control device. It is resolved by
 +udev./para
  /refsect1

  refsect1
 diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c
 index 1271a66..a3f71e9 100644
 --- a/src/backlight/backlight.c
 +++ b/src/backlight/backlight.c
 @@ -255,7 +255,7 @@ static void clamp_brightness(struct udev_device *device, 
 char **value, unsigned
  return;
  }

 -log_info(Saved brightness %s %s to %s., old_value,
 +log_info(Saved brightness %s %s to %s (use -no-clamp to 
 override)., old_value,
   new_brightness  brightness ?
   too low; increasing : too high; decreasing,
   *value);
 @@ -272,8 +272,8 @@ int main(int argc, char *argv[]) {
  unsigned max_brightness;
  int r;

 -if (argc != 3) {
 -log_error(This program requires two arguments.);
 +if (argc  3 || argc  4) {
 +log_error(This program requires two or three arguments.);
  return EXIT_FAILURE;
  }

 @@ -389,8 +389,9 @@ int main(int argc, char *argv[]) {
  log_error_errno(r, Failed to read %s: %m, saved);
  return EXIT_FAILURE;
  }
 -
 -clamp_brightness(device, value, max_brightness);
 +/* Don't clamp brightness if asked */
 +if (!(argc == 4  streq(argv[3], -no-clamp)))
 +clamp_brightness(device, value, max_brightness);

  r = udev_device_set_sysattr_value(device, brightness, 
 value);
  if (r  0) {
 --
 2.1.4

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] backlight: let the administrator override clamping

2015-01-17 Thread David Herrmann 
Hi

On Sat, Jan 17, 2015 at 1:32 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 12:19, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 1:10 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 12:03, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 1:01 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 11:38, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 12:28 PM, Topi Miettinen toiwo...@gmail.com 
 wrote:
 On my computer, the minimum brightness enforced by clamping in
 backlight is too bright.

 How can 5% of the backlight be too bright? Can you give some
 information on your backlight device? (type, max_brightness,
 actual_brightness and so on).

 Well, my eyes start to hurt with level 1, 0 is OK. Max_brightness is 9,
 actual_brightness always matches brightness. This is cheap old Acer
 Aspire 8530 laptop with Mobility Radeon HD 3200, I don't know beyond
 that what handles backlight.

 Which backlight driver is active? acpi? Or the native radeon driver?

 The device path is
 /sys/devices/pci\:00/\:00\:01.0/\:01\:05.0/backlight/acpi_video0/brightness,
 does that mean acpi?

 The problem here is, there're 2 types of backlight drivers in the kernel:

 1) backlight==0 is interpreted as 'off'
 2) backlight==0 is interpreted as 'lowest level that is not off'

 There is a second switch called 'bl_power' which allows to actually
 power the backlight off or on. This works on all devices the same way.
 However, if we want to figure out the lowest backlight level, we
 really cannot use '0' as we have no idea how the driver will interpret
 it.

 I see. Here, setting bl_power to 0 does nothing,

Sure, if the hardware does not support power-down, it will not work.


 We discussed this at the last XDC but haven't really come to a
 conclusion. This is definitely a kernel bug, as user-space has no
 chance of setting a backlight generically to lowest level. If there
 were a property called min_brightness that exposes the minimal
 brightness level supported (which is not 'off'), we could parse it.
 However, we don't want to write per-driver workarounds in userspace if
 kernel people could just fix it.

 Conclusion: Lets try getting kernel backlight people to solve this mess.

 That may be the proper long term path, but because there's already a
 clamping workaround which does not do the right thing for all hardware,
 this override would be useful for such cases until the kernel is fixed.
 After the kernel is fixed, the clamping (along this override) should not
 be applied anymore.

No, we still need clamping! In case your hardware has 256 brightness
levels, we really don't want a brightness of 1 as it would still be
far too dark.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] backlight: let the administrator override clamping

2015-01-17 Thread Topi Miettinen 
On 01/17/15 12:34, David Herrmann wrote:
 Hi
 
 On Sat, Jan 17, 2015 at 1:32 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 12:19, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 1:10 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 12:03, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 1:01 PM, Topi Miettinen toiwo...@gmail.com 
 wrote:
 On 01/17/15 11:38, David Herrmann wrote:
 Hi

 On Sat, Jan 17, 2015 at 12:28 PM, Topi Miettinen toiwo...@gmail.com 
 wrote:
 On my computer, the minimum brightness enforced by clamping in
 backlight is too bright.

 How can 5% of the backlight be too bright? Can you give some
 information on your backlight device? (type, max_brightness,
 actual_brightness and so on).

 Well, my eyes start to hurt with level 1, 0 is OK. Max_brightness is 9,
 actual_brightness always matches brightness. This is cheap old Acer
 Aspire 8530 laptop with Mobility Radeon HD 3200, I don't know beyond
 that what handles backlight.

 Which backlight driver is active? acpi? Or the native radeon driver?

 The device path is
 /sys/devices/pci\:00/\:00\:01.0/\:01\:05.0/backlight/acpi_video0/brightness,
 does that mean acpi?

 The problem here is, there're 2 types of backlight drivers in the kernel:

 1) backlight==0 is interpreted as 'off'
 2) backlight==0 is interpreted as 'lowest level that is not off'

 There is a second switch called 'bl_power' which allows to actually
 power the backlight off or on. This works on all devices the same way.
 However, if we want to figure out the lowest backlight level, we
 really cannot use '0' as we have no idea how the driver will interpret
 it.

 I see. Here, setting bl_power to 0 does nothing,
 
 Sure, if the hardware does not support power-down, it will not work.
 

 We discussed this at the last XDC but haven't really come to a
 conclusion. This is definitely a kernel bug, as user-space has no
 chance of setting a backlight generically to lowest level. If there
 were a property called min_brightness that exposes the minimal
 brightness level supported (which is not 'off'), we could parse it.
 However, we don't want to write per-driver workarounds in userspace if
 kernel people could just fix it.

 Conclusion: Lets try getting kernel backlight people to solve this mess.

 That may be the proper long term path, but because there's already a
 clamping workaround which does not do the right thing for all hardware,
 this override would be useful for such cases until the kernel is fixed.
 After the kernel is fixed, the clamping (along this override) should not
 be applied anymore.
 
 No, we still need clamping! In case your hardware has 256 brightness
 levels, we really don't want a brightness of 1 as it would still be
 far too dark.

How could you know that? The display can be too bright for the poor user
even at 4%.

-Topi

 
 Thanks
 David
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] backlight: let the administrator override clamping

2015-01-17 Thread Topi Miettinen 
On 01/17/15 12:48, David Herrmann wrote:
 Hi
 
 On Sat, Jan 17, 2015 at 1:41 PM, Topi Miettinen toiwo...@gmail.com wrote:
 On 01/17/15 12:34, David Herrmann wrote:
 No, we still need clamping! In case your hardware has 256 brightness
 levels, we really don't want a brightness of 1 as it would still be
 far too dark.

 How could you know that? The display can be too bright for the poor user
 even at 4%.
 
 If the brightness scale is linear, I don't see how 5% can be too
 bright. We write defaults that shall work for everyone.

You are lucky not to have my eyes or hardware. It does not work for me.

 You can
 easily disable our helpers and employ your own. But we cant provide
 configuration hooks for each and every obscure use-case. We want
 people to disable the defaults and write their own, if they don't like
 the defaults.

I understand that you don't want additional knobs to be maintained, but
in this case it's not possible to override the clamping logic other way.
I can of course disable the backlight entirely but then I would miss the
save/restore functionality.

How about splitting the logic into two? The other helper would only
perform save and restore and the other would decide if the values are
reasonable for the current kernel version and hardware of the day?

 
 In your case, it's an outright bug, though. And we want bugs to be
 fixed, instead of workarounds if developers are too lazy to fix their
 stuff.

I fully agree, for me the bug is in clamping logic and the fact that it
can't be overridden or separated from loading.

 
 Thanks
 David
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel