| Message ID | 1322857768-6470-2-git-send-email-seth.forshee@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 02, 2011 at 02:29:28PM -0600, Seth Forshee wrote: > From: Jason Stubbs <jasonbstubbs@gmail.com> > > On some Samsung laptops the brightness regulation works slightly different. > All SABI commands except for set_brightness work as expected. The behaviour > of set_brightness is as follows: > > - Setting a new brightness will only step one level toward the new brightness > level. For example, setting a level of 5 when the current level is 2 will > result in a brightness level of 3. > - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated > along with the change in brightness. > - Neither of the above two issues occur when changing from/to brightness > level 0. > > This patch adds detection and a non-intrusive workaround for the above issues. > > Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com> > Tested-by: David Herrmann <dh.herrmann@googlemail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> > Signed-off-by: Matthew Garrett <mjg@redhat.com> > > (cherry picked from commit ac080523141d5bfa5f60ef2436480f645f915e9c) > BugLink: http://bugs.launchpad.net/bugs/897378 > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > drivers/platform/x86/samsung-laptop.c | 43 +++++++++++++++++++++++++++++++++ > 1 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c > index ec85987..c32dc39 100644 > --- a/drivers/platform/x86/samsung-laptop.c > +++ b/drivers/platform/x86/samsung-laptop.c > @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device; > static struct mutex sabi_mutex; > static struct platform_device *sdev; > static struct rfkill *rfk; > +static bool has_stepping_quirk; > > static int force; > module_param(force, bool, 0); > @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness) > { > u8 user_level = user_brightness + sabi_config->min_brightness; > > + if (has_stepping_quirk && user_level != 0) { > + /* > + * short circuit if the specified level is what's already set > + * to prevent the screen from flickering needlessly > + */ > + if (user_brightness == read_brightness()) > + return; > + > + sabi_set_command(sabi_config->commands.set_brightness, 0); > + } > + > sabi_set_command(sabi_config->commands.set_brightness, user_level); > } > > @@ -390,6 +402,34 @@ static int get_brightness(struct backlight_device *bd) > return (int)read_brightness(); > } > > +static void check_for_stepping_quirk(void) > +{ > + u8 initial_level = read_brightness(); > + u8 check_level; > + > + /* > + * Some laptops exhibit the strange behaviour of stepping toward > + * (rather than setting) the brightness except when changing to/from > + * brightness level 0. This behaviour is checked for here and worked > + * around in set_brightness. > + */ > + > + if (initial_level <= 2) > + check_level = initial_level + 2; > + else > + check_level = initial_level - 2; > + > + has_stepping_quirk = false; > + set_brightness(check_level); > + > + if (read_brightness() != check_level) { > + has_stepping_quirk = true; > + pr_info("enabled workaround for brightness stepping quirk\n"); > + } > + > + set_brightness(initial_level); > +} > + > static int update_status(struct backlight_device *bd) > { > set_brightness(bd->props.brightness); > @@ -813,6 +853,9 @@ static int __init samsung_init(void) > } > } > > + /* Check for stepping quirk */ > + check_for_stepping_quirk(); > + > /* knock up a platform device to hang stuff off of */ > sdev = platform_device_register_simple("samsung", -1, NULL, 0); > if (IS_ERR(sdev)) > -- > 1.7.5.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team >
On Fri, Dec 02, 2011 at 02:29:28PM -0600, Seth Forshee wrote: > From: Jason Stubbs <jasonbstubbs@gmail.com> > > On some Samsung laptops the brightness regulation works slightly different. > All SABI commands except for set_brightness work as expected. The behaviour > of set_brightness is as follows: > > - Setting a new brightness will only step one level toward the new brightness > level. For example, setting a level of 5 when the current level is 2 will > result in a brightness level of 3. > - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated > along with the change in brightness. > - Neither of the above two issues occur when changing from/to brightness > level 0. > > This patch adds detection and a non-intrusive workaround for the above issues. > > Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com> > Tested-by: David Herrmann <dh.herrmann@googlemail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> > Signed-off-by: Matthew Garrett <mjg@redhat.com> > > (cherry picked from commit ac080523141d5bfa5f60ef2436480f645f915e9c) > BugLink: http://bugs.launchpad.net/bugs/897378 > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > drivers/platform/x86/samsung-laptop.c | 43 +++++++++++++++++++++++++++++++++ > 1 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c > index ec85987..c32dc39 100644 > --- a/drivers/platform/x86/samsung-laptop.c > +++ b/drivers/platform/x86/samsung-laptop.c > @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device; > static struct mutex sabi_mutex; > static struct platform_device *sdev; > static struct rfkill *rfk; > +static bool has_stepping_quirk; > > static int force; > module_param(force, bool, 0); > @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness) > { > u8 user_level = user_brightness + sabi_config->min_brightness; > > + if (has_stepping_quirk && user_level != 0) { > + /* > + * short circuit if the specified level is what's already set > + * to prevent the screen from flickering needlessly > + */ > + if (user_brightness == read_brightness()) > + return; > + > + sabi_set_command(sabi_config->commands.set_brightness, 0); > + } > + > sabi_set_command(sabi_config->commands.set_brightness, user_level); So I take it the quirk here is to flick to 0 and back to the new brightness. As we can read the brightness could we just do like: while (user_level != read_brightness()) sabi_set_command(sabi_config->commands.set_brightness, user_level); Either way it looks fine. -apw
On Wed, Dec 07, 2011 at 01:10:23PM +0000, Andy Whitcroft wrote: > On Fri, Dec 02, 2011 at 02:29:28PM -0600, Seth Forshee wrote: > > From: Jason Stubbs <jasonbstubbs@gmail.com> > > > > On some Samsung laptops the brightness regulation works slightly different. > > All SABI commands except for set_brightness work as expected. The behaviour > > of set_brightness is as follows: > > > > - Setting a new brightness will only step one level toward the new brightness > > level. For example, setting a level of 5 when the current level is 2 will > > result in a brightness level of 3. > > - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated > > along with the change in brightness. > > - Neither of the above two issues occur when changing from/to brightness > > level 0. > > > > This patch adds detection and a non-intrusive workaround for the above issues. > > > > Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com> > > Tested-by: David Herrmann <dh.herrmann@googlemail.com> > > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> > > Signed-off-by: Matthew Garrett <mjg@redhat.com> > > > > (cherry picked from commit ac080523141d5bfa5f60ef2436480f645f915e9c) > > BugLink: http://bugs.launchpad.net/bugs/897378 > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > > --- > > drivers/platform/x86/samsung-laptop.c | 43 +++++++++++++++++++++++++++++++++ > > 1 files changed, 43 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c > > index ec85987..c32dc39 100644 > > --- a/drivers/platform/x86/samsung-laptop.c > > +++ b/drivers/platform/x86/samsung-laptop.c > > @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device; > > static struct mutex sabi_mutex; > > static struct platform_device *sdev; > > static struct rfkill *rfk; > > +static bool has_stepping_quirk; > > > > static int force; > > module_param(force, bool, 0); > > @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness) > > { > > u8 user_level = user_brightness + sabi_config->min_brightness; > > > > + if (has_stepping_quirk && user_level != 0) { > > + /* > > + * short circuit if the specified level is what's already set > > + * to prevent the screen from flickering needlessly > > + */ > > + if (user_brightness == read_brightness()) > > + return; > > + > > + sabi_set_command(sabi_config->commands.set_brightness, 0); > > + } > > + > > sabi_set_command(sabi_config->commands.set_brightness, user_level); > > So I take it the quirk here is to flick to 0 and back to the new > brightness. As we can read the brightness could we just do like: > > while (user_level != read_brightness()) > sabi_set_command(sabi_config->commands.set_brightness, > user_level); I actually tried this as on the NF310 as I occasionally see the first part (setting the brightness to zero) succeed and the second part (setting it to the desired value) fail, so I end up with a brightness of 0. These are pretty rare though; I've only seen them when I'm pounding continuously on the backlight controls. When using the loop you suggested, for whatever reason the value the BIOS returns for the current brightness never changes even though the brightness itself is changing, so the loop never terminates. If you read the brightness later on the value is correct, but as long as you're reading it in that loop the value doesn't change. Bouncing through 0 seems to be the most reliable method on these models. The BIOS on the NF310 really just turns out to be utter garbage. There's a number of other things I've found with equally broken behaviors. Seth
On Wed, Dec 07, 2011 at 08:29:07AM -0600, Seth Forshee wrote: > I actually tried this as on the NF310 as I occasionally see the first > part (setting the brightness to zero) succeed and the second part > (setting it to the desired value) fail, so I end up with a brightness of > 0. These are pretty rare though; I've only seen them when I'm pounding > continuously on the backlight controls. > > When using the loop you suggested, for whatever reason the value the > BIOS returns for the current brightness never changes even though the > brightness itself is changing, so the loop never terminates. If you > read the brightness later on the value is correct, but as long as you're > reading it in that loop the value doesn't change. Bouncing through 0 > seems to be the most reliable method on these models. That almost sounds like gcc is not realising the function is volatile and optimising it away. You might want to look at the loop and see if the function is being inlined, and whether the optimiser has neutered it. Stuffing in a full memory barrier in the loop may get gcc's head in the game. -apw
On Thu, Dec 08, 2011 at 10:36:37AM +0000, Andy Whitcroft wrote: > On Wed, Dec 07, 2011 at 08:29:07AM -0600, Seth Forshee wrote: > > > I actually tried this as on the NF310 as I occasionally see the first > > part (setting the brightness to zero) succeed and the second part > > (setting it to the desired value) fail, so I end up with a brightness of > > 0. These are pretty rare though; I've only seen them when I'm pounding > > continuously on the backlight controls. > > > > When using the loop you suggested, for whatever reason the value the > > BIOS returns for the current brightness never changes even though the > > brightness itself is changing, so the loop never terminates. If you > > read the brightness later on the value is correct, but as long as you're > > reading it in that loop the value doesn't change. Bouncing through 0 > > seems to be the most reliable method on these models. > > That almost sounds like gcc is not realising the function is volatile > and optimising it away. You might want to look at the loop and see if > the function is being inlined, and whether the optimiser has neutered > it. Stuffing in a full memory barrier in the loop may get gcc's head in > the game. I was pretty sure that wasn't the case, but I went back and double-checked to be sure. read_brightness is in fact being called for each loop iteration, but the value it returns doesn't always reflect the actual brightness (sometimes the loop works, but rarely).
diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c index ec85987..c32dc39 100644 --- a/drivers/platform/x86/samsung-laptop.c +++ b/drivers/platform/x86/samsung-laptop.c @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device; static struct mutex sabi_mutex; static struct platform_device *sdev; static struct rfkill *rfk; +static bool has_stepping_quirk; static int force; module_param(force, bool, 0); @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness) { u8 user_level = user_brightness + sabi_config->min_brightness; + if (has_stepping_quirk && user_level != 0) { + /* + * short circuit if the specified level is what's already set + * to prevent the screen from flickering needlessly + */ + if (user_brightness == read_brightness()) + return; + + sabi_set_command(sabi_config->commands.set_brightness, 0); + } + sabi_set_command(sabi_config->commands.set_brightness, user_level); } @@ -390,6 +402,34 @@ static int get_brightness(struct backlight_device *bd) return (int)read_brightness(); } +static void check_for_stepping_quirk(void) +{ + u8 initial_level = read_brightness(); + u8 check_level; + + /* + * Some laptops exhibit the strange behaviour of stepping toward + * (rather than setting) the brightness except when changing to/from + * brightness level 0. This behaviour is checked for here and worked + * around in set_brightness. + */ + + if (initial_level <= 2) + check_level = initial_level + 2; + else + check_level = initial_level - 2; + + has_stepping_quirk = false; + set_brightness(check_level); + + if (read_brightness() != check_level) { + has_stepping_quirk = true; + pr_info("enabled workaround for brightness stepping quirk\n"); + } + + set_brightness(initial_level); +} + static int update_status(struct backlight_device *bd) { set_brightness(bd->props.brightness); @@ -813,6 +853,9 @@ static int __init samsung_init(void) } } + /* Check for stepping quirk */ + check_for_stepping_quirk(); + /* knock up a platform device to hang stuff off of */ sdev = platform_device_register_simple("samsung", -1, NULL, 0); if (IS_ERR(sdev))