LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Linux 2.6.38-rc5
@ 2011-02-16  4:16 Linus Torvalds
  2011-02-16 11:14 ` Eric Dumazet
                   ` (2 more replies)
  0 siblings, 3 replies; 48+ messages in thread
From: Linus Torvalds @ 2011-02-16  4:16 UTC (permalink / raw)
  To: Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16  4:16 Linux 2.6.38-rc5 Linus Torvalds
@ 2011-02-16 11:14 ` Eric Dumazet
  2011-02-16 13:55   ` Eric Dumazet
  2011-02-16 15:46   ` Linus Torvalds
  2011-02-16 19:26 ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Alex Riesen
  2011-02-20 14:03 ` Linux 2.6.38-rc5 Paul Rolland
  2 siblings, 2 replies; 48+ messages in thread
From: Eric Dumazet @ 2011-02-16 11:14 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List

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




^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 11:14 ` Eric Dumazet
@ 2011-02-16 13:55   ` Eric Dumazet
  2011-02-16 15:46   ` Linus Torvalds
  1 sibling, 0 replies; 48+ messages in thread
From: Eric Dumazet @ 2011-02-16 13:55 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Al Viro

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)



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 11:14 ` Eric Dumazet
  2011-02-16 13:55   ` Eric Dumazet
@ 2011-02-16 15:46   ` Linus Torvalds
  2011-02-16 16:06     ` Al Viro
  2011-02-16 16:22     ` Eric Dumazet
  1 sibling, 2 replies; 48+ messages in thread
From: Linus Torvalds @ 2011-02-16 15:46 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Linux Kernel Mailing List, Al Viro

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <eric.dumazet@gmail.com> 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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1554 bytes --]

 fs/namei.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 9e701e2..0087cf9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -795,7 +795,7 @@ __do_follow_link(const struct path *link, struct nameidata *nd, void **p)
  * Without that kind of total limit, nasty chains of consecutive
  * symlinks can cause almost arbitrarily long lookups. 
  */
