Another week, another -rc.
Nothing much stands out here - we've fixed some regressions
(including, hopefully, a BUG_ON() due to the new RCU filename lookup
that Ubuntu people saw), and things look fairly normal.
Most of the changes are pretty spread out and small, with drivers/gpu
(radeon and i915) somewhat standing out from the pack. And to a lesser
degree drivers/rtc (converting some missed drivers to the
alarm_irq_enable methods) along with some ext4 updates (mainly the
"serialize unaligned async DIO" thing) standing out as being larger
than average.
There's also some device tree documentation movement that looks huge
if you look at traditional patches, but it's just moving files out of
the powerpc directory (since the whole device tree thing is generic
these days).
But most of it is pretty small. The appended ShortLog gives some flavor of it,
Linus
---
Ajit Khaparde (1):
benet: Avoid potential null deref in be_cmd_get_seeprom_data()
Al Viro (5):
nothing in do_follow_link() is going to see RCU
in do_lookup() split RCU and non-RCU cases of need_revalidate
split do_revalidate() into RCU and non-RCU cases
drop out of RCU in return_reval
get rid of nameidata_dentry_drop_rcu() calling nameidata_drop_rcu()
Alan Stern (3):
USB: prevent buggy hubs from crashing the USB stack
USB: fix race between root-hub resume and wakeup requests
USB: usb-storage: unusual_devs entry for Coby MP3 player
Alex Deucher (9):
drm/radeon/kms: fix interlaced modes on dce4+
drm/radeon/kms: add connector table for mac g5 9600
drm/radeon/kms: evergreen/ni big endian fixes (v2)
drm/radeon/kms: use linear aligned for 6xx/7xx bo blits
drm/radeon/kms: use linear aligned for evergreen/ni bo blits
drm/radeon/kms: improve 6xx/7xx CS error output
drm/radeon/kms: fix a few more atombios endian issues
drm/radeon/kms: add bounds checking to avivo pll algo
drm/radeon/kms: hopefully fix pll issues for real (v3)
Alexander Duyck (1):
ixgbe: limit VF access to network traffic
Alexander Stein (1):
Input: rotary_encoder - use proper irqflags
Alexander Strakh (2):
Input: wacom - fix error path in wacom_probe()
drivers/rtc/rtc-proc.c: add module_put on error path in rtc_proc_open()
Alexey Orishko (2):
CDC NCM errata updates for cdc.h
USB CDC NCM errata updates for cdc_ncm host driver
Amit Shah (3):
virtio: console: Move file back to drivers/char/
virtio: console: Wake up outvq on host notifications
virtio: console: Update Copyright
Anatolij Gustschin (1):
dma: ipu_idmac: do not lose valid received data in the irq handler
Andrea Arcangeli (1):
thp: prevent hugepages during args/env copying into the user stack
Anisse Astier (1):
ALSA: hda - add quirk for Ordissimo EVE using a realtek ALC662
Ari Kauppi (2):
oprofile: Fix usage of CONFIG_HW_PERF_EVENTS for
oprofile_perf_init and friends
ARM: oprofile: Fix backtraces in timer mode
Arnaldo Carvalho de Melo (1):
perf tools: Fix thread_map event synthesizing in top and record
Arvid Ephraim Picciani (1):
USB: cdc-acm: Adding second ACM channel support for Nokia N8
Ben Hutchings (1):
68360serial: Plumb in rs_360_get_icount()
Benny Halevy (1):
NFSD: use nfserr for status after decode_cb_op_status
Bjørn Forsman (1):
ARM: pxa/colibri: use correct SD detect pin
Bjørn Mork (1):
USB: io_edgeport: fix the reported firmware major and minor
Boaz Harrosh (1):
vfs: call rcu_barrier after ->kill_sb()
Bob Liu (1):
usb: musb: hsdma: change back to use musb_read/writew
Borislav Petkov (2):
amd64_edac: Fix DIMMs per DCTs output
x86: Fix mwait_usable section mismatch
Bruce Rogers (1):
virtio_net: Add schedule check to napi_enable call
Chris Mason (3):
md_make_request: don't touch the bio after calling make_request
Btrfs: fix page->private races
Btrfs: don't release pages when we can't clear the uptodate bits
Chris Wilson (7):
drm/i915: Invalidate TLB caches on SNB BLT/BSD rings
drm/i915/lvds: Restore dithering on native modes for gen2/3
drm/i915: Disable RC6 on Ironlake
drm/i915/sdvo: If we have an EDID confirm it matches the mode of
the connection
drm/i915: Trigger modesetting if force-audio changes
drm/i915/tv: Use polling rather than interrupt-based hotplug
drm/i915: Fix resume regression from 5d1d0cc
Chris Wright (3):
security: add cred argument to security_capable()
pci: use security_capable() when checking capablities during
config space read
pci: use security_capable() when checking capablities during
config space read
Christian Lamparter (1):
carl9170: fix typo in PS code
Clemens Ladisch (2):
ALSA: hrtimer: handle delayed timer interrupts
ALSA: hrtimer: remove superfluous tasklet invocation
Corey Minyard (1):
char/ipmi: fix OOPS caused by pnp_unregister_driver on unregistered driver
Curt Wohlgemuth (1):
ext4: Fix data corruption with multi-block writepages support
Cédric Cano (3):
drm/radeon: 6xx/7xx non-kms endian fixes
drm/radeon/kms: atombios big endian fixes
drm/radeon/kms: 6xx/7xx big endian fixes
Dan Rosenberg (1):
btrfs: prevent heap corruption in btrfs_ioctl_space_info()
Dan Williams (1):
dmaengine: add slave-dma maintainer
Dave Airlie (2):
drm/radeon: fix memory debugging since
d961db75ce86a84f1f04e91ad1014653ed7d9f46
drm/radeon: fix race between GPU reset and TTM delayed delete thread.
Dave Martin (1):
ARM: 6659/1: Thumb-2: Make CONFIG_OABI_COMPAT depend on
!CONFIG_THUMB2_KERNEL
David Henningsson (1):
ALSA: HDA: Add subwoofer quirk for Acer Aspire 8942G
David Miller (1):
klist: Fix object alignment on 64-bit.
David S. Miller (4):
net/caif: Fix dangling list pointer in freed object on error.
net: Fix lockdep regression caused by initializing netdev queues
too early.
isdn: hysdn: Kill (partially buggy) CVS regision log reporting.
x25: Do not reference freed memory.
David Teigland (1):
dlm: use single thread workqueues
Dirk Eibach (1):
hwmon: (lm63) Consider LM64 temperature offset
Dmitry Eremin-Solenikov (1):
ARM: 6658/1: collie: do actually pass locomo_info to locomo driver
Dmitry Torokhov (3):
Input: sysrq - rework re-inject logic
Revert "Input: do not pass injected events back to the
originating handler"
Input: ads7846 - check proper condition when freeing gpio
Don Fry (1):
iwlagn: Re-enable RF_KILL interrupt when down
Don Skidmore (3):
ixgbe: fix for 82599 erratum on Header Splitting
ixgbe: cleanup variable initialization
ixgbe: update version string
Don Zickus (1):
watchdog, nmi: Lower the severity of error messages
Duncan Laurie (1):
Input: serio - clear pending rescans after sysfs driver rebind
Emil Tantilov (1):
ixgbe: fix variable set but not used warnings by gcc 4.6
Eric Miao (1):
ARM: pxa: only save/restore registers when pm functions are defined
Eric Sandeen (3):
ext4: fix panic on module unload when stopping lazyinit thread
ext4: make grpinfo slab cache names static
ext4: serialize unaligned asynchronous DIO
Felipe Balbi (1):
usb: musb: disable double buffering when it's broken
Felix Fietkau (1):
mac80211: fix the skb cloned check in the tx path
Florian Fainelli (1):
e1000: add support for Marvell Alaska M88E1118R PHY
Geert Uytterhoeven (1):
m68knommu: Remove dependencies on nonexistent M68KNOMMU
Grant Likely (6):
dt: Move device tree documentation out of powerpc directory
dt: Remove obsolete description of powerpc boot interface
dt: add documentation of ARM dt boot interface
MAINTAINERS: Add entry for GPIO subsystem
MAINTAINERS: Add entry for GPIO subsystem
Revert "dt: add documentation of ARM dt boot interface"
Greg Ungerer (8):
m68knommu: fix use of un-defined _TIF_WORK_MASK
m68k: remove arch specific non-optimized memcmp()
m68knommu: add optimize memmove() function
m68knommu: fix mis-named variable int set_irq_chip loop
m68knommu: add missing linker __modver section
m68knommu: fix dereference of port.tty
m68knommu: remove use of IRQ_FLG_LOCK from 68360 platform support
m68knommu: set flow handler for secondary interrupt controller of 5249
Guennadi Liakhovetski (1):
spi/spi_sh_msiof: fix wrong address calculation, which leads to an Oops
Guenter Roeck (1):
hwmon: (emc1403) Fix I2C address range
Haiyang Zhang (1):
staging: hv: Enable sending GARP packet after live migration
Harsha Priya (1):
staging: sst: Fix for dmic capture on v2 pmic
Ian Abbott (2):
Staging: comedi: Add MODULE_LICENSE and similar to NI modules
Staging: Comedi: Fix a few NI module dependencies
Ilya Dryomov (1):
Btrfs - Fix memory leak in btrfs_init_new_device()
Ionut Nicu (1):
USB: ti_usb: fix module removal
J. Bruce Fields (9):
nfsd: don't leak dentry count on mnt_want_write failure
nfsd4: split up nfsd_break_deleg_cb
nfsd4: add helper function for lease setup
nfsd4: fix leak on allocation error
nfsd4: split lease setting into separate function
nfsd4: remove unused deleg dprintk's.
nfsd4: modify fi_delegations under recall_lock
nfsd4: acquire only one lease per file
nfsd: break lease on unlink due to rename
Jan Beulich (1):
x86: Fix section mismatch in LAPIC initialization
Janusz Krzysztofik (1):
ASoC: fill in snd_soc_pcm_runtime.card before calling
snd_soc_dai_link.init()
Jarkko Nikula (1):
usb: ehci-omap: Show fatal probing time errors to end user
Jean-Christophe PLAGNIOL-VILLARD (1):
USB: ftdi_sio: add ST Micro Connect Lite uart support
Jeff Layton (2):
cifs: clean up checks in cifs_echo_request
cifs: don't always drop malformed replies on the floor (try #3)
Jesper Juhl (5):
wireless, wl1251: Fix potential NULL pointer dereference in
wl1251_op_bss_info_changed()
USB SL811HS HCD: Fix memory leak in sl811h_urb_enqueue()
USB, Mass Storage, composite, gadget: Fix build failure and
memset of a struct
sis900: Fix mem leak in sis900_rx error path
radeon mkregtable: Add missing fclose() calls
Jesse Brandeburg (1):
e1000e: tx_timeout should not increment for non-hang events
Joerg Roedel (1):
KVM: SVM: Make sure KERNEL_GS_BASE is valid when loading gs_index
Johannes Berg (1):
mac80211: fix TX status cookie in HW offload case
Johannes Weiner (1):
vmscan: fix zone shrinking exit when scan work is done
John Stultz (3):
RTC: Fix rtc driver ioctl specific shortcutting
RTC: Convert rtc drivers to use the alarm_irq_enable method
RTC: Fix minor compile warning
Joseph Teichman (1):
ALSA: usbaudio - Enable the E-MU 0204 USB
Julia Lawall (1):
drivers/w1/masters/omap_hdq.c: add missing clk_put
Justin TerAvest (1):
cfq-iosched: Don't wait if queue already has requests.
KAMEZAWA Hiroyuki (1):
memcg: fix leak of accounting at failure path of hugepage collapsing
Kees Cook (2):
timer debug: Hide kernel addresses via %pK in /proc/timer_list
drm: do not leak kernel addresses via /proc/dri/*/vma
Ken Mills (1):
n_gsm: copy mtu over when configuring via ioctl interface
Konstantin Khorenko (1):
NFSD: memory corruption due to writing beyond the stat array
Krzysztof Wojcik (2):
Add raid1->raid0 takeover support
FIX: md: process hangs at wait_barrier after 0->10 takeover
Kukjin Kim (1):
ARM: S5PV310: Cleanup System MMU
Len Brown (1):
tools: turbostat: style updates
Linus Torvalds (4):
cap_syslog: accept CAP_SYS_ADMIN for now
Fix possible filp_cachep memory corruption
Revert "pci: use security_capable() when checking capablities
during config space read"
Linux 2.6.38-rc5
Lukas Czerner (1):
ext4: unregister features interface on module unload
Marek Olšák (3):
drm/radeon/kms: optimize CS state checking for r100->r500
drm/radeon/kms: fix tracking of BLENDCNTL, COLOR_CHANNEL_MASK,
and GB_Z on r300
drm/radeon/kms: check AA resolve registers on r300
Marek Vasut (1):
ARM: pxa: Properly configure PWM period for palm27x
Mark Brown (3):
ASoC: Create an AIF1ADCDAT signal widget to match AIF2
ASoC: Improve WM8994 digital power sequencing
ARM: SAMSUNG: Ensure struct sys_device is declared in plat/pm.h
Martin Schwidefsky (1):
s390: remove task_show_regs
Mian Yousaf Kaukab (2):
usb: musb: maintain three states for buffer mappings instead of two
usb: musb: introduce api for dma code to check compatibility
with usb request
Michael Buesch (1):
ssb-pcmcia: Fix parsing of invariants tuples
Michael Karcher (1):
ACPI / Video: Probe for output switch method when searching video devices.
Michael Williamson (1):
USB: ftdi_sio: Add VID=0x0647, PID=0x0100 for Acton Research spectrograph
Michal Simek (4):
microblaze: Fix IRQ flag handling for MSR=0
microblaze: Fix asm compilation warning
microblaze: Fix pte_update function
microblaze: Fix msr instruction detection
Michel Lespinasse (2):
mlock: fix race when munlocking pages in do_wp_page()
mlock: do not munlock pages in __do_fault()
Ming Lei (1):
usb: musb: fix kernel panic during s2ram(v2)
Mohammed Shafi Shajakhan (1):
ath9k: Fix possible double free of PAPRD skb's
Naga Chumbalkar (1):
x86, dmi, debug: Log board name (when present) in dmesg/oops output
NeilBrown (8):
md: revert change to raid_disks on failure.
md: simplify some 'if' conditionals in raid5_start_reshape.
md: fix the test for finding spares in raid5_start_reshape.
md: don't abort checking spares as soon as one cannot be added.
md: Remove the AllReserved flag for component devices.
md: Don't use remove_and_add_spares to remove failed devices
from a read-only array
md: don't clear curr_resync_completed at end of resync.
md: Don't allow slot_store while resync/recovery is happening.
Nick Holloway (1):
USB: Storage: Add unusual_devs entry for VTech Kidizoom
Nicolas de Pesloüan (1):
deb-pkg: Fix building outside of source tree (O=...).
Nitin Gupta (1):
staging: zram: fix data corruption issue
Pablo Neira Ayuso (1):
netfilter: nf_conntrack: set conntrack templates again if we
return NF_REPEAT
Paul Bolle (2):
devicetree-discuss is moderated for non-subscribers
x86, ioapic: Don't warn about non-existing IOAPICs if we have none
Pavankumar Kondeti (1):
USB: Fix trout build failure with ci13xxx_msm gadget
Peter Zijlstra (2):
Revert "lockdep, timer: Fix del_timer_sync() annotation"
x86: Fix text_poke_smp_batch() deadlock
Philippe De Muyter (2):
m68knommu: fix m548x_wdt.c compilation after headers renaming
m68knommu: Rename m548x_wdt.c to m54xx_wdt.c
Ping Cheng (1):
Input: wacom_w8001 - report resolution to userland
Rabin Vincent (1):
ARM: 6654/1: perf/oprofile: fix off-by-one in stack check
Rafael J. Wysocki (3):
ACPI: Fix acpi_os_read_memory() and acpi_os_write_memory() (v2)
ACPI / ACPICA: Avoid crashing if _PRW is defined for the root object
ACPI / Wakeup: Enable button GPEs unconditionally during initialization
Randy Dunlap (1):
can: softing_cs needs slab.h
Roland Stigge (1):
drivers/gpio/pca953x.c: add a mutex to fix race condition
Roland Vossen (1):
staging: brcm80211: bugfix for softmac crash on multi cpu configurations
Russell King (3):
ARM: Avoid building unsafe kernels on OMAP2 and MX3
ARM: make SWP emulation explicit on !CPU_USE_DOMAINS
ARM: fixup SMP alternatives in modules
Russell King - ARM Linux (2):
DMA: PL08x: fix infinite wait when terminating transfers
DMA: PL08x: fix channel pausing to timeout rather than lockup
Sascha Hauer (10):
dmaengine i.MX SDMA: Fix firmware loading
dmaengine i.MX sdma: set maximum segment size for our device
dmaengine i.MX sdma: check sg entries for valid addresses and lengths
dmaengine i.MX SDMA: do not initialize chan_id field
dmaengine i.MX SDMA: initialize dma capabilities outside channel loop
dmaengine i.MX SDMA: reserve channel 0 by not registering it
dmaengine i.MX dma: set maximum segment size for our device
dmaengine i.MX dma: check sg entries for valid addresses and lengths
dmaengine i.MX DMA: do not initialize chan_id field
dmaengine i.MX dma: initialize dma capabilities outside channel loop
Sergei Shtylyov (1):
usb: musb: core: fix IRQ check
Sergey Senozhatsky (1):
loop: queue_lock NULL pointer derefence in blk_throtl_exit
Shawn Guo (7):
dmaengine: imx-sdma: propagate error in sdma_probe() instead of
returning 0
dmaengine: imx-sdma: fix inconsistent naming in sdma_assign_cookie()
dmaengine: imx-sdma: remove IMX_DMA_SG_LOOP handling in
sdma_prep_slave_sg()
dmaengine: imx-sdma: set sdmac->status to DMA_ERROR in err_out
of sdma_prep_slave_sg()
dmaengine: imx-sdma: return sdmac->status in sdma_tx_status()
dmaengine: imx-sdma: correct sdmac->status in sdma_handle_channel_loop()
dmaengine: imx-sdma: fix up param for the last BD in sdma_prep_slave_sg()
Simon Arlott (1):
cdrom: support devices that have check_events but not media_changed
Sonic Zhang (1):
serial: bfin_5xx: split uart RX lock from uart port lock to avoid deadlock
Soren Hansen (1):
nbd: remove module-level ioctl mutex
Stefan Berger (1):
tpm_tis: Use timeouts returned from TPM
Stephen M. Cameron (1):
cciss: make cciss_revalidate not loop through CISS_MAX_LUNS
volumes unnecessarily.
Steve French (1):
[CIFS] Do not send SMBEcho requests on new sockets until SMBNegotiate
Sven Eckelmann (1):
batman-adv: Linearize fragment packets before merge
Takashi Iwai (2):
ALSA: hda - Fix missing CA initialization for HDMI/DP
ALSA: hda - Don't handle empty patch files
Tao Ma (1):
blktrace: Don't output messages if NOTIFY isn't set.
Tejun Heo (1):
ptrace: use safer wake up on ptrace_detach()
Theodore Ts'o (2):
ext4: fix up ext4 error handling
jbd2: call __jbd2_log_start_commit with j_state_lock write locked
Thomas Abraham (1):
ARM: S5PV310: Add support System MMU on SMDKV310
Thomas Gleixner (1):
x86: Readd missing irq_to_desc() in fixup_irq()
Thomas Renninger (1):
tools: turbostat: fix bitwise and operand
Tim Deegan (1):
fix jiffy calculations in calibrate_delay_direct to handle overflow
Tomoya (3):
pch_can: fix 800k comms issue
pch_can: fix rmmod issue
pch_can: fix module reload issue with MSI
Tomoya MORINAGA (1):
pch_can: fix tseg1/tseg2 setting issue
Toshiharu Okada (1):
pch_gbe: Fix the issue which a driver locks when rx offload is
set by ethtool
Tracey Dent (2):
drivers/block/Makefile: replace the use of <module>-objs with <module>-y
drivers/block/aoe/Makefile: replace the use of <module>-objs
with <module>-y
Trilok Soni (1):
Input: matrix_keypad - increase the limit of rows and columns
Tsutomu Itoh (1):
Btrfs: check return value of alloc_extent_map()
Vaibhav Bedia (1):
asoc: davinci: da830/omap-l137: correct cpu_dai_name
Vivek Goyal (2):
cfq: rename a function to give it more appropriate name
blkio-throttle: Avoid calling blkiocg_lookup_group() for root group
Vladislav Zolotarov (1):
bnx2x: Duplication in promisc mode
Wey-Yi Guy (1):
iwlagn: overwrite EEPROM chain setting for 6250 devices
Will Deacon (2):
ARM: 6656/1: hw_breakpoint: avoid UNPREDICTABLE behaviour when
reading DBGDSCR
ARM: 6657/1: hw_breakpoint: fix ptrace breakpoint advertising on
unsupported arch
Xiao Jiang (1):
drm: fix wrong usages of drm_device in DRM Developer's Guide
Yan, Zheng (1):
Btrfs: Fix balance panic
Yin Kangkai (1):
USB: EHCI: fix scheduling while atomic during suspend
Yinghai Lu (1):
memblock: don't adjust size in memblock_find_base()
Yu Tang (1):
ARM: pxa: fix mfpr_sync to read from valid offset
Yusuke Goda (1):
usb: r8a66597-udc: Fixed bufnum of Bulk
andrew hendry (1):
x25: possible skb leak on bad facilities
maximilian attems (1):
deb-pkg: Use $SRCARCH for include path
Łukasz Wojniłowicz (1):
ALSA: hda - switch lfe with side in mixer for 4930g
Le mardi 15 février 2011 à 20:16 -0800, Linus Torvalds a écrit :
> Another week, another -rc.
>
> Nothing much stands out here - we've fixed some regressions
> (including, hopefully, a BUG_ON() due to the new RCU filename lookup
> that Ubuntu people saw), and things look fairly normal.
Using this kernel on my dev machine (2x4x2 cpus), I hit BUG_ON() in
fs/namei.c:1461 on my kernel build (make -j16)
BUG_ON(inode != next.dentry->d_inode)
I do have DEBUG_PAGEALLOC and LOCKDEP on
Le mercredi 16 février 2011 à 12:14 +0100, Eric Dumazet a écrit :
> Le mardi 15 février 2011 à 20:16 -0800, Linus Torvalds a écrit :
> > Another week, another -rc.
> >
> > Nothing much stands out here - we've fixed some regressions
> > (including, hopefully, a BUG_ON() due to the new RCU filename lookup
> > that Ubuntu people saw), and things look fairly normal.
>
> Using this kernel on my dev machine (2x4x2 cpus), I hit BUG_ON() in
> fs/namei.c:1461 on my kernel build (make -j16)
>
> BUG_ON(inode != next.dentry->d_inode)
>
> I do have DEBUG_PAGEALLOC and LOCKDEP on
>
>
Since I hit this BUG_ON() even on boot, I spent some time tracking it.
I bisected the problem to commit 844a391799c25d9b
(nothing in do_follow_link() is going to see RCU)
Reverting this commit makes my machine happy again.
My filesystems are ext3
# mount
/dev/sda2 on / type ext3 (rw)
none on /proc type proc (rw)
none on /sys type sysfs (rw)
none on /dev/pts type devpts (rw,gid=5,mode=620)
usbfs on /proc/bus/usb type usbfs (rw)
/dev/sda6 on /appli type ext3 (rw)
/dev/sda1 on /boot type ext3 (rw)
none on /dev/shm type tmpfs (rw)
/dev/sda7 on /opt type ext3 (rw)
/dev/sda3 on /var type ext3 (rw)
none on /sys/kernel/debug type debugfs (rw)
none on /proc/sys/fs/binfmt_misc type binfmt_misc (rw)
On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <[email protected]> wrote:
>
> Using this kernel on my dev machine (2x4x2 cpus), I hit BUG_ON() in
> fs/namei.c:1461 on my kernel build (make -j16)
Uhhuh. We replaced one BUG_ON() with another.
And I think it's a really silly problem too: when Al moved the
/* We drop rcu-walk here */
if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
return -ECHILD;
test into do_follow_link(), he _should_ have moved the BUG_ON() in
there too, methinks. He didn't, and as a result the BUG_ON() is now
before the "drop_rcu_maybe".
This patch should fix it. Al? Comments?
(Of course, we could just remove the BUG_ON() entirely, but
considering that this is still fragile new code I'd rather leave it
in)
Linus
On Wed, Feb 16, 2011 at 07:46:20AM -0800, Linus Torvalds wrote:
> On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <[email protected]> wrote:
> >
> > Using this kernel on my dev machine (2x4x2 cpus), I hit BUG_ON() in
> > fs/namei.c:1461 on my kernel build (make -j16)
>
> Uhhuh. We replaced one BUG_ON() with another.
>
> And I think it's a really silly problem too: when Al moved the
>
> /* We drop rcu-walk here */
> if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
> return -ECHILD;
>
> test into do_follow_link(), he _should_ have moved the BUG_ON() in
> there too, methinks. He didn't, and as a result the BUG_ON() is now
> before the "drop_rcu_maybe".
>
> This patch should fix it. Al? Comments?
Sigh... I see what's going on. We'd got inode from dentry that is getting
crapped under us. We will *not* survive dropping RCU - it's bad enough for
full restart in normal mode. So right after we'd seen that (already wrong)
inode has ->follow_link(), we decide to drop RCU. Originally this BUG_ON
hadn't been reached in that case - we had already failed with -ECHILD before
we got to it. Now we don't...
_However_, I don't like passing inode to do_follow_link(). I'd rather set
nd->inode to inode first and use it there. Let me think a bit and see if
it's feasible...
On Wed, Feb 16, 2011 at 04:06:43PM +0000, Al Viro wrote:
> Sigh... I see what's going on. We'd got inode from dentry that is getting
> crapped under us. We will *not* survive dropping RCU - it's bad enough for
> full restart in normal mode. So right after we'd seen that (already wrong)
> inode has ->follow_link(), we decide to drop RCU. Originally this BUG_ON
> hadn't been reached in that case - we had already failed with -ECHILD before
> we got to it. Now we don't...
>
> _However_, I don't like passing inode to do_follow_link(). I'd rather set
> nd->inode to inode first and use it there. Let me think a bit and see if
> it's feasible...
No, that won't do. The damn thing uses previous value of nd->inode if
it walks into relative symlink...
Let's shift that call of nameidata_dentry_drop_rcu_maybe() into both
callers of do_follow_link() instead. Marginally less obvious that we won't
reach the guts of do_follow_link() in RCU mode, just as obvious that overall
structure is ugly as hell and avoids making it even uglier by passing inode
down there. How about this:
diff --git a/fs/namei.c b/fs/namei.c
index 9e701e2..d7003cf 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -800,10 +800,6 @@ static inline int do_follow_link(struct path *path, struct nameidata *nd)
void *cookie;
int err = -ELOOP;
- /* We drop rcu-walk here */
- if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
- return -ECHILD;
-
if (current->link_count >= MAX_NESTED_LINKS)
goto loop;
if (current->total_link_count >= 40)
@@ -1413,6 +1409,9 @@ exec_again:
goto out_dput;
if (inode->i_op->follow_link) {
+ /* We drop rcu-walk here */
+ if (nameidata_dentry_drop_rcu_maybe(nd, next.dentry))
+ return -ECHILD;
BUG_ON(inode != next.dentry->d_inode);
err = do_follow_link(&next, nd);
if (err)
@@ -1458,6 +1457,8 @@ last_component:
break;
if (inode && unlikely(inode->i_op->follow_link) &&
(lookup_flags & LOOKUP_FOLLOW)) {
+ if (nameidata_dentry_drop_rcu_maybe(nd, next.dentry))
+ return -ECHILD;
BUG_ON(inode != next.dentry->d_inode);
err = do_follow_link(&next, nd);
if (err)
Le mercredi 16 février 2011 à 07:46 -0800, Linus Torvalds a écrit :
> On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <[email protected]> wrote:
> >
> > Using this kernel on my dev machine (2x4x2 cpus), I hit BUG_ON() in
> > fs/namei.c:1461 on my kernel build (make -j16)
>
> Uhhuh. We replaced one BUG_ON() with another.
>
> And I think it's a really silly problem too: when Al moved the
>
> /* We drop rcu-walk here */
> if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
> return -ECHILD;
>
> test into do_follow_link(), he _should_ have moved the BUG_ON() in
> there too, methinks. He didn't, and as a result the BUG_ON() is now
> before the "drop_rcu_maybe".
>
> This patch should fix it. Al? Comments?
>
Yes it does, lets see Al patch ;)
> (Of course, we could just remove the BUG_ON() entirely, but
> considering that this is still fragile new code I'd rather leave it
> in)
>
> Linus
On Wed, Feb 16, 2011 at 8:19 AM, Al Viro <[email protected]> wrote:
>
> ? ? ? ?Let's shift that call of nameidata_dentry_drop_rcu_maybe() into both
> callers of do_follow_link() instead. ?Marginally less obvious that we won't
> reach the guts of do_follow_link() in RCU mode, just as obvious that overall
> structure is ugly as hell and avoids making it even uglier by passing inode
> down there. ?How about this:
Well, that just reverts to the old state, so it certainly will work.
But isn't it much nicer to try to keep the shared logic - including
the BUG_ON() - in do_follow_link().
Sure, it means that we have to pass in 'inode', but that's largely
free (just about everything passes three arguments in registers), and
we could eventually decide that we don't need the BUG_ON() any more
and then drop it.
So I don't see the point of duplicating logic just to remove the
(almost free) inode argument.
Does it make tons of conceptual sense to pass in 'inode' to
do_follow_link? No, it's clearly redundant information, which is the
whole point of the BUG_ON(). But it does allow that extra shared
sanity test, and we _could_ also then do
- if (!IS_ERR(cookie) && path->dentry->d_inode->i_op->put_link)
- path->dentry->d_inode->i_op->put_link(path->dentry,
nd, cookie);
+ if (!IS_ERR(cookie) && inode->i_op->put_link)
+ inode->i_op->put_link(path->dentry, nd, cookie);
since we've verified that 'inode' is 'path->dentry->d_inode', and all
of those should be stable over all the calls (in the non-RCU case,
which we are in).
I dunno. I don't care _deeply_, but I do have to say that I much liked
how you moved the
if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
..
into do_follow_link(). I think it made it clearer that do_follow_link
(and __do_follow_link()) aren't done with RCU.
But whatever.
Linus
On Wed, Feb 16, 2011 at 08:33:32AM -0800, Linus Torvalds wrote:
> Does it make tons of conceptual sense to pass in 'inode' to
> do_follow_link? No, it's clearly redundant information, which is the
> whole point of the BUG_ON(). But it does allow that extra shared
> sanity test, and we _could_ also then do
>
> - if (!IS_ERR(cookie) && path->dentry->d_inode->i_op->put_link)
> - path->dentry->d_inode->i_op->put_link(path->dentry,
> nd, cookie);
> + if (!IS_ERR(cookie) && inode->i_op->put_link)
> + inode->i_op->put_link(path->dentry, nd, cookie);
>
> since we've verified that 'inode' is 'path->dentry->d_inode', and all
> of those should be stable over all the calls (in the non-RCU case,
> which we are in).
>
> I dunno. I don't care _deeply_, but I do have to say that I much liked
> how you moved the
>
> if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
> ..
>
> into do_follow_link(). I think it made it clearer that do_follow_link
> (and __do_follow_link()) aren't done with RCU.
OK, I can live with that. Consider me convinced, let's go with your variant.
Speaking of ugliness: Nick, why the _fuck_ have you reverted non-create case
in do_filp_open() to do_path_lookup()?
Le mercredi 16 février 2011 à 16:39 +0000, Al Viro a écrit :
> On Wed, Feb 16, 2011 at 08:33:32AM -0800, Linus Torvalds wrote:
> > Does it make tons of conceptual sense to pass in 'inode' to
> > do_follow_link? No, it's clearly redundant information, which is the
> > whole point of the BUG_ON(). But it does allow that extra shared
> > sanity test, and we _could_ also then do
> >
> > - if (!IS_ERR(cookie) && path->dentry->d_inode->i_op->put_link)
> > - path->dentry->d_inode->i_op->put_link(path->dentry,
> > nd, cookie);
> > + if (!IS_ERR(cookie) && inode->i_op->put_link)
> > + inode->i_op->put_link(path->dentry, nd, cookie);
> >
> > since we've verified that 'inode' is 'path->dentry->d_inode', and all
> > of those should be stable over all the calls (in the non-RCU case,
> > which we are in).
> >
> > I dunno. I don't care _deeply_, but I do have to say that I much liked
> > how you moved the
> >
> > if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
> > ..
> >
> > into do_follow_link(). I think it made it clearer that do_follow_link
> > (and __do_follow_link()) aren't done with RCU.
>
> OK, I can live with that. Consider me convinced, let's go with your variant.
> Speaking of ugliness: Nick, why the _fuck_ have you reverted non-create case
> in do_filp_open() to do_path_lookup()?
I tested both (Linus/Al) versions, please feel free to add
Tested-by: Eric Dumazet <[email protected]>
Signed-off-by: Alex Riesen <[email protected]>
---
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
> Most of the changes are pretty spread out and small, with drivers/gpu
> (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the
lid, and BIOS does not seem to know anything about that (the keyboard
shortcuts to set backlight brightness cause it to jump to the level next to
the one set before closing the lid).
Maybe i915 code for LVDS panels have lost some parts of the backlight
enable/disable balancing patch by Chris Wilson? Or maybe it just got broken
along the way...
This part of the patch by Chris helped here, but I afraid it might be not
complete or just wrong (for instance, the original patch didn't have to remove
the i915_read_blc_pwm_ctl function).
drivers/gpu/drm/i915/intel_panel.c | 65 ++++++++++--------------------------
1 files changed, 18 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -125,55 +125,15 @@ static int is_backlight_combination_mode(struct drm_device *dev)
return 0;
}
-static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
-{
- u32 val;
-
- /* Restore the CTL value if it lost, e.g. GPU reset */
-
- if (HAS_PCH_SPLIT(dev_priv->dev)) {
- val = I915_READ(BLC_PWM_PCH_CTL2);
- if (dev_priv->saveBLC_PWM_CTL2 == 0) {
- dev_priv->saveBLC_PWM_CTL2 = val;
- } else if (val == 0) {
- I915_WRITE(BLC_PWM_PCH_CTL2,
- dev_priv->saveBLC_PWM_CTL);
- val = dev_priv->saveBLC_PWM_CTL;
- }
- } else {
- val = I915_READ(BLC_PWM_CTL);
- if (dev_priv->saveBLC_PWM_CTL == 0) {
- dev_priv->saveBLC_PWM_CTL = val;
- dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
- } else if (val == 0) {
- I915_WRITE(BLC_PWM_CTL,
- dev_priv->saveBLC_PWM_CTL);
- I915_WRITE(BLC_PWM_CTL2,
- dev_priv->saveBLC_PWM_CTL2);
- val = dev_priv->saveBLC_PWM_CTL;
- }
- }
-
- return val;
-}
-
u32 intel_panel_get_max_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
u32 max;
- max = i915_read_blc_pwm_ctl(dev_priv);
- if (max == 0) {
- /* XXX add code here to query mode clock or hardware clock
- * and program max PWM appropriately.
- */
- printk_once(KERN_WARNING "fixme: max PWM is zero.\n");
- return 1;
- }
-
if (HAS_PCH_SPLIT(dev)) {
- max >>= 16;
+ max = I915_READ(BLC_PWM_PCH_CTL2) >> 16;
} else {
+ max = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
max >>= 17;
} else {
@@ -186,6 +146,14 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
max *= 0xff;
}
+ if (max == 0) {
+ /* XXX add code here to query mode clock or hardware clock
+ * and program max PWM appropriately.
+ */
+ DRM_ERROR("fixme: max PWM is zero.\n");
+ max = 1;
+ }
+
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
return max;
}
@@ -255,11 +223,11 @@ void intel_panel_disable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- if (dev_priv->backlight_enabled) {
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
- dev_priv->backlight_enabled = false;
- }
+ if (!dev_priv->backlight_enabled)
+ return;
+ dev_priv->backlight_enabled = false;
+ dev_priv->backlight_level = intel_panel_get_backlight(dev);
intel_panel_set_backlight(dev, 0);
}
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled)
+ return;
+
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
@@ -278,6 +249,6 @@ void intel_panel_setup_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
+ dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
}
--
1.7.4.27.gf5729
On Wed, Feb 16, 2011 at 20:26, Alex Riesen <[email protected]> wrote:
> Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
>> Most of the changes are pretty spread out and small, with drivers/gpu
>> (radeon and i915) somewhat standing out from the pack. ...
>
> The backlight level on this Dell XPS M1330 reduces every time I reopen
> the lid, and BIOS does not seem to know anything about that (the
> keyboard shortcuts to set backlight brightness cause it to jump to the
> level next to the one set before closing the lid).
It is this bug, I believe:
https://bugzilla.kernel.org/show_bug.cgi?id=25072
I somehow missed it at first, and only noticed after sending the patch.
On Wed, 16 Feb 2011 20:46:45 +0100
Alex Riesen <[email protected]> wrote:
> On Wed, Feb 16, 2011 at 20:26, Alex Riesen <[email protected]> wrote:
> > Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
> >> Most of the changes are pretty spread out and small, with drivers/gpu
> >> (radeon and i915) somewhat standing out from the pack. ...
> >
> > The backlight level on this Dell XPS M1330 reduces every time I reopen
> > the lid, and BIOS does not seem to know anything about that (the
> > keyboard shortcuts to set backlight brightness cause it to jump to the
> > level next to the one set before closing the lid).
>
> It is this bug, I believe:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=25072
>
> I somehow missed it at first, and only noticed after sending the patch.
There's also this patch: https://patchwork.kernel.org/patch/499221/.
It got things working on my E6510 again at least.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, Feb 16, 2011 at 20:54, Jesse Barnes <[email protected]> wrote:
> On Wed, 16 Feb 2011 20:46:45 +0100
> Alex Riesen <[email protected]> wrote:
>> > The backlight level on this Dell XPS M1330 reduces every time I reopen
>> > the lid, and BIOS does not seem to know anything about that (the
>> > keyboard shortcuts to set backlight brightness cause it to jump to the
>> > level next to the one set before closing the lid).
>>
>> It is this bug, I believe:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=25072
>>
>> I somehow missed it at first, and only noticed after sending the patch.
>
> There's also this patch: https://patchwork.kernel.org/patch/499221/.
> It got things working on my E6510 again at least.
I don't think it is related. I don't have problems switching
the outputs (frankly, didn't try) and I do have problems
restoring backlight, very similar to what I had earlier in
2.6.37.
On Wed, 16 Feb 2011 20:59:35 +0100
Alex Riesen <[email protected]> wrote:
> On Wed, Feb 16, 2011 at 20:54, Jesse Barnes <[email protected]> wrote:
> > On Wed, 16 Feb 2011 20:46:45 +0100
> > Alex Riesen <[email protected]> wrote:
> >> > The backlight level on this Dell XPS M1330 reduces every time I reopen
> >> > the lid, and BIOS does not seem to know anything about that (the
> >> > keyboard shortcuts to set backlight brightness cause it to jump to the
> >> > level next to the one set before closing the lid).
> >>
> >> It is this bug, I believe:
> >>
> >> https://bugzilla.kernel.org/show_bug.cgi?id=25072
> >>
> >> I somehow missed it at first, and only noticed after sending the patch.
> >
> > There's also this patch: https://patchwork.kernel.org/patch/499221/.
> > It got things working on my E6510 again at least.
>
> I don't think it is related. I don't have problems switching
> the outputs (frankly, didn't try) and I do have problems
> restoring backlight, very similar to what I had earlier in
> 2.6.37.
Right, but it affects the registration of the backlight and ACPI video
interface as well, so can affect backlight restore on resume. In my
case, without the above patch my backlight wouldn't be restored on
resume, so I'd have to manually echo a value
into /sys/class/backlight/<foo> to get my display back.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, Feb 16, 2011 at 21:05, Jesse Barnes <[email protected]> wrote:
> On Wed, 16 Feb 2011 20:59:35 +0100
> Alex Riesen <[email protected]> wrote:
>>
>> I don't think it is related. I don't have problems switching
>> the outputs (frankly, didn't try) and I do have problems
>> restoring backlight, very similar to what I had earlier in
>> 2.6.37.
>
> Right, but it affects the registration of the backlight and ACPI video
> interface as well, so can affect backlight restore on resume. In my
> case, without the above patch my backlight wouldn't be restored on
> resume, so I'd have to manually echo a value
> into /sys/class/backlight/<foo> to get my display back.
Ok, I tried, but only to find out that my tree already has the
patch (it is in mainline since today). So it definitely
does not help.
Thanks anyway
drm/i915: Do not handle backlight combination mode specially.
The current code does not follow Intel documentation: It misses some things
and does other, undocumented things. This causes wrong backlight values in
certain conditions. Instead of adding tricky code handling badly documented
and rare corner cases, don't handle combination mode specially at all. This
way PCI_LBPC is never touched and weird things shouldn't happen.
If combination mode is enabled, then the only downside is that changing the
brightness has a greater granularity (the LBPC value), but LBPC is at most
254 and the maximum is in the thousands, so this is no real functional loss.
A potential problem with not handling combined mode is that a brightness of
max * PCI_LBPC is not bright enough. However, this is very unlikely because
from the documentation LBPC seems to act as a scaling factor and doesn't look
like it's supposed to be changed after boot. The value at boot should always
result in a bright enough screen.
IMPORTANT: However, although usually the above is true, it may not be when
people ran an older (2.6.37) kernel which messed up the LBPC register, and
they are unlucky enough to have a BIOS that saves and restores the LBPC value.
Then a good kernel may seem to not work: Max brightness isn't bright enough.
If this happens people should boot back into the old kernel, set brightness
to the maximum, and then reboot. After that everything should be fine.
For more information see the below links. This fixes bugs:
http://bugzilla.kernel.org/show_bug.cgi?id=23472
http://bugzilla.kernel.org/show_bug.cgi?id=25072
Signed-off-by: Indan Zupancic <[email protected]>
---
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5cfc689..af2fc32 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1551,17 +1551,7 @@
/* Backlight control */
#define BLC_PWM_CTL 0x61254
-#define BACKLIGHT_MODULATION_FREQ_SHIFT (17)
#define BLC_PWM_CTL2 0x61250 /* 965+ only */
-#define BLM_COMBINATION_MODE (1 << 30)
-/*
- * This is the most significant 15 bits of the number of backlight cycles in a
- * complete cycle of the modulated backlight control.
- *
- * The actual value is this field multiplied by two.
- */
-#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17)
-#define BLM_LEGACY_MODE (1 << 16)
/*
* This is the number of cycles out of the backlight modulation cycle for which
* the backlight is on.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..d860abe 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,8 +30,6 @@
#include "intel_drv.h"
-#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
-
void
intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode)
@@ -112,19 +110,6 @@ done:
dev_priv->pch_pf_size = (width << 16) | height;
}
-static int is_backlight_combination_mode(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- if (INTEL_INFO(dev)->gen >= 4)
- return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
-
- if (IS_GEN2(dev))
- return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
-
- return 0;
-}
-
static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
{
u32 val;
@@ -181,9 +166,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
if (INTEL_INFO(dev)->gen < 4)
max &= ~1;
}
-
- if (is_backlight_combination_mode(dev))
- max *= 0xff;
}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -201,15 +183,6 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val >>= 1;
-
- if (is_backlight_combination_mode(dev)){
- u8 lbpc;
-
- val &= ~1;
- pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
- val *= lbpc;
- val >>= 1;
- }
}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -232,16 +205,6 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
-
- if (is_backlight_combination_mode(dev)){
- u32 max = intel_panel_get_max_backlight(dev);
- u8 lpbc;
-
- lpbc = level * 0xfe / max + 1;
- level /= lpbc;
- pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
- }
-
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote:
> Signed-off-by: Alex Riesen <[email protected]>
>
> ---
> Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
> > Most of the changes are pretty spread out and small, with drivers/gpu
> > (radeon and i915) somewhat standing out from the pack. ...
>
> The backlight level on this Dell XPS M1330 reduces every time I reopen the
> lid, and BIOS does not seem to know anything about that (the keyboard
> shortcuts to set backlight brightness cause it to jump to the level next to
> the one set before closing the lid).
Hi,
with kernel 2.6.37, the display brightness of my ThinkPad X61s was
always reduced after lid open, resume from suspend etc. With this
patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Regards,
Tino
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
> with kernel 2.6.37, the display brightness of my ThinkPad X61s was
> always reduced after lid open, resume from suspend etc. With this
> patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve
the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled)
+ return;
+
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
The other bits either don't change the logic, or should be harmless, or are
plain wrong, like setting the brightness to maximum at bootup.
If the above bit "fixes" it then it's because intel_panel_set_backlight() is called
less often, as that's the buggy function the problem doesn't show up (or is less
clear). Calling intel_panel_set_backlight() with the same value should keep the
brightness the same. Because of the buggy combination code it doesn't always.
Also, try suspending/resuming or "xset dpms force off/on" often in a loop with both
highest and lowest brightness and check if it works correctly with just Alex's patch.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
it for you too? (Make sure you're at max brightness before rebooting.)
That said, the above bit of Alex's patch should be fine to apply, because it avoids
unnecessary register fiddling either way.
Greetings,
Indan
Sorry for this late answer. I only get a very little time for this.
On Fri, Feb 18, 2011 at 05:57, Indan Zupancic <[email protected]> wrote:
> On Thu, February 17, 2011 23:13, Tino Keitel wrote:
>> with kernel 2.6.37, the display brightness of my ThinkPad X61s was
>> always reduced after lid open, resume from suspend etc. With this
>> patch on top of 2.6.38-rc5, the problem is gone. Thanks.
>
> Tino, I think Alex's patch only hides the problem and doesn't properly solve
Could well be. I don't understand what the code is supposed to do.
The patch was created just be looking at diffs.
> the real bug. Can you confirm that this is the bit that fixes it for you?
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index c65992d..c4b1ca4 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (dev_priv->backlight_enabled)
> + return;
> +
> if (dev_priv->backlight_level == 0)
> dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
>
> (Alex's patch edited by hand, offsets might be wrong.)
It is not enough, at least for me.
> The other bits either don't change the logic, or should be harmless, or are
> plain wrong, like setting the brightness to maximum at bootup.
I am not absolutely sure, but I don't think this is what happens on this laptop.
> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
> it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
On Sat, Feb 19, 2011 at 13:11, Alex Riesen <[email protected]> wrote:
>> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
>> it for you too? (Make sure you're at max brightness before rebooting.)
>
> I'll try it now.
>
I can confirm that it does fix backlight in my case (Dell XPS 1330,
LVDS panel, GM965/GL960).
Tested-by: Alex Riesen <[email protected]>
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <[email protected]> wrote:
> On Sat, Feb 19, 2011 at 13:11, Alex Riesen <[email protected]> wrote:
>>> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
>>> it for you too? (Make sure you're at max brightness before rebooting.)
>>
>> I'll try it now.
>>
>
> I can confirm that it does fix backlight in my case (Dell XPS 1330,
> LVDS panel, GM965/GL960).
>
> Tested-by: Alex Riesen <[email protected]>
Guys, should I apply this, or will I get it through somebody's pull?
Linus
Hello,
On Tue, 15 Feb 2011 20:16:01 -0800
Linus Torvalds <[email protected]> wrote:
> Chris Wilson (7):
...
> drm/i915: Fix resume regression from 5d1d0cc
Not sure if what I have is the same or not, but starting with -rc4, when
resuming from suspend, the video stays on the framebuffer output it
switches too when suspending, and the X graphical content is not restored.
Nothing is then updated on the screen, making the machine unusable after
resume (well, I could try to ssh to the box, but it is a laptop, not a
server, so X's pretty mandatory).
[root@tux ~]# lspci
00:00.0 Host bridge: Intel Corporation Mobile 4 Series Chipset Memory Controller Hub (rev 07)
00:02.0 VGA compatible controller: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller (rev 07)
00:02.1 Display controller: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller (rev 07)
...
00:02.0 VGA compatible controller: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller (rev 07) (prog-if 00 [VGA controller])
Subsystem: Dell Device 02bc
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 46
Region 0: Memory at f6800000 (64-bit, non-prefetchable) [size=4M]
Region 2: Memory at d0000000 (64-bit, prefetchable) [size=256M]
Region 4: I/O ports at 1800 [size=8]
Expansion ROM at <unassigned> [disabled]
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Address: fee0100c Data: 4189
Capabilities: [d0] Power Management version 3
Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Kernel driver in use: i915
00:02.1 Display controller: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller (rev 07)
Subsystem: Dell Device 02bc
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Region 0: Memory at f6100000 (64-bit, non-prefetchable) [size=1M]
Capabilities: [d0] Power Management version 3
Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
00:1a.0 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #4 (rev 03) (prog-if 00 [UHCI])
Subsystem: Dell Device 02bc
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 16
Region 4: I/O ports at 1820 [size=32]
Capabilities: [50] PCI Advanced Features
AFCap: TP+ FLR+
AFCtrl: FLR-
AFStatus: TP-
Kernel driver in use: uhci_hcd
If there is any other info I can provide, please feel free to ask.
If you have any patch to test, please feel free to send.
Best,
Paul
On Sat, 19 Feb 2011 15:07:49 -0800
Linus Torvalds <[email protected]> wrote:
> On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <[email protected]> wrote:
> > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <[email protected]> wrote:
> >>> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
> >>> it for you too? (Make sure you're at max brightness before rebooting.)
> >>
> >> I'll try it now.
> >>
> >
> > I can confirm that it does fix backlight in my case (Dell XPS 1330,
> > LVDS panel, GM965/GL960).
> >
> > Tested-by: Alex Riesen <[email protected]>
>
> Guys, should I apply this, or will I get it through somebody's pull?
I'm worried that removing combo mode will break some working setups,
but if it's seen a lot of testing and is ok, then I'm fine with it, as
it definitely simplifies things.
--
Jesse Barnes, Intel Open Source Technology Center
On Tue, Feb 22, 2011 at 13:04:40 -0800, Jesse Barnes wrote:
> On Sat, 19 Feb 2011 15:07:49 -0800
> Linus Torvalds <[email protected]> wrote:
>
> > On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <[email protected]> wrote:
> > > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <[email protected]> wrote:
> > >>> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes
> > >>> it for you too? (Make sure you're at max brightness before rebooting.)
> > >>
> > >> I'll try it now.
> > >>
> > >
> > > I can confirm that it does fix backlight in my case (Dell XPS 1330,
> > > LVDS panel, GM965/GL960).
> > >
> > > Tested-by: Alex Riesen <[email protected]>
> >
> > Guys, should I apply this, or will I get it through somebody's pull?
>
> I'm worried that removing combo mode will break some working setups,
> but if it's seen a lot of testing and is ok, then I'm fine with it, as
> it definitely simplifies things.
Hi,
I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM
related patches, and my backlight issue is gone.
Regards,
Tino
On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel <[email protected]> wrote:
>
> I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM
> related patches, and my backlight issue is gone.
I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had
several testers and seemed to simplify the code nicely too.
Linus
On Tue, February 22, 2011 22:04, Jesse Barnes wrote:
> On Sat, 19 Feb 2011 15:07:49 -0800
> Linus Torvalds <[email protected]> wrote:
>
>> On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <[email protected]> wrote:
>> > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <[email protected]> wrote:
>> >>> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
>> fixes
>> >>> it for you too? (Make sure you're at max brightness before rebooting.)
>> >>
>> >> I'll try it now.
>> >>
>> >
>> > I can confirm that it does fix backlight in my case (Dell XPS 1330,
>> > LVDS panel, GM965/GL960).
>> >
>> > Tested-by: Alex Riesen <[email protected]>
>>
>> Guys, should I apply this, or will I get it through somebody's pull?
>
> I'm worried that removing combo mode will break some working setups,
> but if it's seen a lot of testing and is ok, then I'm fine with it, as
> it definitely simplifies things.
This all seems new code added in 2.6.37, it wasn't there before. The working
setups stopped working when that code was added. The only reason it may work
for some gen 2 and gen 3 hardware is because of a random value of the max
brightness (bit 16). The buggy code seems to be written in a haste without
any testing done. It's so off from the official documentation that I suspect
that the windows driver was used as reference, but its code was misinterpreted.
I grepped the userspace driver source, and LBPC is defined there for 810,
but not used anywhere either.
This patch should be added to stable kernel 2.6.37.2, because it messes
up the LPBC register, which some laptops store between boots.
Quoting https://bugzilla.kernel.org/show_bug.cgi?id=23472#c57
- Checking bit 16 in BLC_PWM_CTL is wrong, it has no special meaning.
- If LBPC == 0xff, it should be ignored and it's not in combination mode.
(This is for gen 3).
- Gen 2 documentation doesn't mention LBPC or combination mode at all.
Gen 3 does, but doesn't tell what the register value is or how to use it,
it just mentions it.
- The calculations are rubbish, resulting in bogus LBPC values, and
depending on how lucky you are, it writes different values for the
registers after a restore.
All this code is new and causes problems, while everything worked before
just fine, when the driver didn't do anything special.
So it seems a bit like voodoo programming, because nothing the driver did
followed the official Intel documentation.
Now, there may be real reasons for some of the code, but I propose we add
them one at a time when people show up with problems without the weird code
added. That way the reason for any weirdness is also documented.
Greetings,
Indan
Hello,
On Wed, February 23, 2011 02:09, Linus Torvalds wrote:
> On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel <[email protected]> wrote:
>>
>> I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM
>> related patches, and my backlight issue is gone.
>
> I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had
> several testers and seemed to simplify the code nicely too.
Sadly, as so often in life, it's not correct. At this point I'm not sure
if it's better to revert that patch and add a correct one, or to just
fix it up. The end result is the same I suppose. I've also found more
documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE
stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions
LBPC (I was looking at 3 before). Apparently the undocumented stuff
the old code did was correct. What I don't understand is how BIOS
makers could know about those bits.
The good side is that that big warning in my patch description is
invalid, something else was going on: The BIOS used LBPC to set and
restore brightness, while the driver only used BLC_PWM_CTL after my
patch.
All credits to Intel for making something simple as backlight control
as stupid and complex as possible:
- It has two registers to control brightness, sometimes one is used,
sometimes the other, sometimes both, and it's unknown what the BIOS
uses, and it's undefined what registers are restored by the BIOS after
reboot/resume.
- When using ACPI and ASLE, the kernel requests a brightness change
via a standard ACPI method, which in turns lets the BIOS generate an
ASLE interrupt, which is handled by the driver. The brightness to set
is between 0 and 255, and the driver is supposed to store the current
brightness in another register. That register stores the brightness in
percentages, which is used by the BIOS to restore brightness. How it
does that is undefined, so it can use either register. So the BIOS
obviously knows how to change the brightness, and it's still seemed
like a good idea to bother the driver with it. The ASLE interface is
a mess.
All in all, after my patch, systems using ASLE and a BIOS storing
the brightness in LBPC stopped working. The reason it works without
ASLE is because the brightness is never changed by the driver, the
backlight is only enabled or disabled.
I'd love to clean up the whole backlight mess, but it's too late in
the RC cycle to do that.
So please revert my patch and apply Takashi Iwai's, which fixes the
most immediate bug without changing anything else. This should go
in stable too.
>From f6b8a45b9544072e6ddbb944a4c03a9ec8cbca3a Mon Sep 17 00:00:00 2001 From: Takashi
Iwai <[email protected]>
Date: Mon, 21 Feb 2011 14:19:27 +0100
Subject: [PATCH] drm/i915: Fix calculation of backlight value in combined mode
The commit a95735569312f2ab0c80425e2cd1e5cb0b4e1870
drm/i915: Refactor panel backlight controls
causes a regression for GM45 that is using the combined mode for
controlling the backlight brightness. The commit introduced a wrong bit shift for
computing the current backlight level.
Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=672946
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
Signed-off-by: Takashi Iwai <[email protected]>
Cc: <[email protected]>
---
drivers/gpu/drm/i915/intel_panel.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -176,7 +176,6 @@
val &= ~1;
pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
val *= lbpc;
- val >>= 1;
}
}
Alex, can you confirm that the revert of 951f3512dba5 plus the
one-liner patch from Takashi that Indan quoted also works for you?
Linus
On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>
> So please revert my patch and apply Takashi Iwai's, which fixes the
> most immediate bug without changing anything else. This should go
> in stable too.
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> Alex, can you confirm that the revert of 951f3512dba5 plus the
> one-liner patch from Takashi that Indan quoted also works for you?
>
> Linus
>
> On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>>
>> So please revert my patch and apply Takashi Iwai's, which fixes the
>> most immediate bug without changing anything else. This should go
>> in stable too.
>
For what it's worth, doing the above does prevent the regression for the
ASLE case, but for me with gen 2 hardware the brightness isn't quite
right after suspend/resume, while it is with my patch applied. So
assuming that there are no gen 2 systems with ASLE out there, the best
solution may be the remove the combination mode check for gen 2 hardware
and leave it for gen >=4. Would be nice if Jesse or Chris could confirm
that there are no gen 2 ASLE systems out there though.
I'm going camping, I'll send a patch next week.
Greetings,
Indan
Indan Zupancic wrote:
> What I don't understand is how BIOS makers could know about those bits.
They have relationships with Intel since 30 years, ie. they get what
they need in one form or other.
//Peter
On Fri, Mar 4, 2011 at 19:47, Linus Torvalds
<[email protected]> wrote:
> Alex, can you confirm that the revert of 951f3512dba5 plus the
> one-liner patch from Takashi that Indan quoted also works for you?
I afraid mine is a different case, because backlight in this system
works properly even with Indan's patch reverted.
Sorry for the late reply...
> On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>>
>> So please revert my patch and apply Takashi Iwai's, which fixes the
>> most immediate bug without changing anything else. This should go
>> in stable too.
>
>
> Linus
>
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> Alex, can you confirm that the revert of 951f3512dba5 plus the
> one-liner patch from Takashi that Indan quoted also works for you?
>
> Linus
>
> On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>>
>> So please revert my patch and apply Takashi Iwai's, which fixes the
>> most immediate bug without changing anything else. This should go
>> in stable too.
>
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called,
but intel_panel_enable_backlight() is called at resume. With the
effect that if the brightness was ever changed after screen
blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with
combination mode, because then the brightness is always the same
(zero or the maximum, the BIOS only uses LBPC on my system.) I'll
send a patch in a moment.
Alternative for reverting the combination mode removal (I can also
redo the patch against the revert and Takashi's patch, if that's
preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code
and the weirdness removed: No val >>= 1, but also no val &= ~1. The
old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
The other change is clearer calculations: Just check for zero level
explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic <[email protected]>
---
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..b05631a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
+#define BLM_COMBINATION_MODE (1 << 30)
+#define BLM_LEGACY_MODE (1 << 16)
+
void
intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode)
@@ -110,6 +114,22 @@ done:
dev_priv->pch_pf_size = (width << 16) | height;
}
+/*
+ * What about gen 3? If there are no gen 3 systems with ASLE,
+ * then it doesn't matter, as we don't need to change the
+ * brightness. But then the gen 2 check can be removed too.
+ */
+static int is_backlight_combination_mode(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (INTEL_INFO(dev)->gen >= 4)
+ return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
+ if (IS_GEN2(dev))
+ return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
+ return 0;
+}
+
static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
{
u32 val;
@@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
max >>= 17;
} else {
max >>= 16;
+ /* Ignore BLM_LEGACY_MODE bit */
if (INTEL_INFO(dev)->gen < 4)
max &= ~1;
}
+ if (is_backlight_combination_mode(dev))
+ max *= 0xff;
}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val >>= 1;
+ if (is_backlight_combination_mode(dev)){
+ u8 lbpc;
+
+ pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
+ val *= lbpc;
+ }
}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
+
+ if (level && is_backlight_combination_mode(dev)){
+ u32 max = intel_panel_get_max_backlight(dev);
+ u8 lpbc;
+
+ lpbc = level * 0xff / max;
+ level /= lpbc;
+ pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
+ }
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
drm/i915: Fix DPMS and suspend interaction for intel_panel.c
When suspending intel_panel_disable_backlight() is never called,
but intel_panel_enable_backlight() is called at resume. With the
effect that if the brightness was ever changed after screen
blanking, the wrong brightness gets restored at resume time.
Nothing guarantees that those calls will be balanced, so having
backlight_enabled makes no sense, as the real state can change
without the panel code noticing. So keep things as stateless as
possible.
Signed-off-by: Indan Zupancic <[email protected]>
---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 456f404..4a3d9ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -333,7 +333,6 @@ typedef struct drm_i915_private {
/* LVDS info */
int backlight_level; /* restore backlight to this value */
- bool backlight_enabled;
struct drm_display_mode *panel_fixed_mode;
struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1220,10 +1219,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor);
extern int i915_save_state(struct drm_device *dev);
extern int i915_restore_state(struct drm_device *dev);
-/* i915_suspend.c */
-extern int i915_save_state(struct drm_device *dev);
-extern int i915_restore_state(struct drm_device *dev);
-
/* intel_i2c.c */
extern int intel_setup_gmbus(struct drm_device *dev);
extern void intel_teardown_gmbus(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..1b5a32d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5924,8 +5924,6 @@ static void intel_setup_outputs(struct drm_device *dev)
encoder->base.possible_clones =
intel_encoder_clones(dev, encoder->clone_mask);
}
-
- intel_panel_setup_backlight(dev);
}
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2c43104..70e8b82 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -257,7 +257,6 @@ extern void intel_pch_panel_fitting(struct drm_device *dev,
extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
extern u32 intel_panel_get_backlight(struct drm_device *dev);
extern void intel_panel_set_backlight(struct drm_device *dev, u32 level);
-extern void intel_panel_setup_backlight(struct drm_device *dev);
extern void intel_panel_enable_backlight(struct drm_device *dev);
extern void intel_panel_disable_backlight(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..b05631a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -217,12 +255,11 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
void intel_panel_disable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ u32 level = intel_panel_get_backlight(dev);
- if (dev_priv->backlight_enabled) {
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
- dev_priv->backlight_enabled = false;
- }
-
+ if (level == 0)
+ return;
+ dev_priv->backlight_level = level;
intel_panel_set_backlight(dev, 0);
}
@@ -230,17 +267,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (intel_panel_get_backlight(dev))
+ return;
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
-
intel_panel_set_backlight(dev, dev_priv->backlight_level);
- dev_priv->backlight_enabled = true;
-}
-
-void intel_panel_setup_backlight(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
- dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
}
At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
Indan Zupancic wrote:
>
> Hello,
>
> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> > Alex, can you confirm that the revert of 951f3512dba5 plus the
> > one-liner patch from Takashi that Indan quoted also works for you?
> >
> > Linus
> >
> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
> >>
> >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> most immediate bug without changing anything else. This should go
> >> in stable too.
> >
>
> I found another backlight bug:
>
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored.
>
> This explains the weird behaviour I've seen. I didn't see it with
> combination mode, because then the brightness is always the same
> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> send a patch in a moment.
>
> Alternative for reverting the combination mode removal (I can also
> redo the patch against the revert and Takashi's patch, if that's
> preferred):
>
> --
>
> drm/i915: Do handle backlight combination mode specially
>
> Add back the combination mode check, but with slightly cleaner code
> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> The other change is clearer calculations: Just check for zero level
> explicitly instead of avoiding the divide-by-zero.
>
> Signed-off-by: Indan Zupancic <[email protected]>
>
> ---
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index d860abe..b05631a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -30,6 +30,10 @@
>
> #include "intel_drv.h"
>
> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> +#define BLM_COMBINATION_MODE (1 << 30)
> +#define BLM_LEGACY_MODE (1 << 16)
> +
> void
> intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> struct drm_display_mode *adjusted_mode)
> @@ -110,6 +114,22 @@ done:
> dev_priv->pch_pf_size = (width << 16) | height;
> }
>
> +/*
> + * What about gen 3? If there are no gen 3 systems with ASLE,
> + * then it doesn't matter, as we don't need to change the
> + * brightness. But then the gen 2 check can be removed too.
> + */
> +static int is_backlight_combination_mode(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (INTEL_INFO(dev)->gen >= 4)
> + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> + if (IS_GEN2(dev))
> + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> + return 0;
> +}
> +
> static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> {
> u32 val;
> @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> max >>= 17;
> } else {
> max >>= 16;
> + /* Ignore BLM_LEGACY_MODE bit */
> if (INTEL_INFO(dev)->gen < 4)
> max &= ~1;
> }
> + if (is_backlight_combination_mode(dev))
> + max *= 0xff;
> }
>
> DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> if (IS_PINEVIEW(dev))
> val >>= 1;
> + if (is_backlight_combination_mode(dev)){
> + u8 lbpc;
> +
> + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> + val *= lbpc;
> + }
> }
>
> DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
>
> if (HAS_PCH_SPLIT(dev))
> return intel_pch_panel_set_backlight(dev, level);
> +
> + if (level && is_backlight_combination_mode(dev)){
> + u32 max = intel_panel_get_max_backlight(dev);
> + u8 lpbc;
> +
> + lpbc = level * 0xff / max;
> + level /= lpbc;
Hmm, I don't think this calculation is correct. This would result
in level of opregion over its limit. For example, assume the level
max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1
level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case
above...
thanks,
Takashi
At Thu, 10 Mar 2011 08:49:37 +0100,
Takashi Iwai wrote:
>
> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> Indan Zupancic wrote:
> >
> > Hello,
> >
> > On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> > > Alex, can you confirm that the revert of 951f3512dba5 plus the
> > > one-liner patch from Takashi that Indan quoted also works for you?
> > >
> > > Linus
> > >
> > > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
> > >>
> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
> > >> most immediate bug without changing anything else. This should go
> > >> in stable too.
> > >
> >
> > I found another backlight bug:
> >
> > When suspending intel_panel_disable_backlight() is never called,
> > but intel_panel_enable_backlight() is called at resume. With the
> > effect that if the brightness was ever changed after screen
> > blanking, the wrong brightness gets restored.
> >
> > This explains the weird behaviour I've seen. I didn't see it with
> > combination mode, because then the brightness is always the same
> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> > send a patch in a moment.
> >
> > Alternative for reverting the combination mode removal (I can also
> > redo the patch against the revert and Takashi's patch, if that's
> > preferred):
> >
> > --
> >
> > drm/i915: Do handle backlight combination mode specially
> >
> > Add back the combination mode check, but with slightly cleaner code
> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> > The other change is clearer calculations: Just check for zero level
> > explicitly instead of avoiding the divide-by-zero.
> >
> > Signed-off-by: Indan Zupancic <[email protected]>
> >
> > ---
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index d860abe..b05631a 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -30,6 +30,10 @@
> >
> > #include "intel_drv.h"
> >
> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> > +#define BLM_COMBINATION_MODE (1 << 30)
> > +#define BLM_LEGACY_MODE (1 << 16)
> > +
> > void
> > intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> > struct drm_display_mode *adjusted_mode)
> > @@ -110,6 +114,22 @@ done:
> > dev_priv->pch_pf_size = (width << 16) | height;
> > }
> >
> > +/*
> > + * What about gen 3? If there are no gen 3 systems with ASLE,
> > + * then it doesn't matter, as we don't need to change the
> > + * brightness. But then the gen 2 check can be removed too.
> > + */
> > +static int is_backlight_combination_mode(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > + if (INTEL_INFO(dev)->gen >= 4)
> > + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> > + if (IS_GEN2(dev))
> > + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> > + return 0;
> > +}
> > +
> > static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> > {
> > u32 val;
> > @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> > max >>= 17;
> > } else {
> > max >>= 16;
> > + /* Ignore BLM_LEGACY_MODE bit */
> > if (INTEL_INFO(dev)->gen < 4)
> > max &= ~1;
> > }
> > + if (is_backlight_combination_mode(dev))
> > + max *= 0xff;
> > }
> >
> > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> > @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> > if (IS_PINEVIEW(dev))
> > val >>= 1;
> > + if (is_backlight_combination_mode(dev)){
> > + u8 lbpc;
> > +
> > + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> > + val *= lbpc;
> > + }
> > }
> >
> > DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> > @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
> >
> > if (HAS_PCH_SPLIT(dev))
> > return intel_pch_panel_set_backlight(dev, level);
> > +
> > + if (level && is_backlight_combination_mode(dev)){
> > + u32 max = intel_panel_get_max_backlight(dev);
> > + u8 lpbc;
> > +
> > + lpbc = level * 0xff / max;
> > + level /= lpbc;
>
> Hmm, I don't think this calculation is correct. This would result
> in level of opregion over its limit. For example, assume the level
> max = 100, so total max = 25500. Passing level=150 here will be:
>
> lbpc = 150 * 0xff / 25500 = 1.5 = 1
> level = 150 / 1 = 150, which is over limit.
>
> More worse, lbpc can be zero when level is below 100 in the case
> above...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);
u8 lpbc;
lpbc = level * 0xfe / max + 1;
level /= lpbc;
pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
}
This will fit within the right range.
Though, changing like below will give a bit better calculation,
closer to the real level.
lpbc = level * 0xfe / max + 1;
level = (level + lpbc / 2) / lpbc;
Takashi
On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> Indan Zupancic wrote:
>>
>> Hello,
>>
>> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
>> > Alex, can you confirm that the revert of 951f3512dba5 plus the
>> > one-liner patch from Takashi that Indan quoted also works for you?
>> >
>> > Linus
>> >
>> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>> >>
>> >> So please revert my patch and apply Takashi Iwai's, which fixes the
>> >> most immediate bug without changing anything else. This should go
>> >> in stable too.
>> >
>>
>> I found another backlight bug:
>>
>> When suspending intel_panel_disable_backlight() is never called,
>> but intel_panel_enable_backlight() is called at resume. With the
>> effect that if the brightness was ever changed after screen
>> blanking, the wrong brightness gets restored.
>>
>> This explains the weird behaviour I've seen. I didn't see it with
>> combination mode, because then the brightness is always the same
>> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
>> send a patch in a moment.
>>
>> Alternative for reverting the combination mode removal (I can also
>> redo the patch against the revert and Takashi's patch, if that's
>> preferred):
>>
>> --
>>
>> drm/i915: Do handle backlight combination mode specially
>>
>> Add back the combination mode check, but with slightly cleaner code
>> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
>> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
>> The other change is clearer calculations: Just check for zero level
>> explicitly instead of avoiding the divide-by-zero.
>>
>> Signed-off-by: Indan Zupancic <[email protected]>
>>
>> ---
>>
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index d860abe..b05631a 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -30,6 +30,10 @@
>>
>> #include "intel_drv.h"
>>
>> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
>> +#define BLM_COMBINATION_MODE (1 << 30)
>> +#define BLM_LEGACY_MODE (1 << 16)
>> +
>> void
>> intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>> struct drm_display_mode *adjusted_mode)
>> @@ -110,6 +114,22 @@ done:
>> dev_priv->pch_pf_size = (width << 16) | height;
>> }
>>
>> +/*
>> + * What about gen 3? If there are no gen 3 systems with ASLE,
>> + * then it doesn't matter, as we don't need to change the
>> + * brightness. But then the gen 2 check can be removed too.
>> + */
>> +static int is_backlight_combination_mode(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + if (INTEL_INFO(dev)->gen >= 4)
>> + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
>> + if (IS_GEN2(dev))
>> + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
>> + return 0;
>> +}
>> +
>> static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
>> {
>> u32 val;
>> @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
>> max >>= 17;
>> } else {
>> max >>= 16;
>> + /* Ignore BLM_LEGACY_MODE bit */
>> if (INTEL_INFO(dev)->gen < 4)
>> max &= ~1;
>> }
>> + if (is_backlight_combination_mode(dev))
>> + max *= 0xff;
>> }
>>
>> DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
>> @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
>> val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>> if (IS_PINEVIEW(dev))
>> val >>= 1;
>> + if (is_backlight_combination_mode(dev)){
>> + u8 lbpc;
>> +
>> + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
>> + val *= lbpc;
>> + }
>> }
>>
>> DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
>> @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
>>
>> if (HAS_PCH_SPLIT(dev))
>> return intel_pch_panel_set_backlight(dev, level);
>> +
>> + if (level && is_backlight_combination_mode(dev)){
>> + u32 max = intel_panel_get_max_backlight(dev);
>> + u8 lpbc;
>> +
>> + lpbc = level * 0xff / max;
>> + level /= lpbc;
>
> Hmm, I don't think this calculation is correct. This would result
> in level of opregion over its limit. For example, assume the level
> max = 100, so total max = 25500. Passing level=150 here will be:
>
> lbpc = 150 * 0xff / 25500 = 1.5 = 1
> level = 150 / 1 = 150, which is over limit.
>
> More worse, lbpc can be zero when level is below 100 in the case
> above...
Yes, you're right. It seems that any simplification I try to do
creates a new bug.
Do you have any bright idea why the old code did val &= ~1; too?
It seems obvious it's related to val >>= 1, but...
Thanks,
Indan
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
> At Thu, 10 Mar 2011 08:49:37 +0100,
> Takashi Iwai wrote:
>>
>> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
>> Indan Zupancic wrote:
>> >
>> > Hello,
>> >
>> > On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
>> > > Alex, can you confirm that the revert of 951f3512dba5 plus the
>> > > one-liner patch from Takashi that Indan quoted also works for you?
>> > >
>> > > Linus
>> > >
>> > > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
>> > >>
>> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
>> > >> most immediate bug without changing anything else. This should go
>> > >> in stable too.
>> > >
>> >
>> > I found another backlight bug:
>> >
>> > When suspending intel_panel_disable_backlight() is never called,
>> > but intel_panel_enable_backlight() is called at resume. With the
>> > effect that if the brightness was ever changed after screen
>> > blanking, the wrong brightness gets restored.
>> >
>> > This explains the weird behaviour I've seen. I didn't see it with
>> > combination mode, because then the brightness is always the same
>> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
>> > send a patch in a moment.
>> >
>> > Alternative for reverting the combination mode removal (I can also
>> > redo the patch against the revert and Takashi's patch, if that's
>> > preferred):
>> >
>> > --
>> >
>> > drm/i915: Do handle backlight combination mode specially
>> >
>> > Add back the combination mode check, but with slightly cleaner code
>> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
>> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
>> > The other change is clearer calculations: Just check for zero level
>> > explicitly instead of avoiding the divide-by-zero.
>> >
>> > Signed-off-by: Indan Zupancic <[email protected]>
>> >
>> > ---
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> > index d860abe..b05631a 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -30,6 +30,10 @@
>> >
>> > #include "intel_drv.h"
>> >
>> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
>> > +#define BLM_COMBINATION_MODE (1 << 30)
>> > +#define BLM_LEGACY_MODE (1 << 16)
>> > +
>> > void
>> > intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>> > struct drm_display_mode *adjusted_mode)
>> > @@ -110,6 +114,22 @@ done:
>> > dev_priv->pch_pf_size = (width << 16) | height;
>> > }
>> >
>> > +/*
>> > + * What about gen 3? If there are no gen 3 systems with ASLE,
>> > + * then it doesn't matter, as we don't need to change the
>> > + * brightness. But then the gen 2 check can be removed too.
>> > + */
>> > +static int is_backlight_combination_mode(struct drm_device *dev)
>> > +{
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > + if (INTEL_INFO(dev)->gen >= 4)
>> > + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
>> > + if (IS_GEN2(dev))
>> > + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
>> > + return 0;
>> > +}
>> > +
>> > static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
>> > {
>> > u32 val;
>> > @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
>> > max >>= 17;
>> > } else {
>> > max >>= 16;
>> > + /* Ignore BLM_LEGACY_MODE bit */
>> > if (INTEL_INFO(dev)->gen < 4)
>> > max &= ~1;
>> > }
>> > + if (is_backlight_combination_mode(dev))
>> > + max *= 0xff;
>> > }
>> >
>> > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
>> > @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
>> > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>> > if (IS_PINEVIEW(dev))
>> > val >>= 1;
>> > + if (is_backlight_combination_mode(dev)){
>> > + u8 lbpc;
>> > +
>> > + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
>> > + val *= lbpc;
>> > + }
>> > }
>> >
>> > DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
>> > @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
>> >
>> > if (HAS_PCH_SPLIT(dev))
>> > return intel_pch_panel_set_backlight(dev, level);
>> > +
>> > + if (level && is_backlight_combination_mode(dev)){
>> > + u32 max = intel_panel_get_max_backlight(dev);
>> > + u8 lpbc;
>> > +
>> > + lpbc = level * 0xff / max;
>> > + level /= lpbc;
>>
>> Hmm, I don't think this calculation is correct. This would result
>> in level of opregion over its limit. For example, assume the level
>> max = 100, so total max = 25500. Passing level=150 here will be:
>>
>> lbpc = 150 * 0xff / 25500 = 1.5 = 1
>> level = 150 / 1 = 150, which is over limit.
>>
>> More worse, lbpc can be zero when level is below 100 in the case
>> above...
>
> That is, Chris' original code in that portion was correct:
>
> if (is_backlight_combination_mode(dev)){
> u32 max = intel_panel_get_max_backlight(dev);
> u8 lpbc;
>
> lpbc = level * 0xfe / max + 1;
> level /= lpbc;
> pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
> }
>
> This will fit within the right range.
> Though, changing like below will give a bit better calculation,
> closer to the real level.
>
> lpbc = level * 0xfe / max + 1;
> level = (level + lpbc / 2) / lpbc;
Indeed, though I don't think it makes much difference in practise.
All in all it seems best to just revert my patch and apply your fix.
Any "improvements" I may have are either buggy or can be added later.
Care to make a new patch with the above improvement added? You can
add my acked-by, for what it's worth.
At this point I don't even dare removing that "obviously" bogus
val &= ~1; I bet it's an undocumented bit having some obscure
secret function on not well tested systems.
Greetings,
Indan
At Thu, 10 Mar 2011 09:45:18 +0100 (CET),
Indan Zupancic wrote:
>
> On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> > Indan Zupancic wrote:
> >>
> >> Hello,
> >>
> >> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> >> > Alex, can you confirm that the revert of 951f3512dba5 plus the
> >> > one-liner patch from Takashi that Indan quoted also works for you?
> >> >
> >> > Linus
> >> >
> >> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
> >> >>
> >> >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> >> most immediate bug without changing anything else. This should go
> >> >> in stable too.
> >> >
> >>
> >> I found another backlight bug:
> >>
> >> When suspending intel_panel_disable_backlight() is never called,
> >> but intel_panel_enable_backlight() is called at resume. With the
> >> effect that if the brightness was ever changed after screen
> >> blanking, the wrong brightness gets restored.
> >>
> >> This explains the weird behaviour I've seen. I didn't see it with
> >> combination mode, because then the brightness is always the same
> >> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> >> send a patch in a moment.
> >>
> >> Alternative for reverting the combination mode removal (I can also
> >> redo the patch against the revert and Takashi's patch, if that's
> >> preferred):
> >>
> >> --
> >>
> >> drm/i915: Do handle backlight combination mode specially
> >>
> >> Add back the combination mode check, but with slightly cleaner code
> >> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> >> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> >> The other change is clearer calculations: Just check for zero level
> >> explicitly instead of avoiding the divide-by-zero.
> >>
> >> Signed-off-by: Indan Zupancic <[email protected]>
> >>
> >> ---
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index d860abe..b05631a 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -30,6 +30,10 @@
> >>
> >> #include "intel_drv.h"
> >>
> >> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> >> +#define BLM_COMBINATION_MODE (1 << 30)
> >> +#define BLM_LEGACY_MODE (1 << 16)
> >> +
> >> void
> >> intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >> struct drm_display_mode *adjusted_mode)
> >> @@ -110,6 +114,22 @@ done:
> >> dev_priv->pch_pf_size = (width << 16) | height;
> >> }
> >>
> >> +/*
> >> + * What about gen 3? If there are no gen 3 systems with ASLE,
> >> + * then it doesn't matter, as we don't need to change the
> >> + * brightness. But then the gen 2 check can be removed too.
> >> + */
> >> +static int is_backlight_combination_mode(struct drm_device *dev)
> >> +{
> >> + struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> + if (INTEL_INFO(dev)->gen >= 4)
> >> + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> >> + if (IS_GEN2(dev))
> >> + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> >> + return 0;
> >> +}
> >> +
> >> static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >> {
> >> u32 val;
> >> @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> >> max >>= 17;
> >> } else {
> >> max >>= 16;
> >> + /* Ignore BLM_LEGACY_MODE bit */
> >> if (INTEL_INFO(dev)->gen < 4)
> >> max &= ~1;
> >> }
> >> + if (is_backlight_combination_mode(dev))
> >> + max *= 0xff;
> >> }
> >>
> >> DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> >> @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> >> val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> >> if (IS_PINEVIEW(dev))
> >> val >>= 1;
> >> + if (is_backlight_combination_mode(dev)){
> >> + u8 lbpc;
> >> +
> >> + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> >> + val *= lbpc;
> >> + }
> >> }
> >>
> >> DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> >> @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
> >>
> >> if (HAS_PCH_SPLIT(dev))
> >> return intel_pch_panel_set_backlight(dev, level);
> >> +
> >> + if (level && is_backlight_combination_mode(dev)){
> >> + u32 max = intel_panel_get_max_backlight(dev);
> >> + u8 lpbc;
> >> +
> >> + lpbc = level * 0xff / max;
> >> + level /= lpbc;
> >
> > Hmm, I don't think this calculation is correct. This would result
> > in level of opregion over its limit. For example, assume the level
> > max = 100, so total max = 25500. Passing level=150 here will be:
> >
> > lbpc = 150 * 0xff / 25500 = 1.5 = 1
> > level = 150 / 1 = 150, which is over limit.
> >
> > More worse, lbpc can be zero when level is below 100 in the case
> > above...
>
> Yes, you're right. It seems that any simplification I try to do
> creates a new bug.
>
> Do you have any bright idea why the old code did val &= ~1; too?
> It seems obvious it's related to val >>= 1, but...
I guess it's for the case GEN < 4. But, no certain idea.
In my patch, I left it since this is relatively harmless, even if it's
not correct.
Takashi
At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
>
> On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 08:49:37 +0100,
> > Takashi Iwai wrote:
> >>
> >> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> >> Indan Zupancic wrote:
> >> >
> >> > Hello,
> >> >
> >> > On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
> >> > > Alex, can you confirm that the revert of 951f3512dba5 plus the
> >> > > one-liner patch from Takashi that Indan quoted also works for you?
> >> > >
> >> > > Linus
> >> > >
> >> > > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <[email protected]> wrote:
> >> > >>
> >> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
> >> > >> most immediate bug without changing anything else. This should go
> >> > >> in stable too.
> >> > >
> >> >
> >> > I found another backlight bug:
> >> >
> >> > When suspending intel_panel_disable_backlight() is never called,
> >> > but intel_panel_enable_backlight() is called at resume. With the
> >> > effect that if the brightness was ever changed after screen
> >> > blanking, the wrong brightness gets restored.
> >> >
> >> > This explains the weird behaviour I've seen. I didn't see it with
> >> > combination mode, because then the brightness is always the same
> >> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
> >> > send a patch in a moment.
> >> >
> >> > Alternative for reverting the combination mode removal (I can also
> >> > redo the patch against the revert and Takashi's patch, if that's
> >> > preferred):
> >> >
> >> > --
> >> >
> >> > drm/i915: Do handle backlight combination mode specially
> >> >
> >> > Add back the combination mode check, but with slightly cleaner code
> >> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
> >> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
> >> > The other change is clearer calculations: Just check for zero level
> >> > explicitly instead of avoiding the divide-by-zero.
> >> >
> >> > Signed-off-by: Indan Zupancic <[email protected]>
> >> >
> >> > ---
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> > index d860abe..b05631a 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -30,6 +30,10 @@
> >> >
> >> > #include "intel_drv.h"
> >> >
> >> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> >> > +#define BLM_COMBINATION_MODE (1 << 30)
> >> > +#define BLM_LEGACY_MODE (1 << 16)
> >> > +
> >> > void
> >> > intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >> > struct drm_display_mode *adjusted_mode)
> >> > @@ -110,6 +114,22 @@ done:
> >> > dev_priv->pch_pf_size = (width << 16) | height;
> >> > }
> >> >
> >> > +/*
> >> > + * What about gen 3? If there are no gen 3 systems with ASLE,
> >> > + * then it doesn't matter, as we don't need to change the
> >> > + * brightness. But then the gen 2 check can be removed too.
> >> > + */
> >> > +static int is_backlight_combination_mode(struct drm_device *dev)
> >> > +{
> >> > + struct drm_i915_private *dev_priv = dev->dev_private;
> >> > +
> >> > + if (INTEL_INFO(dev)->gen >= 4)
> >> > + return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> >> > + if (IS_GEN2(dev))
> >> > + return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> >> > + return 0;
> >> > +}
> >> > +
> >> > static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >> > {
> >> > u32 val;
> >> > @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> >> > max >>= 17;
> >> > } else {
> >> > max >>= 16;
> >> > + /* Ignore BLM_LEGACY_MODE bit */
> >> > if (INTEL_INFO(dev)->gen < 4)
> >> > max &= ~1;
> >> > }
> >> > + if (is_backlight_combination_mode(dev))
> >> > + max *= 0xff;
> >> > }
> >> >
> >> > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> >> > @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
> >> > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> >> > if (IS_PINEVIEW(dev))
> >> > val >>= 1;
> >> > + if (is_backlight_combination_mode(dev)){
> >> > + u8 lbpc;
> >> > +
> >> > + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> >> > + val *= lbpc;
> >> > + }
> >> > }
> >> >
> >> > DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> >> > @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
> >> >
> >> > if (HAS_PCH_SPLIT(dev))
> >> > return intel_pch_panel_set_backlight(dev, level);
> >> > +
> >> > + if (level && is_backlight_combination_mode(dev)){
> >> > + u32 max = intel_panel_get_max_backlight(dev);
> >> > + u8 lpbc;
> >> > +
> >> > + lpbc = level * 0xff / max;
> >> > + level /= lpbc;
> >>
> >> Hmm, I don't think this calculation is correct. This would result
> >> in level of opregion over its limit. For example, assume the level
> >> max = 100, so total max = 25500. Passing level=150 here will be:
> >>
> >> lbpc = 150 * 0xff / 25500 = 1.5 = 1
> >> level = 150 / 1 = 150, which is over limit.
> >>
> >> More worse, lbpc can be zero when level is below 100 in the case
> >> above...
> >
> > That is, Chris' original code in that portion was correct:
> >
> > if (is_backlight_combination_mode(dev)){
> > u32 max = intel_panel_get_max_backlight(dev);
> > u8 lpbc;
> >
> > lpbc = level * 0xfe / max + 1;
> > level /= lpbc;
> > pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
> > }
> >
> > This will fit within the right range.
> > Though, changing like below will give a bit better calculation,
> > closer to the real level.
> >
> > lpbc = level * 0xfe / max + 1;
> > level = (level + lpbc / 2) / lpbc;
>
> Indeed, though I don't think it makes much difference in practise.
>
> All in all it seems best to just revert my patch and apply your fix.
> Any "improvements" I may have are either buggy or can be added later.
Agreed. We should take a safer way.
> Care to make a new patch with the above improvement added? You can
> add my acked-by, for what it's worth.
OK, I'm going to send it now.
> At this point I don't even dare removing that "obviously" bogus
> val &= ~1; I bet it's an undocumented bit having some obscure
> secret function on not well tested systems.
Yes, I also left it...
Takashi