Bug 88325 - screen brightness regression on resume
Summary: screen brightness regression on resume
Status: CLOSED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Jani Nikula
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-12 14:12 UTC by Daniel Nicoletti
Modified: 2018-12-06 02:23 UTC (History)
2 users (show)

See Also:
i915 platform: I965GM
i915 features: display/backlight


Attachments
DMESG before suspend (125.78 KB, text/plain)
2015-01-12 16:00 UTC, Daniel Nicoletti
no flags Details
DMESG after suspend (123.44 KB, text/plain)
2015-01-12 16:00 UTC, Daniel Nicoletti
no flags Details
drm/i915: quirk backlight present on Macbook 2007 (1.09 KB, patch)
2015-10-29 08:15 UTC, Jani Nikula
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Nicoletti 2015-01-12 14:12:25 UTC
Hi,

On my macbook 2007, when resumming from suspend, the screen gets blank and brightness keys doesnt work.

This works fine on 3.14 an lower kernels, on 3.16 and 3.18 its broken

I dont see any issues on dmesg
Comment 1 Jani Nikula 2015-01-12 14:22:57 UTC
Please add drm.debug=14 module parameter and attach dmesg all the way from boot.

Providing a bisect to find the offending commit may be the fastest way to get results.
Comment 2 Daniel Nicoletti 2015-01-12 16:00:12 UTC
Created attachment 112131 [details]
DMESG before suspend
Comment 3 Daniel Nicoletti 2015-01-12 16:00:40 UTC
Created attachment 112132 [details]
DMESG after suspend
Comment 4 Daniel Nicoletti 2015-01-12 16:02:31 UTC
I thought of bisecting the commit but I'm not very sure of which parts of the code could affect this. Is it possible to give me a list of commits between 3.14 and 3.16 where I could go on trying? 

Thanks
Comment 5 Rodrigo Vivi 2015-01-12 19:24:39 UTC
Hi Daniel,

Could you please retest with latest drm-intel-nightly branch. I believe we have a revert for the offending commit there. Although I'm not sure it was on 3.16.
Anyway it is always good to give a shot at latest drm-intel-nightly.

Better way to bisect is using git bisect.
git bisect start
git bisect good <commit at 3.14>
git bisect bad <commit at -nightly>

Thanks,
Rodrigo.
Comment 6 Daniel Nicoletti 2015-01-12 19:42:21 UTC
Cool I've always saw people saying about git bisect but never thought it was so easy...
Can you please tell me which is the right repo url to clone? drm-intel-nightly on google showed a fdo repo which doesn't seem very uptodate...