-static inline int do_follow_link(struct path *path, struct nameidata *nd)
+static inline int do_follow_link(struct inode *inode, struct path *path, struct nameidata *nd)
 {
 	void *cookie;
 	int err = -ELOOP;
@@ -803,6 +803,7 @@ static inline int do_follow_link(struct path *path, struct nameidata *nd)
 	/* We drop rcu-walk here */
 	if (nameidata_dentry_drop_rcu_maybe(nd, path->dentry))
 		return -ECHILD;
+	BUG_ON(inode != path->dentry->d_inode);
 
 	if (current->link_count >= MAX_NESTED_LINKS)
 		goto loop;
@@ -1413,8 +1414,7 @@ exec_again:
 			goto out_dput;
 
 		if (inode->i_op->follow_link) {
-			BUG_ON(inode != next.dentry->d_inode);
-			err = do_follow_link(&next, nd);
+			err = do_follow_link(inode, &next, nd);
 			if (err)
 				goto return_err;
 			nd->inode = nd->path.dentry->d_inode;
@@ -1458,8 +1458,7 @@ last_component:
 			break;
 		if (inode && unlikely(inode->i_op->follow_link) &&
 		    (lookup_flags & LOOKUP_FOLLOW)) {
-			BUG_ON(inode != next.dentry->d_inode);
-			err = do_follow_link(&next, nd);
+			err = do_follow_link(inode, &next, nd);
 			if (err)
 				goto return_err;
 			nd->inode = nd->path.dentry->d_inode;

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 15:46   ` Linus Torvalds
@ 2011-02-16 16:06     ` Al Viro
  2011-02-16 16:19       ` Al Viro
  2011-02-16 16:22     ` Eric Dumazet
  1 sibling, 1 reply; 48+ messages in thread
From: Al Viro @ 2011-02-16 16:06 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Eric Dumazet, Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 07:46:20AM -0800, Linus Torvalds wrote:
> On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <eric.dumazet@gmail.com> 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...

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 16:06     ` Al Viro
@ 2011-02-16 16:19       ` Al Viro
  2011-02-16 16:33         ` Linus Torvalds
  0 siblings, 1 reply; 48+ messages in thread
From: Al Viro @ 2011-02-16 16:19 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Eric Dumazet, Linux Kernel Mailing List

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)

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 15:46   ` Linus Torvalds
  2011-02-16 16:06     ` Al Viro
@ 2011-02-16 16:22     ` Eric Dumazet
  1 sibling, 0 replies; 48+ messages in thread
From: Eric Dumazet @ 2011-02-16 16:22 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Al Viro

Le mercredi 16 février 2011 à 07:46 -0800, Linus Torvalds a écrit :
> On Wed, Feb 16, 2011 at 3:14 AM, Eric Dumazet <eric.dumazet@gmail.com> 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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 16:19       ` Al Viro
@ 2011-02-16 16:33         ` Linus Torvalds
  2011-02-16 16:39           ` Al Viro
  0 siblings, 1 reply; 48+ messages in thread
From: Linus Torvalds @ 2011-02-16 16:33 UTC (permalink / raw)
  To: Al Viro; +Cc: Eric Dumazet, Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 8:19 AM, Al Viro <viro@zeniv.linux.org.uk> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 16:33         ` Linus Torvalds
@ 2011-02-16 16:39           ` Al Viro
  2011-02-16 16:47             ` Eric Dumazet
  0 siblings, 1 reply; 48+ messages in thread
From: Al Viro @ 2011-02-16 16:39 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Eric Dumazet, Linux Kernel Mailing List, npiggin

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16 16:39           ` Al Viro
@ 2011-02-16 16:47             ` Eric Dumazet
  0 siblings, 0 replies; 48+ messages in thread
From: Eric Dumazet @ 2011-02-16 16:47 UTC (permalink / raw)
  To: Al Viro; +Cc: Linus Torvalds, Linux Kernel Mailing List, npiggin

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 <eric.dumazet@gmail.com>




^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16  4:16 Linux 2.6.38-rc5 Linus Torvalds
  2011-02-16 11:14 ` Eric Dumazet
@ 2011-02-16 19:26 ` Alex Riesen
  2011-02-16 19:46   ` Alex Riesen
  2011-02-17 22:13   ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Tino Keitel
  2011-02-20 14:03 ` Linux 2.6.38-rc5 Paul Rolland
  2 siblings, 2 replies; 48+ messages in thread
From: Alex Riesen @ 2011-02-16 19:26 UTC (permalink / raw)
  To: DRI mailing list, Chris Wilson; +Cc: Linus Torvalds, Linux Kernel Mailing List

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 19:26 ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Alex Riesen
@ 2011-02-16 19:46   ` Alex Riesen
  2011-02-16 19:54     ` Jesse Barnes
  2011-02-17  1:41     ` [PATCH] drm/i915: Do not handle backlight combination mode specially Indan Zupancic
  2011-02-17 22:13   ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Tino Keitel
  1 sibling, 2 replies; 48+ messages in thread
From: Alex Riesen @ 2011-02-16 19:46 UTC (permalink / raw)
  To: DRI mailing list, Chris Wilson; +Cc: Linus Torvalds, Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 20:26, Alex Riesen <raa.lkml@gmail.com> 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.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 19:46   ` Alex Riesen
@ 2011-02-16 19:54     ` Jesse Barnes
  2011-02-16 19:59       ` Alex Riesen
  2011-02-17  1:41     ` [PATCH] drm/i915: Do not handle backlight combination mode specially Indan Zupancic
  1 sibling, 1 reply; 48+ messages in thread
From: Jesse Barnes @ 2011-02-16 19:54 UTC (permalink / raw)
  To: Alex Riesen
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Wed, 16 Feb 2011 20:46:45 +0100
Alex Riesen <raa.lkml@gmail.com> wrote:

> On Wed, Feb 16, 2011 at 20:26, Alex Riesen <raa.lkml@gmail.com> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 19:54     ` Jesse Barnes
@ 2011-02-16 19:59       ` Alex Riesen
  2011-02-16 20:05         ` Jesse Barnes
  0 siblings, 1 reply; 48+ messages in thread
From: Alex Riesen @ 2011-02-16 19:59 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 20:54, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 16 Feb 2011 20:46:45 +0100
> Alex Riesen <raa.lkml@gmail.com> 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.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 19:59       ` Alex Riesen
@ 2011-02-16 20:05         ` Jesse Barnes
  2011-02-16 20:28           ` Alex Riesen
  0 siblings, 1 reply; 48+ messages in thread
From: Jesse Barnes @ 2011-02-16 20:05 UTC (permalink / raw)
  To: Alex Riesen
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Wed, 16 Feb 2011 20:59:35 +0100
Alex Riesen <raa.lkml@gmail.com> wrote:

> On Wed, Feb 16, 2011 at 20:54, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Wed, 16 Feb 2011 20:46:45 +0100
> > Alex Riesen <raa.lkml@gmail.com> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 20:05         ` Jesse Barnes
@ 2011-02-16 20:28           ` Alex Riesen
  0 siblings, 0 replies; 48+ messages in thread
From: Alex Riesen @ 2011-02-16 20:28 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 21:05, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 16 Feb 2011 20:59:35 +0100
> Alex Riesen <raa.lkml@gmail.com> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH] drm/i915: Do not handle backlight combination mode  specially.
  2011-02-16 19:46   ` Alex Riesen
  2011-02-16 19:54     ` Jesse Barnes
@ 2011-02-17  1:41     ` Indan Zupancic
  1 sibling, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-02-17  1:41 UTC (permalink / raw)
  To: Alex Riesen
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

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 <indan@nul.nu>

---

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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-16 19:26 ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Alex Riesen
  2011-02-16 19:46   ` Alex Riesen
@ 2011-02-17 22:13   ` Tino Keitel
  2011-02-18  4:57     ` Indan Zupancic
  1 sibling, 1 reply; 48+ messages in thread
From: Tino Keitel @ 2011-02-17 22:13 UTC (permalink / raw)
  To: Alex Riesen
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote:
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> 
> ---
> 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


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after  reopening lid
  2011-02-17 22:13   ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Tino Keitel
@ 2011-02-18  4:57     ` Indan Zupancic
  2011-02-19 12:11       ` Alex Riesen
  0 siblings, 1 reply; 48+ messages in thread
From: Indan Zupancic @ 2011-02-18  4:57 UTC (permalink / raw)
  To: Alex Riesen, DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-18  4:57     ` Indan Zupancic
@ 2011-02-19 12:11       ` Alex Riesen
  2011-02-19 12:26         ` Alex Riesen
  0 siblings, 1 reply; 48+ messages in thread
From: Alex Riesen @ 2011-02-19 12:11 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

Sorry for this late answer. I only get a very little time for this.

On Fri, Feb 18, 2011 at 05:57, Indan Zupancic <indan@nul.nu> 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.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-19 12:11       ` Alex Riesen
@ 2011-02-19 12:26         ` Alex Riesen
  2011-02-19 23:07           ` Linus Torvalds
  0 siblings, 1 reply; 48+ messages in thread
From: Alex Riesen @ 2011-02-19 12:26 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: DRI mailing list, Chris Wilson, Linus Torvalds,
	Linux Kernel Mailing List

On Sat, Feb 19, 2011 at 13:11, Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-19 12:26         ` Alex Riesen
@ 2011-02-19 23:07           ` Linus Torvalds
  2011-02-22 21:04             ` Jesse Barnes
  0 siblings, 1 reply; 48+ messages in thread
From: Linus Torvalds @ 2011-02-19 23:07 UTC (permalink / raw)
  To: Alex Riesen
  Cc: Indan Zupancic, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List

On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Sat, Feb 19, 2011 at 13:11, Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>

Guys, should I apply this, or will I get it through somebody's pull?

                          Linus

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: Linux 2.6.38-rc5
  2011-02-16  4:16 Linux 2.6.38-rc5 Linus Torvalds
  2011-02-16 11:14 ` Eric Dumazet
  2011-02-16 19:26 ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Alex Riesen
@ 2011-02-20 14:03 ` Paul Rolland
  2 siblings, 0 replies; 48+ messages in thread
From: Paul Rolland @ 2011-02-20 14:03 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Linux Kernel Mailing List, rol

[-- Attachment #1: Type: text/plain, Size: 3529 bytes --]

Hello,

On Tue, 15 Feb 2011 20:16:01 -0800
Linus Torvalds <torvalds@linux-foundation.org> 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-19 23:07           ` Linus Torvalds
@ 2011-02-22 21:04             ` Jesse Barnes
  2011-02-22 22:31               ` Tino Keitel
  2011-02-23  1:32               ` Indan Zupancic
  0 siblings, 2 replies; 48+ messages in thread
From: Jesse Barnes @ 2011-02-22 21:04 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Alex Riesen, Indan Zupancic, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List

On Sat, 19 Feb 2011 15:07:49 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>
> 
> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-22 21:04             ` Jesse Barnes
@ 2011-02-22 22:31               ` Tino Keitel
  2011-02-23  1:09                 ` Linus Torvalds
  2011-02-23  1:32               ` Indan Zupancic
  1 sibling, 1 reply; 48+ messages in thread
From: Tino Keitel @ 2011-02-22 22:31 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: Linus Torvalds, Alex Riesen, Indan Zupancic, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List

On Tue, Feb 22, 2011 at 13:04:40 -0800, Jesse Barnes wrote:
> On Sat, 19 Feb 2011 15:07:49 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> > > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>
> > 
> > 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-02-22 22:31               ` Tino Keitel
@ 2011-02-23  1:09                 ` Linus Torvalds
  2011-03-04  6:53                   ` Indan Zupancic
  0 siblings, 1 reply; 48+ messages in thread
From: Linus Torvalds @ 2011-02-23  1:09 UTC (permalink / raw)
  To: Jesse Barnes, Alex Riesen, Indan Zupancic, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List
  Cc: Tino Keitel

On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel <tino.keitel@tikei.de> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after  reopening lid
  2011-02-22 21:04             ` Jesse Barnes
  2011-02-22 22:31               ` Tino Keitel
@ 2011-02-23  1:32               ` Indan Zupancic
  1 sibling, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-02-23  1:32 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: Linus Torvalds, Alex Riesen, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, stable

On Tue, February 22, 2011 22:04, Jesse Barnes wrote:
> On Sat, 19 Feb 2011 15:07:49 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
>> > On Sat, Feb 19, 2011 at 13:11, Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>
>>
>> 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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after  reopening lid
  2011-02-23  1:09                 ` Linus Torvalds
@ 2011-03-04  6:53                   ` Indan Zupancic
  2011-03-04 18:47                     ` Linus Torvalds
  2011-03-05  0:26                     ` Peter Stuge
  0 siblings, 2 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-04  6:53 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jesse Barnes, Alex Riesen, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, Tino Keitel, stable

Hello,

On Wed, February 23, 2011 02:09, Linus Torvalds wrote:
> On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel <tino.keitel@tikei.de> 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 <tiwai@suse.de>
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 <tiwai@suse.de>
Cc: <stable@kernel.org>
---
 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;
 		}
 	}



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-03-04  6:53                   ` Indan Zupancic
@ 2011-03-04 18:47                     ` Linus Torvalds
  2011-03-04 23:32                       ` Indan Zupancic
                                         ` (2 more replies)
  2011-03-05  0:26                     ` Peter Stuge
  1 sibling, 3 replies; 48+ messages in thread
From: Linus Torvalds @ 2011-03-04 18:47 UTC (permalink / raw)
  To: Indan Zupancic, Alex Riesen
  Cc: Jesse Barnes, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after  reopening lid
  2011-03-04 18:47                     ` Linus Torvalds
@ 2011-03-04 23:32                       ` Indan Zupancic
  2011-03-06 17:40                       ` Alex Riesen
  2011-03-10  5:50                       ` Indan Zupancic
  2 siblings, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-04 23:32 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Alex Riesen, Jesse Barnes, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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




^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-03-04  6:53                   ` Indan Zupancic
  2011-03-04 18:47                     ` Linus Torvalds
@ 2011-03-05  0:26                     ` Peter Stuge
  1 sibling, 0 replies; 48+ messages in thread
From: Peter Stuge @ 2011-03-05  0:26 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Linux Kernel Mailing List, DRI mailing list,
	Alex Riesen, Tino Keitel, stable

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
  2011-03-04 18:47                     ` Linus Torvalds
  2011-03-04 23:32                       ` Indan Zupancic
@ 2011-03-06 17:40                       ` Alex Riesen
  2011-03-10  5:50                       ` Indan Zupancic
  2 siblings, 0 replies; 48+ messages in thread
From: Alex Riesen @ 2011-03-06 17:40 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Indan Zupancic, Jesse Barnes, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, Tino Keitel, stable

On Fri, Mar 4, 2011 at 19:47, Linus Torvalds
<torvalds@linux-foundation.org> 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 <indan@nul.nu> 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
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after  reopening lid
  2011-03-04 18:47                     ` Linus Torvalds
  2011-03-04 23:32                       ` Indan Zupancic
  2011-03-06 17:40                       ` Alex Riesen
@ 2011-03-10  5:50                       ` Indan Zupancic
  2011-03-10  6:00                         ` Indan Zupancic
  2011-03-10  7:49                         ` Takashi Iwai
  2 siblings, 2 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-10  5:50 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Alex Riesen, Jesse Barnes, DRI mailing list, Chris Wilson,
	Linux Kernel Mailing List, Tino Keitel, stable, indan,
	Takashi Iwai

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 <indan@nul.nu> 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 <indan@nul.nu>

---

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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after      reopening lid
  2011-03-10  5:50                       ` Indan Zupancic
@ 2011-03-10  6:00                         ` Indan Zupancic
  2011-03-10  7:49                         ` Takashi Iwai
  1 sibling, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-10  6:00 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable,
	Takashi Iwai

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 <indan@nul.nu>

---

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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after      reopening lid
  2011-03-10  5:50                       ` Indan Zupancic
  2011-03-10  6:00                         ` Indan Zupancic
@ 2011-03-10  7:49                         ` Takashi Iwai
  2011-03-10  8:25                           ` Takashi Iwai
  2011-03-10  8:45                           ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Indan Zupancic
  1 sibling, 2 replies; 48+ messages in thread
From: Takashi Iwai @ 2011-03-10  7:49 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
> 
> ---
> 
> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after      reopening lid
  2011-03-10  7:49                         ` Takashi Iwai
@ 2011-03-10  8:25                           ` Takashi Iwai
  2011-03-10 10:06                             ` Indan Zupancic
  2011-03-10  8:45                           ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Indan Zupancic
  1 sibling, 1 reply; 48+ messages in thread
From: Takashi Iwai @ 2011-03-10  8:25 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
> > 
> > ---
> > 
> > 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after      reopening lid
  2011-03-10  7:49                         ` Takashi Iwai
  2011-03-10  8:25                           ` Takashi Iwai
@ 2011-03-10  8:45                           ` Indan Zupancic
  2011-03-10 12:51                             ` Takashi Iwai
  1 sibling, 1 reply; 48+ messages in thread
From: Indan Zupancic @ 2011-03-10  8:45 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
>>
>> ---
>>
>> 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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after      reopening lid
  2011-03-10  8:25                           ` Takashi Iwai
@ 2011-03-10 10:06                             ` Indan Zupancic
  2011-03-10 12:59                               ` Takashi Iwai
  2011-03-10 13:02                               ` [PATCH] drm/i915: Revive combination mode for backlight control Takashi Iwai
  0 siblings, 2 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-10 10:06 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
>> >
>> > ---
>> >
>> > 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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after           reopening lid
  2011-03-10  8:45                           ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Indan Zupancic
@ 2011-03-10 12:51                             ` Takashi Iwai
  0 siblings, 0 replies; 48+ messages in thread
From: Takashi Iwai @ 2011-03-10 12:51 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
> >>
> >> ---
> >>
> >> 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] fix backlight brightness on intel LVDS panel after           reopening lid
  2011-03-10 10:06                             ` Indan Zupancic
@ 2011-03-10 12:59                               ` Takashi Iwai
  2011-03-10 13:02                               ` [PATCH] drm/i915: Revive combination mode for backlight control Takashi Iwai
  1 sibling, 0 replies; 48+ messages in thread
From: Takashi Iwai @ 2011-03-10 12:59 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

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 <indan@nul.nu> 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 <indan@nul.nu>
> >> >
> >> > ---
> >> >
> >> > 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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-10 10:06                             ` Indan Zupancic
  2011-03-10 12:59                               ` Takashi Iwai
@ 2011-03-10 13:02                               ` Takashi Iwai
  2011-03-10 19:36                                 ` Keith Packard
  2011-03-11  1:23                                 ` Indan Zupancic
  1 sibling, 2 replies; 48+ messages in thread
From: Takashi Iwai @ 2011-03-10 13:02 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb

    drm/i915: Do not handle backlight combination mode specially

since this commit introduced other regressions due to untouched LBPC
register, e.g. the backlight dimmed after resume.

In addition to the revert, this patch includes a fix for the original
issue (weird backlight levels) by removing the wrong bit shift for
computing the current backlight level.
Also, including typo fixes (lpbc -> lbpc).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
Acked-by: Indan Zupancic <indan@nul.nu>
Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_reg.h    |   10 ++++++++++
 drivers/gpu/drm/i915/intel_panel.c |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3e6f486..2abe240 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1553,7 +1553,17 @@
 
 /* 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 d860abe..f8f86e5 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,6 +30,8 @@
 
 #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)
@@ -110,6 +112,19 @@ 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;
@@ -166,6 +181,9 @@ 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);
@@ -183,6 +201,14 @@ 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;
+		}
 	}
 
 	DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +231,16 @@ 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 lbpc;
+
+		lbpc = level * 0xfe / max + 1;
+		level /= lbpc;
+		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
+	}
+
 	tmp = I915_READ(BLC_PWM_CTL);
 	if (IS_PINEVIEW(dev)) {
 		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-10 13:02                               ` [PATCH] drm/i915: Revive combination mode for backlight control Takashi Iwai
@ 2011-03-10 19:36                                 ` Keith Packard
  2011-03-11  1:30                                   ` Indan Zupancic
  2011-03-11  1:23                                 ` Indan Zupancic
  1 sibling, 1 reply; 48+ messages in thread
From: Keith Packard @ 2011-03-10 19:36 UTC (permalink / raw)
  To: Takashi Iwai, Indan Zupancic
  Cc: Linux Kernel Mailing List, DRI mailing list, Alex Riesen,
	Tino Keitel, Linus Torvalds, stable

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On Thu, 10 Mar 2011 14:02:12 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> 
>     drm/i915: Do not handle backlight combination mode specially
> 
> since this commit introduced other regressions due to untouched LBPC
> register, e.g. the backlight dimmed after resume.
> 
> In addition to the revert, this patch includes a fix for the original
> issue (weird backlight levels) by removing the wrong bit shift for
> computing the current backlight level.
> Also, including typo fixes (lpbc -> lbpc).
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
> Acked-by: Indan Zupancic <indan@nul.nu>
> Cc: <stable@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

This looks good, and ensures that LBPC ranges from 0 - 0xfe as required.

The one thing we want is a comment that explains

> +			val &= ~1;

The reason for this is that some ancient platforms wire this bit to
be "go to max backlight", or at least that's why this was in the 2D
driver from which this code was derived.

Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-10 13:02                               ` [PATCH] drm/i915: Revive combination mode for backlight control Takashi Iwai
  2011-03-10 19:36                                 ` Keith Packard
@ 2011-03-11  1:23                                 ` Indan Zupancic
  2011-03-11  1:28                                   ` Linus Torvalds
                                                     ` (2 more replies)
  1 sibling, 3 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-11  1:23 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

Hi,

Some nitpicks below. I know you're just restoring the original code,
but if we improve it we can as well do it now.

On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
> This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
>
>     drm/i915: Do not handle backlight combination mode specially
>
> since this commit introduced other regressions due to untouched LBPC
> register, e.g. the backlight dimmed after resume.

The regression was that, if ALSE was used, the maximum brightness
would be the brightness at boot, because LBPC isn't touched and
the BIOS restores it at boot. So the sympom would be less brightness
levels or lower maximum brightness.

Systems with no ASLE support work fine because there the code is only
used to save and restore the PWM register. LBPC is saved and restored
by the BIOS.

The backlight dimming after resume/blanking was the original bug
caused by the bogus val <<=1 shift.

> In addition to the revert, this patch includes a fix for the original
> issue (weird backlight levels) by removing the wrong bit shift for
> computing the current backlight level.
> Also, including typo fixes (lpbc -> lbpc).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
> Acked-by: Indan Zupancic <indan@nul.nu>
> Cc: <stable@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |   10 ++++++++++
>  drivers/gpu/drm/i915/intel_panel.c |   36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3e6f486..2abe240 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1553,7 +1553,17 @@
>
>  /* Backlight control */
>  #define BLC_PWM_CTL		0x61254
> +#define   BACKLIGHT_MODULATION_FREQ_SHIFT		(17)

This one isn't used anywhere.

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

This one isn't used and the comment is misleading, I think.

> +#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 d860abe..f8f86e5 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -30,6 +30,8 @@
>
>  #include "intel_drv.h"
>
> +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> +

I'd either put all the defines in i915_reg.h, or have all combination
mode specific defines here. Though I guess LBPC is an odd one.

>  void
>  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>  		       struct drm_display_mode *adjusted_mode)
> @@ -110,6 +112,19 @@ 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;
> @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
>  			if (INTEL_INFO(dev)->gen < 4)
>  				max &= ~1;

I'd add a comment that this is to clear the BLM_LEGACY_MODE bit.

>  		}
> +
> +		if (is_backlight_combination_mode(dev))
> +			max *= 0xff;
>  	}
>
>  	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> @@ -183,6 +201,14 @@ 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;
> +		}
>  	}
>
>  	DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> @@ -205,6 +231,16 @@ 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 lbpc;
> +
> +		lbpc = level * 0xfe / max + 1;
> +		level /= lbpc;
> +		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
> +	}
> +
>  	tmp = I915_READ(BLC_PWM_CTL);
>  	if (IS_PINEVIEW(dev)) {
>  		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);

