2011-02-16 04:16:46

by Linus Torvalds

[permalink] [raw]
Subject: Linux 2.6.38-rc5

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


2011-02-16 11:14:11

by Eric Dumazet

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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


2011-02-16 13:55:53

by Eric Dumazet

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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)

2011-02-16 15:47:15

by Linus Torvalds

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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


Attachments:
patch.diff (1.52 kB)

2011-02-16 16:06:48

by Al Viro

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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...

2011-02-16 16:19:35

by Al Viro

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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)

2011-02-16 16:23:12

by Eric Dumazet

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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

2011-02-16 16:33:57

by Linus Torvalds

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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

2011-02-16 16:39:57

by Al Viro

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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()?

2011-02-16 16:47:52

by Eric Dumazet

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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]>


2011-02-16 19:27:17

by Alex Riesen

[permalink] [raw]
Subject: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-16 19:46:48

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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.

2011-02-16 19:54:12

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-16 19:59:39

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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.

2011-02-16 20:05:27

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-16 20:28:54

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-17 01:42:01

by Indan Zupancic

[permalink] [raw]
Subject: [PATCH] drm/i915: Do not handle backlight combination mode specially.

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);

2011-02-17 22:13:38

by Tino Keitel

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-18 04:57:10

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-19 12:11:05

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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.

2011-02-19 12:26:47

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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]>

2011-02-19 23:08:40

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-20 14:04:07

by Paul Rolland

[permalink] [raw]
Subject: Re: Linux 2.6.38-rc5

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


Attachments:
signature.asc (198.00 B)

2011-02-22 21:04:47

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-22 22:31:35

by Tino Keitel

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-23 01:09:50

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-02-23 01:33:07

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-04 06:53:33

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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;
}
}

2011-03-04 18:47:30

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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.

2011-03-04 23:32:48

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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


2011-03-05 00:26:20

by Peter Stuge

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-06 17:40:18

by Alex Riesen

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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
>

2011-03-10 05:50:22

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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);

2011-03-10 06:01:04

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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;
}

2011-03-10 07:49:41

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-10 08:25:25

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-10 08:45:31

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-10 10:06:35

by Indan Zupancic

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-10 12:51:57

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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

2011-03-10 12:59:41

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

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