| Message ID | 1392305571-4895-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thursday, February 13, 2014 04:32:51 PM Hans de Goede wrote: > Some devices have duplicate entries in there brightness levels table, ie > on my Dell Latitude E6430 the table looks like this: > > [ 3.686060] acpi backlight index 0, val 80 > [ 3.686095] acpi backlight index 1, val 50 > [ 3.686122] acpi backlight index 2, val 5 > [ 3.686147] acpi backlight index 3, val 5 > [ 3.686172] acpi backlight index 4, val 5 > [ 3.686197] acpi backlight index 5, val 5 > [ 3.686223] acpi backlight index 6, val 5 > [ 3.686248] acpi backlight index 7, val 5 > [ 3.686273] acpi backlight index 8, val 6 > [ 3.686332] acpi backlight index 9, val 7 > [ 3.686356] acpi backlight index 10, val 8 > [ 3.686380] acpi backlight index 11, val 9 > etc. > > Notice that brightness values 0-5 are all mapped to 5. This means that > if userspace writes any value between 0 and 5 to the brightness sysfs attribute > and then reads it, it will always return 0, which is somewhat unexpected. > > This is a problem for ie gnome-settings-daemon, which uses read-modify-write > logic when the users presses the brightness up or down keys. This is done > this way to take brightness changes from other sources into account. > > On this specific laptop what happens once the brightness has been set to 0, > is that gsd reads 0, adds 5, writes 5, and on the next brightness up key press > again reads 0, so things get stuck at the lowest brightness setting. > > Filtering out the duplicate table entries, makes any write to brightness > read back as the written value as one would expect, fixing this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Aaron, can you please have a look at this? > --- > drivers/acpi/video.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index b727d10..ea9d914 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -685,6 +685,7 @@ acpi_video_init_brightness(struct acpi_video_device *device) > union acpi_object *o; > struct acpi_video_device_brightness *br = NULL; > int result = -EINVAL; > + u32 value; > > if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available " > @@ -715,7 +716,12 @@ acpi_video_init_brightness(struct acpi_video_device *device) > printk(KERN_ERR PREFIX "Invalid data\n"); > continue; > } > - br->levels[count] = (u32) o->integer.value; > + value = (u32) o->integer.value; > + /* Skip duplicate entries */ > + if (count > 2 && br->levels[count - 1] == value) > + continue; > + > + br->levels[count] = value; > > if (br->levels[count] > max_level) > max_level = br->levels[count]; >
On 02/13/2014 11:32 PM, Hans de Goede wrote: > Some devices have duplicate entries in there brightness levels table, ie > on my Dell Latitude E6430 the table looks like this: For reference's purpose, can you please file a bug in kernel bugzilla under the ACPI/Power-Video category and attach the acpidump there? Thanks. -Aaron > > [ 3.686060] acpi backlight index 0, val 80 > [ 3.686095] acpi backlight index 1, val 50 > [ 3.686122] acpi backlight index 2, val 5 > [ 3.686147] acpi backlight index 3, val 5 > [ 3.686172] acpi backlight index 4, val 5 > [ 3.686197] acpi backlight index 5, val 5 > [ 3.686223] acpi backlight index 6, val 5 > [ 3.686248] acpi backlight index 7, val 5 > [ 3.686273] acpi backlight index 8, val 6 > [ 3.686332] acpi backlight index 9, val 7 > [ 3.686356] acpi backlight index 10, val 8 > [ 3.686380] acpi backlight index 11, val 9 > etc. > > Notice that brightness values 0-5 are all mapped to 5. This means that > if userspace writes any value between 0 and 5 to the brightness sysfs attribute > and then reads it, it will always return 0, which is somewhat unexpected. > > This is a problem for ie gnome-settings-daemon, which uses read-modify-write > logic when the users presses the brightness up or down keys. This is done > this way to take brightness changes from other sources into account. > > On this specific laptop what happens once the brightness has been set to 0, > is that gsd reads 0, adds 5, writes 5, and on the next brightness up key press > again reads 0, so things get stuck at the lowest brightness setting. > > Filtering out the duplicate table entries, makes any write to brightness > read back as the written value as one would expect, fixing this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/video.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index b727d10..ea9d914 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -685,6 +685,7 @@ acpi_video_init_brightness(struct acpi_video_device *device) > union acpi_object *o; > struct acpi_video_device_brightness *br = NULL; > int result = -EINVAL; > + u32 value; > > if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available " > @@ -715,7 +716,12 @@ acpi_video_init_brightness(struct acpi_video_device *device) > printk(KERN_ERR PREFIX "Invalid data\n"); > continue; > } > - br->levels[count] = (u32) o->integer.value; > + value = (u32) o->integer.value; > + /* Skip duplicate entries */ > + if (count > 2 && br->levels[count - 1] == value) > + continue; > + > + br->levels[count] = value; > > if (br->levels[count] > max_level) > max_level = br->levels[count]; > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 02/14/2014 02:16 AM, Aaron Lu wrote: > On 02/13/2014 11:32 PM, Hans de Goede wrote: >> Some devices have duplicate entries in there brightness levels table, ie >> on my Dell Latitude E6430 the table looks like this: > > For reference's purpose, can you please file a bug in kernel bugzilla > under the ACPI/Power-Video category and attach the acpidump there? Thanks. Done: https://bugzilla.kernel.org/show_bug.cgi?id=70571 Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/14/2014 05:05 PM, Hans de Goede wrote: > Hi, > > On 02/14/2014 02:16 AM, Aaron Lu wrote: >> On 02/13/2014 11:32 PM, Hans de Goede wrote: >>> Some devices have duplicate entries in there brightness levels table, ie >>> on my Dell Latitude E6430 the table looks like this: >> >> For reference's purpose, can you please file a bug in kernel bugzilla >> under the ACPI/Power-Video category and attach the acpidump there? Thanks. > > Done: > > https://bugzilla.kernel.org/show_bug.cgi?id=70571 Thanks, and the patch looks good to me. Reviewed-by: Aaron Lu <aaron.lu@intel.com> -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, February 14, 2014 10:53:47 PM Aaron Lu wrote: > On 02/14/2014 05:05 PM, Hans de Goede wrote: > > Hi, > > > > On 02/14/2014 02:16 AM, Aaron Lu wrote: > >> On 02/13/2014 11:32 PM, Hans de Goede wrote: > >>> Some devices have duplicate entries in there brightness levels table, ie > >>> on my Dell Latitude E6430 the table looks like this: > >> > >> For reference's purpose, can you please file a bug in kernel bugzilla > >> under the ACPI/Power-Video category and attach the acpidump there? Thanks. > > > > Done: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=70571 > > Thanks, and the patch looks good to me. > Reviewed-by: Aaron Lu <aaron.lu@intel.com> OK, thanks! Is this stable material? If so, which stable kernels should it go into?
Hi, On 02/15/2014 02:00 AM, Rafael J. Wysocki wrote: > On Friday, February 14, 2014 10:53:47 PM Aaron Lu wrote: >> On 02/14/2014 05:05 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 02/14/2014 02:16 AM, Aaron Lu wrote: >>>> On 02/13/2014 11:32 PM, Hans de Goede wrote: >>>>> Some devices have duplicate entries in there brightness levels table, ie >>>>> on my Dell Latitude E6430 the table looks like this: >>>> >>>> For reference's purpose, can you please file a bug in kernel bugzilla >>>> under the ACPI/Power-Video category and attach the acpidump there? Thanks. >>> >>> Done: >>> >>> https://bugzilla.kernel.org/show_bug.cgi?id=70571 >> >> Thanks, and the patch looks good to me. >> Reviewed-by: Aaron Lu <aaron.lu@intel.com> > > OK, thanks! > > Is this stable material? If so, which stable kernels should it go into? Not sure if you're asking me or Aaron. I did not think about this before, but I think adding this to stable makes sense. I'll let Aaron judge how far it should be backported. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 15, 2014 at 02:00:50AM +0100, Rafael J. Wysocki wrote: > On Friday, February 14, 2014 10:53:47 PM Aaron Lu wrote: > > On 02/14/2014 05:05 PM, Hans de Goede wrote: > > > Hi, > > > > > > On 02/14/2014 02:16 AM, Aaron Lu wrote: > > >> On 02/13/2014 11:32 PM, Hans de Goede wrote: > > >>> Some devices have duplicate entries in there brightness levels table, ie > > >>> on my Dell Latitude E6430 the table looks like this: > > >> > > >> For reference's purpose, can you please file a bug in kernel bugzilla > > >> under the ACPI/Power-Video category and attach the acpidump there? Thanks. > > > > > > Done: > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=70571 > > > > Thanks, and the patch looks good to me. > > Reviewed-by: Aaron Lu <aaron.lu@intel.com> > > OK, thanks! > > Is this stable material? If so, which stable kernels should it go into? Yes, all stables I think. Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, February 15, 2014 07:46:41 PM Aaron Lu wrote: > On Sat, Feb 15, 2014 at 02:00:50AM +0100, Rafael J. Wysocki wrote: > > On Friday, February 14, 2014 10:53:47 PM Aaron Lu wrote: > > > On 02/14/2014 05:05 PM, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 02/14/2014 02:16 AM, Aaron Lu wrote: > > > >> On 02/13/2014 11:32 PM, Hans de Goede wrote: > > > >>> Some devices have duplicate entries in there brightness levels table, ie > > > >>> on my Dell Latitude E6430 the table looks like this: > > > >> > > > >> For reference's purpose, can you please file a bug in kernel bugzilla > > > >> under the ACPI/Power-Video category and attach the acpidump there? Thanks. > > > > > > > > Done: > > > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=70571 > > > > > > Thanks, and the patch looks good to me. > > > Reviewed-by: Aaron Lu <aaron.lu@intel.com> > > > > OK, thanks! > > > > Is this stable material? If so, which stable kernels should it go into? > > Yes, all stables I think. OK, thanks! Queued up for the next PM/ACPI pull request.
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index b727d10..ea9d914 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -685,6 +685,7 @@ acpi_video_init_brightness(struct acpi_video_device *device) union acpi_object *o; struct acpi_video_device_brightness *br = NULL; int result = -EINVAL; + u32 value; if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) { ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available " @@ -715,7 +716,12 @@ acpi_video_init_brightness(struct acpi_video_device *device) printk(KERN_ERR PREFIX "Invalid data\n"); continue; } - br->levels[count] = (u32) o->integer.value; + value = (u32) o->integer.value; + /* Skip duplicate entries */ + if (count > 2 && br->levels[count - 1] == value) + continue; + + br->levels[count] = value; if (br->levels[count] > max_level) max_level = br->levels[count];
Some devices have duplicate entries in there brightness levels table, ie on my Dell Latitude E6430 the table looks like this: [ 3.686060] acpi backlight index 0, val 80 [ 3.686095] acpi backlight index 1, val 50 [ 3.686122] acpi backlight index 2, val 5 [ 3.686147] acpi backlight index 3, val 5 [ 3.686172] acpi backlight index 4, val 5 [ 3.686197] acpi backlight index 5, val 5 [ 3.686223] acpi backlight index 6, val 5 [ 3.686248] acpi backlight index 7, val 5 [ 3.686273] acpi backlight index 8, val 6 [ 3.686332] acpi backlight index 9, val 7 [ 3.686356] acpi backlight index 10, val 8 [ 3.686380] acpi backlight index 11, val 9 etc. Notice that brightness values 0-5 are all mapped to 5. This means that if userspace writes any value between 0 and 5 to the brightness sysfs attribute and then reads it, it will always return 0, which is somewhat unexpected. This is a problem for ie gnome-settings-daemon, which uses read-modify-write logic when the users presses the brightness up or down keys. This is done this way to take brightness changes from other sources into account. On this specific laptop what happens once the brightness has been set to 0, is that gsd reads 0, adds 5, writes 5, and on the next brightness up key press again reads 0, so things get stuck at the lowest brightness setting. Filtering out the duplicate table entries, makes any write to brightness read back as the written value as one would expect, fixing this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/video.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)