After this patch, combined with my new patch

"drm/i915: Fix DPMS and suspend interaction for intel_panel.c"

all known backlight problems should be fixed.

Greetings,

Indan



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-11  1:23                                 ` Indan Zupancic
@ 2011-03-11  1:28                                   ` Linus Torvalds
  2011-03-11  7:26                                   ` Takashi Iwai
  2011-03-11  7:34                                   ` Keith Packard
  2 siblings, 0 replies; 48+ messages in thread
From: Linus Torvalds @ 2011-03-11  1:28 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Takashi Iwai, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable,
	Keith Packard

On Thu, Mar 10, 2011 at 5:23 PM, Indan Zupancic <indan@nul.nu> wrote:
>
> After this patch, combined with my new patch
>
> "drm/i915: Fix DPMS and suspend interaction for intel_panel.c"
>
> all known backlight problems should be fixed.

Btw, can you repost that one as a new email (and cc keithp too)? I
think it got hidden in the other thread by different subject line.

Jesse, did you take a look at that one? It was in the thread with a
subject like "[PATCH] fix backlight brightness on intel LVDS panel
after reopening lid".

Search for

  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.

in your mailbox.

                    Linus

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-10 19:36                                 ` Keith Packard
@ 2011-03-11  1:30                                   ` Indan Zupancic
  0 siblings, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-11  1:30 UTC (permalink / raw)
  To: Keith Packard
  Cc: Takashi Iwai, Linux Kernel Mailing List, DRI mailing list,
	Alex Riesen, Tino Keitel, Linus Torvalds, stable