Thanks
Comment 7 Jani Nikula 2015-01-13 08:51:38 UTC
(In reply to Rodrigo Vivi from comment #5)
> Better way to bisect is using git bisect.
> git bisect start
> git bisect good <commit at 3.14>
> git bisect bad <commit at -nightly>

If you know 3.16 was indeed bad, you can skip a few iterations with git bisect bad v3.16.

(In reply to Daniel Nicoletti from comment #6)
> Can you please tell me which is the right repo url to clone?
> drm-intel-nightly on google showed a fdo repo which doesn't seem very
> uptodate...

http://cgit.freedesktop.org/drm-intel see git urls at the end.

I usually clone Linus' tree and add other kernel repos as remotes, but it doesn't make a difference to the end result here.
Comment 8 Jani Nikula 2015-01-13 08:53:13 UTC
Also I was hoping to see dmesg all the way from beginning of boot. Please add log_buf_len=4M or something to get more log.
Comment 9 Daniel Nicoletti 2015-01-14 16:55:18 UTC
This is the commit which introduced the regression, by the description it seems to be it:

daniel@bart:~/code/drm-intel$ git bisect good
c675949ec58ca50d5a3ae3c757892f1560f6e896 is the first bad commit
commit c675949ec58ca50d5a3ae3c757892f1560f6e896
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Apr 9 11:31:37 2014 +0300

    drm/i915: do not setup backlight if not available according to VBT
    
    Some machines use an external EC for controlling the backlight. Info
    about this is present in the VBT. Do not setup native backlight control
    if no PWM backlight is available or supported according to VBT. The
    acpi_backlight interface appears to work for the EC control.
    
    In most cases there has been no harm done, but it looks like there are
    machines out there that have both an EC and our PWM line connected to
    the same wire. This, obviously, does not end well.
    
    This should fix the regression caused by
    commit bc0bb9fd1c7810407ab810d204bbaecb255fddde
    Author: Jani Nikula <jani.nikula@intel.com>
    Date:   Thu Nov 14 12:14:29 2013 +0200
    
        drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE
    
    AFAICT the quirk removed by the above commit effectively resulted in
    i915 not driving the backlight PWM output, thus not messing things up.
    
    Additionally this should fix the regression caused by
    commit fbc9fe1b4f222a7c575e3bd8e9defe59c6190a04
    Author: Aaron Lu <aaron.lu@intel.com>
    Date:   Fri Oct 11 21:27:45 2013 +0800
    
        ACPI / video: Do not register backlight if win8 and native interface exists
    
    which left some machines without a functioning backlight interface.
                                                                                                                                                                                    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76276                                                                                                                    
    Reference: https://bugzilla.kernel.org/show_bug.cgi?id=47941                                                                                                                    
    Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=62281                                                                                                                     
    CC: Aaron Lu <aaron.lu@intel.com>                                                                                                                                               
    CC: Eric Griffith <EGriffith92@gmail.com>                                                                                                                                       
    CC: Kent Baxley <kent.baxley@canonical.com>                                                                                                                                     
    Tested-by: Kamal Mostafa <kamal@canonical.com>                                                                                                                                  
    Tested-by: Martin <bugs@mrvanes.com>                                                                                                                                            
    Tested-by: jrg.otte@gmail.com
    Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    Signed-off-by: Jani Nikula <jani.nikula@intel.com>

:040000 040000 9ed3e48521f181c53dfe780b28450e678aa4b9ea b9a776bb2de3ba84f614087619e9e91a2bdcc960 M      drivers
Comment 10 Daniel Nicoletti 2015-01-14 16:56:03 UTC
Do you still need dmesg with log_buf_len=4M?
Comment 11 Jani Nikula 2015-01-16 11:46:59 UTC
Sigh, I think I need 'lspci -vmmnn -s 00:02' instead.
Comment 12 Daniel Nicoletti 2015-01-16 14:20:53 UTC
Slot:   00:02.0
Class:  VGA compatible controller [0300]
Vendor: Intel Corporation [8086]
Device: Mobile GM965/GL960 Integrated Graphics Controller (primary) [2a02]
SVendor:        Apple Inc. [106b]
SDevice:        Device [00a1]
Rev:    03

Slot:   00:02.1
Class:  Display controller [0380]
Vendor: Intel Corporation [8086]
Device: Mobile GM965/GL960 Integrated Graphics Controller (secondary) [2a03]
SVendor:        Apple Inc. [106b]
SDevice:        Device [00a1]
Rev:    03
Comment 13 Daniel Nicoletti 2015-01-21 16:57:47 UTC
For the record I just found out this issue only happens if I'm running a 32bit OS,
I couldn't reproduce on 3.16 running on 64bits.
Comment 14 Chris Wilson 2015-01-21 17:01:10 UTC
UEFI vs BIOS-compat modes?
Comment 15 Daniel Nicoletti 2015-01-21 17:07:45 UTC
Indeed, I didn't noticed I was on EFI mode.
BTW on refit page it says Intel acceleration doesn't work on EFI mode, but using refind and Kwin compositing is working...
Comment 16 Daniel Nicoletti 2015-10-28 23:39:25 UTC
Wouldn't hurt knowing why you won't fix, it's quite frustrating to waste a lot of time figuring an offending commit to 10 months later get a WONTFIX...
Luckily I have a new machine but I still use this old one some times.
Comment 17 Jani Nikula 2015-10-29 08:15:00 UTC
Created attachment 119278 [details] [review]
drm/i915: quirk backlight present on Macbook 2007

Sorry, I guess I misunderstood the issue with EFI and 32-bit. Please try the attached patch, and report back.

What's the macbook "revision" you have (e.g. Macbook 2,1) I'd like to include that in the patch.
Comment 18 Daniel Nicoletti 2015-11-04 21:13:50 UTC
Thanks, I'm recompiling right now, for the revision where do I get it? From OSX or dmidecode?
Comment 19 Daniel Nicoletti 2015-11-04 21:38:35 UTC
The patch works thanks, dmidecode has "MacBook4,1"
Comment 20 Sean Bolton 2015-11-07 02:01:26 UTC
I also have a MacBook4,1 (black, 2008, 2.4GHz Core 2 Duo T8300), and have had the same problem (no backlight after resumption from suspend) on kernels 3.16 and later. Jani's 'drm/i915: quirk backlight present on Macbook 2007' patch on a 4.1.12 kernel solved the problem for me.

I'd really like to see this fix end up in the mainstream kernel, so let me know if there's anything I can do to help.
Comment 21 Jani Nikula 2015-11-11 13:45:50 UTC
I'll likely push the patch to drm-intel-fixes on Monday, provided the v4.4-rc1 is released on schedule.
Comment 22 Jani Nikula 2015-11-16 12:46:19 UTC
commit 1b9448b071caa7d10bb2569fabe3020a2c25ae59
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Thu Nov 5 11:49:59 2015 +0200

    drm/i915: quirk backlight present on Macbook 4, 1

in drm-intel-fixes, with cc: stable.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.