Bug 27452

Summary:	Backlight control broken on Vaio SZ650
Product:	ACPI	Reporter:	Michael Doube (michael)
Component:	Power-Video	Assignee:	Rafael J. Wysocki (rjw)
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	acpi-bugzilla, chris, florian, lenb, malattia, rjw, yakui.zhao
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	2.6.38-rc2	Subsystem:
Regression:	Yes	Bisected commit-id:
Bug Depends on:
Bug Blocks:	27352
Attachments:	output of acpidump \| cat > acpidump.txt

Description Michael Doube 2011-01-23 20:26:38 UTC

This commit introduces a bug that prevents backlight control from either hotkeys or xbacklight.  The video adapter is a Intel Corporation Mobile GM965/GL960 Integrated Graphics Controller (rev 0c).

6d5bbf00d251cc73223a71422d69e069dc2e0b8d is the first bad commit
commit 6d5bbf00d251cc73223a71422d69e069dc2e0b8d
Author: Len Brown <len.brown@intel.com>
Date:   Fri Jan 7 01:46:40 2011 +0100

    ACPI: Use ioremap_cache()
    
    Although the temporary boot-time ACPI table mappings
    were set up with CPU caching enabled, the permanent table
    mappings and AML run-time region memory accesses were
    set up with ioremap(), which on x86 is a synonym for
    ioremap_nocache().
    
    Changing this to ioremap_cache() improves performance as
    seen when accessing the tables via acpidump,
    or /sys/firmware/acpi/tables.  It should also improve
    AML run-time performance.
    
    No change on ia64.
    
    Reported-by: Jack Steiner <steiner@sgi.com>
    Signed-off-by: Len Brown <len.brown@intel.com>
    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

:040000 040000 2657fb60f4b7176b4c3599d8723222eef4c6c58d a0aa669cc80f2804ebc73818e8df16d7cc147cdf M	arch
:040000 040000 a27e58b84f29112d0d2717d71e52fafba08cfdaf 3c51f290a55060d35f04cc1a8768c3b0b5b68b36 M	drivers

Comment 1 Len Brown 2011-01-23 20:48:20 UTC

as there have been a couple of changes in this area,
please verify that the problem is still there with 2.6.38-rc2

please attach the output from acpidump

Comment 2 Michael Doube 2011-01-23 23:35:02 UTC

Confirmed on 2.6.38-rc2 to be still present.

Comment 3 Michael Doube 2011-01-23 23:35:59 UTC

Created attachment 44972 [details]
output of acpidump | cat > acpidump.txt

Comment 4 Rafael J. Wysocki 2011-01-24 21:41:34 UTC

Please verify that replacing ioremap_cache() with ioremap() in
include/linux/acpi_io.h:acpi_os_ioremap() fixes the issue for you.

Comment 5 ykzhao 2011-01-25 04:51:34 UTC

Hi,
   Does there exist the interface of /sys/class/backlight/*/brightness ? Can the brightness be changed by using /sys/class/backlight/*/brightness?
    >echo X > /sys/class/backlight/*/brightness

Thanks.

Comment 6 Michael Doube 2011-01-25 08:33:19 UTC

Testing with 2.6.38-rc2:
/sys/class/backlight/sony/brightness exists but writing values to it changes nothing (backlight burns at 100% regardless of the value held by brightness file).

Comment 7 Michael Doube 2011-01-25 10:14:58 UTC

In response to Rafael in comment #4 above, yes, making that replacement fixes the issue for me.

Comment 8 Florian Mickler 2011-01-29 11:23:16 UTC

First-Bad-Commit: 6d5bbf00d251cc73223a71422d69e069dc2e0b8d

Comment 9 ykzhao 2011-02-01 01:01:55 UTC

From the acpidump it seems that SMI operation will be executed when trying to change the brightness level. And the following memory opregion in AML code is used in couse of triggering SMI.
     >OperationRegion (SMI0, SystemMemory, 0xBF6DEB38, 0x00000415)
    Field (SMI0, AnyAcc, NoLock, Preserve)
    {
        BCMD,   8,
        DID,    32,
        INFO,   4096
    }
   
    Before the commit 6d5bbf00d, the corresponding memory is mapped as uncacheable. After it is mapped as cacheable, it is not clear whether the system still can keep the cache coherent after triggering the SMI operation. Maybe it is safer to still map them as uncacheable.

Thanks.

Comment 10 Rafael J. Wysocki 2011-02-01 10:40:25 UTC

I agree, but the question is what we should do now.

I think that operation regions should be mapped as uncacheable, so we'll
need a special OS callback for mapping them.  I'll try to prepare a patch for
testing.

Comment 11 Michael Doube 2011-02-01 15:22:49 UTC

This bug is gone in 2.6.38-rc3.  Is this expected, or should I reverse bisect to find the commit that fixed it?

Comment 12 Rafael J. Wysocki 2011-02-01 18:35:17 UTC

Well, if you could do that, I would appreciate it.

I _think_ it has been fixed by one of the i915 changes that went in after -rc2.

Anyway, I'm closing the bug as fixed.

Comment 13 Michael Doube 2011-02-04 10:44:56 UTC

This bug was fixed by:

commit b705120e4198315f4ae043de06c62f65e0851fd3
Author: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
Date:   Sun Jan 23 18:17:17 2011 +0000

    drm/i915: Use consistent mappings for OpRegion between ACPI and i915
    
    The opregion is a shared memory region between ACPI and the graphics
    driver. As the ACPI mapping has been changed to cachable in commit
    6d5bbf00d251cc73223a71422d69e069dc2e0b8d, mapping the intel opregion
    non-cachable now fails. As no bus-master hardware is involved in the
    opregion, cachable map should do no harm.
    
    Tested on a Fujitsu Lifebook P8010.
    
    Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
    [ickle: convert to acpi_os_ioremap for consistency]
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

:040000 040000 fc5080e1daa2c8d8e113ee0c71c9a267e524520c 192d5b3b1f7f2c6665a8a7f51b5af30e868963d9 M	drivers

Comment 14 Florian Mickler 2011-03-30 21:50:07 UTC

Thanks for digging that up. I just tried to trace if it was queued for stable, but it seems to have not yet been included. But I found a bisection result[1] returning this commit on lkml... so maybe it shouldn't go to stable? or at least needs a fixup patch on top. 

Chris, do you already track this for stable? Or by chance now the correct patch sequence to go to stable?

[1]: https://patchwork.kernel.org/patch/514761/

Comment 15 Florian Mickler 2011-03-30 21:51:35 UTC

ok, never mind. This was fixed in time for 2.6.38, so all is well, no need to backport it.