On Thu, March 10, 2011 20:36, Keith Packard wrote:
> On Thu, 10 Mar 2011 14:02:12 +0100, Takashi Iwai <tiwai@suse.de> wrote:
>> +			val &= ~1;
>
> The reason for this is that some ancient platforms wire this bit to
> be "go to max backlight", or at least that's why this was in the 2D
> driver from which this code was derived.

If that is the case, then shouldn't it be at the end, after val *= lbpc?
I know it doesn't make much difference, as multiplying with an even number
always gives an even result, but it would make the intention clearer and
give a more precise result.

What about gen 3? Does it support combination mode too?

If you can confirm that there are no gen 2 systems with ASLE support,
we can remove the gen 2 check and only touch the PWM register, as the
ASLE code is the only one that changes the brightness. The panel code
only saves and restores it, and LBPC is saved and restored by the
BIOS already. Then those weird gen 2 quirks can be removed.

(Something for 2.6.39 perhaps.)

It's sad that something as simple as backlight control needs so much
code.

Greetings,

Indan



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-11  1:23                                 ` Indan Zupancic
  2011-03-11  1:28                                   ` Linus Torvalds
@ 2011-03-11  7:26                                   ` Takashi Iwai
  2011-03-11  9:08                                     ` Indan Zupancic
  2011-03-11  7:34                                   ` Keith Packard
  2 siblings, 1 reply; 48+ messages in thread
From: Takashi Iwai @ 2011-03-11  7:26 UTC (permalink / raw)
  To: Indan Zupancic
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

At Fri, 11 Mar 2011 02:23:08 +0100 (CET),
Indan Zupancic wrote:
> 
> Hi,
> 
> Some nitpicks below. I know you're just restoring the original code,
> but if we improve it we can as well do it now.

Well, Linus already merged my patch quickly.  So, we can improve the
readability as an additional patch, but I think it's likely a 2.6.39
material.

All you comments below look reasonable.  Could you send a new patch?


thanks,

Takashi


> On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
> > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> >
> >     drm/i915: Do not handle backlight combination mode specially
> >
> > since this commit introduced other regressions due to untouched LBPC
> > register, e.g. the backlight dimmed after resume.
> 
> The regression was that, if ALSE was used, the maximum brightness
> would be the brightness at boot, because LBPC isn't touched and
> the BIOS restores it at boot. So the sympom would be less brightness
> levels or lower maximum brightness.
> 
> Systems with no ASLE support work fine because there the code is only
> used to save and restore the PWM register. LBPC is saved and restored
> by the BIOS.
> 
> The backlight dimming after resume/blanking was the original bug
> caused by the bogus val <<=1 shift.
> 
> > In addition to the revert, this patch includes a fix for the original
> > issue (weird backlight levels) by removing the wrong bit shift for
> > computing the current backlight level.
> > Also, including typo fixes (lpbc -> lbpc).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
> > Acked-by: Indan Zupancic <indan@nul.nu>
> > Cc: <stable@kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |   10 ++++++++++
> >  drivers/gpu/drm/i915/intel_panel.c |   36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 3e6f486..2abe240 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1553,7 +1553,17 @@
> >
> >  /* Backlight control */
> >  #define BLC_PWM_CTL		0x61254
> > +#define   BACKLIGHT_MODULATION_FREQ_SHIFT		(17)
> 
> This one isn't used anywhere.
> 
> >  #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)
> 
> This one isn't used and the comment is misleading, I think.
> 
> > +#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 d860abe..f8f86e5 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -30,6 +30,8 @@
> >
> >  #include "intel_drv.h"
> >
> > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
> > +
> 
> I'd either put all the defines in i915_reg.h, or have all combination
> mode specific defines here. Though I guess LBPC is an odd one.
> 
> >  void
> >  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> >  		       struct drm_display_mode *adjusted_mode)
> > @@ -110,6 +112,19 @@ 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;
> > @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> >  			if (INTEL_INFO(dev)->gen < 4)
> >  				max &= ~1;
> 
> I'd add a comment that this is to clear the BLM_LEGACY_MODE bit.
> 
> >  		}
> > +
> > +		if (is_backlight_combination_mode(dev))
> > +			max *= 0xff;
> >  	}
> >
> >  	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> > @@ -183,6 +201,14 @@ 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;
> > +		}
> >  	}
> >
> >  	DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
> > @@ -205,6 +231,16 @@ 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 lbpc;
> > +
> > +		lbpc = level * 0xfe / max + 1;
> > +		level /= lbpc;
> > +		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
> > +	}
> > +
> >  	tmp = I915_READ(BLC_PWM_CTL);
> >  	if (IS_PINEVIEW(dev)) {
> >  		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
> 
> After this patch, combined with my new patch
> 
> "drm/i915: Fix DPMS and suspend interaction for intel_panel.c"
> 
> all known backlight problems should be fixed.
> 
> Greetings,
> 
> Indan
> 
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-11  1:23                                 ` Indan Zupancic
  2011-03-11  1:28                                   ` Linus Torvalds
  2011-03-11  7:26                                   ` Takashi Iwai
@ 2011-03-11  7:34                                   ` Keith Packard
  2 siblings, 0 replies; 48+ messages in thread
From: Keith Packard @ 2011-03-11  7:34 UTC (permalink / raw)
  To: Indan Zupancic, Takashi Iwai
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Fri, 11 Mar 2011 02:23:08 +0100 (CET), "Indan Zupancic" <indan@nul.nu> wrote:

> Some nitpicks below. I know you're just restoring the original code,
> but if we improve it we can as well do it now.

Let's pend these changes until after 2.6.38; the backlight code is
fragile enough without trying to 'clean it up' at this point in the
release cycle.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH] drm/i915: Revive combination mode for backlight control
  2011-03-11  7:26                                   ` Takashi Iwai
@ 2011-03-11  9:08                                     ` Indan Zupancic
  0 siblings, 0 replies; 48+ messages in thread
From: Indan Zupancic @ 2011-03-11  9:08 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Linus Torvalds, Alex Riesen, Jesse Barnes, DRI mailing list,
	Chris Wilson, Linux Kernel Mailing List, Tino Keitel, stable

On Fri, March 11, 2011 08:26, Takashi Iwai wrote:
> At Fri, 11 Mar 2011 02:23:08 +0100 (CET),
> Indan Zupancic wrote:
>>
>> Hi,
>>
>> Some nitpicks below. I know you're just restoring the original code,
>> but if we improve it we can as well do it now.
>
> Well, Linus already merged my patch quickly.  So, we can improve the
> readability as an additional patch, but I think it's likely a 2.6.39
> material.
>
> All you comments below look reasonable.  Could you send a new patch?

Well, my main concern was the slightly incorrect commit message,
but oh well, too late for that now. I'll probably send a patch
after 2.6.38 is released.

Greetings,

Indan



^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2011-03-11  9:08 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-16  4:16 Linux 2.6.38-rc5 Linus Torvalds
2011-02-16 11:14 ` Eric Dumazet
2011-02-16 13:55   ` Eric Dumazet
2011-02-16 15:46   ` Linus Torvalds
2011-02-16 16:06     ` Al Viro
2011-02-16 16:19       ` Al Viro
2011-02-16 16:33         ` Linus Torvalds
2011-02-16 16:39           ` Al Viro
2011-02-16 16:47             ` Eric Dumazet
2011-02-16 16:22     ` Eric Dumazet
2011-02-16 19:26 ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Alex Riesen
2011-02-16 19:46   ` Alex Riesen
2011-02-16 19:54     ` Jesse Barnes
2011-02-16 19:59       ` Alex Riesen
2011-02-16 20:05         ` Jesse Barnes
2011-02-16 20:28           ` Alex Riesen
2011-02-17  1:41     ` [PATCH] drm/i915: Do not handle backlight combination mode specially Indan Zupancic
2011-02-17 22:13   ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Tino Keitel
2011-02-18  4:57     ` Indan Zupancic
2011-02-19 12:11       ` Alex Riesen
2011-02-19 12:26         ` Alex Riesen
2011-02-19 23:07           ` Linus Torvalds
2011-02-22 21:04             ` Jesse Barnes
2011-02-22 22:31               ` Tino Keitel
2011-02-23  1:09                 ` Linus Torvalds
2011-03-04  6:53                   ` Indan Zupancic
2011-03-04 18:47                     ` Linus Torvalds
2011-03-04 23:32                       ` Indan Zupancic
2011-03-06 17:40                       ` Alex Riesen
2011-03-10  5:50                       ` Indan Zupancic
2011-03-10  6:00                         ` Indan Zupancic
2011-03-10  7:49                         ` Takashi Iwai
2011-03-10  8:25                           ` Takashi Iwai
2011-03-10 10:06                             ` Indan Zupancic
2011-03-10 12:59                               ` Takashi Iwai
2011-03-10 13:02                               ` [PATCH] drm/i915: Revive combination mode for backlight control Takashi Iwai
2011-03-10 19:36                                 ` Keith Packard
2011-03-11  1:30                                   ` Indan Zupancic
2011-03-11  1:23                                 ` Indan Zupancic
2011-03-11  1:28                                   ` Linus Torvalds
2011-03-11  7:26                                   ` Takashi Iwai
2011-03-11  9:08                                     ` Indan Zupancic
2011-03-11  7:34                                   ` Keith Packard
2011-03-10  8:45                           ` [PATCH] fix backlight brightness on intel LVDS panel after reopening lid Indan Zupancic
2011-03-10 12:51                             ` Takashi Iwai
2011-03-05  0:26                     ` Peter Stuge
2011-02-23  1:32               ` Indan Zupancic
2011-02-20 14:03 ` Linux 2.6.38-rc5 Paul Rolland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).