[Intel-gfx] [BUG, bisect] Re: drm/i915: WARN_ON(dev_priv->mm.busy)
Jani, On Mon, Jun 15, 2015 at 02:40:42PM +0300, Jani Nikula wrote: > On Mon, 15 Jun 2015, Ville Syrjälä wrote: > > On Mon, Jun 15, 2015 at 01:25:38AM -0700, Jeremiah Mahler wrote: > >> Daniel, > >> > >> On Mon, Jun 15, 2015 at 08:57:47AM +0200, Daniel Vetter wrote: > >> > Can you please retest with > >> > > >> > commit 0aedb1626566efd72b369c01992ee7413c82a0c5 > >> > Author: Ville Syrjälä > >> > Date: Thu May 28 18:32:36 2015 +0300 > >> > > >> > drm/i915: Don't skip request retirement if the active list is empty > >> > > >> > Thanks, Daniel > >> > > >> > >> The bug is still present with that patch applied. And it is still > >> present up to linux-next 20150611. > > > > The patch was misapplied, so what's in the tree at the moment isn't what > > I sent to the list. > > This should be rectified in current drm-intel-nightly branch of > [1]. Jeremiah, please give that a try. > > BR, > Jani. > > > [1] http://cgit.freedesktop.org/drm-intel > > > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Jani Nikula, Intel Open Source Technology Center I tested drm-intel-nightly and all the warnings appear to be resolved in there. So when these get to -next it should be good. -- - Jeremiah Mahler
[Intel-gfx] [BUG, bisect] Re: drm/i915: WARN_ON(dev_priv->mm.busy)
Daniel, On Mon, Jun 15, 2015 at 08:57:47AM +0200, Daniel Vetter wrote: > Can you please retest with > > commit 0aedb1626566efd72b369c01992ee7413c82a0c5 > Author: Ville Syrjälä > Date: Thu May 28 18:32:36 2015 +0300 > > drm/i915: Don't skip request retirement if the active list is empty > > Thanks, Daniel > The bug is still present with that patch applied. And it is still present up to linux-next 20150611. -- - Jeremiah Mahler
[BUG, bisect] Re: drm/i915: WARN_ON(dev_priv->mm.busy)
all, On Sat, Jun 06, 2015 at 08:09:34PM -0700, Jeremiah Mahler wrote: > all, > > On all my machines with Intel graphics I get the following warning > in the logs when the machine is suspended. Apparently some part of > the graphics system is busy when it should be idle. This is present > on the latest linux-next 20150604. > > ... > [ 33.141747] Suspending console(s) (use no_console_suspend to debug) > [ 33.142146] wlan0: deauthenticating from 00:1a:70:5a:6e:0b by local > choice (Reason: 3=DEAUTH_LEAVING) > [ 33.147395] queueing ieee80211 work while going to suspend > [ 33.151597] cfg80211: Calling CRDA to update world regulatory domain > [ 33.190430] sd 0:0:0:0: [sda] Synchronizing SCSI cache > [ 33.190523] sd 0:0:0:0: [sda] Stopping disk > [ 33.275743] [ cut here ] > [ 33.275764] WARNING: CPU: 0 PID: 1617 at > drivers/gpu/drm/i915/i915_gem.c:4808 i915_gem_suspend+0xe4/0xf0 [i915]() > [ 33.275766] WARN_ON(dev_priv->mm.busy) > [ 33.275811] Modules linked in: binfmt_misc snd_hda_codec_hdmi > hid_generic isl29018(C) industrialio regmap_i2c cyapatp crc_itu_t usbhid > hid arc4 x86_pkg_temp_thermal intel_powerclamp intel_rapl iosf_mbi > coretemp ath9k tpm_infineon kvm_intel kvm ath9k_common ath9k_hw > crct10dif_pclmul crc32_pclmul crc32c_intel chromeos_laptop ath mac80211 > ghash_clmulni_intel cryptd i915 cfg80211 pcspkr serio_raw sg ath3k btusb > btrtl lpc_ich snd_hda_codec_realtek shpchp i2c_i801 mfd_core > snd_hda_codec_generic btbcm btintel bluetooth snd_hda_intel battery > snd_hda_codec ac i2c_algo_bit drm_kms_helper tpm_tis snd_hwdep tpm > snd_hda_core drm snd_pcm video rfkill processor button snd_timer snd > soundcore i2c_designware_pci i2c_designware_core evdev uvcvideo > videobuf2_vmalloc videobuf2_memops videobuf2_core v4l2_common videodev > [ 33.275825] media i2c_core fuse autofs4 ext4 crc16 mbcache jbd2 > sd_mod fan xhci_pci sdhci_acpi sdhci xhci_hcd mmc_core thermal > thermal_sys usbcore ahci libahci usb_common libata scsi_mod > [ 33.275828] CPU: 0 PID: 1617 Comm: kworker/u4:4 Tainted: G C > 4.1.0-rc6-next-20150604+ #207 > [ 33.275829] Hardware name: Acer Peppy, BIOS 04/30/2014 > [ 33.275834] Workqueue: events_unbound async_run_entry_fn > [ 33.275838] a05b7908 8152ca4d > 880035effc58 > [ 33.275840] 8106bce1 880073587f20 > 88007358 > [ 33.275842] 88003534f860 88007358 8106bd5a > a05c74c1 > [ 33.275843] Call Trace: > [ 33.275849] [] ? dump_stack+0x40/0x50 > [ 33.275853] [] ? warn_slowpath_common+0x81/0xb0 > [ 33.275855] [] ? warn_slowpath_fmt+0x4a/0x50 > [ 33.275865] [] ? i915_gem_suspend+0xe4/0xf0 [i915] > [ 33.275872] [] ? i915_drm_suspend+0x61/0x1b0 > [i915] > [ 33.275876] [] ? pci_pm_suspend+0x71/0x140 > [ 33.275878] [] ? pci_pm_freeze+0xd0/0xd0 > [ 33.275881] [] ? dpm_run_callback+0x39/0xd0 > [ 33.275883] [] ? __device_suspend+0xe4/0x300 > [ 33.275884] [] ? async_suspend+0x1e/0x90 > [ 33.275887] [] ? async_run_entry_fn+0x43/0x150 > [ 33.275890] [] ? process_one_work+0x148/0x3b0 > [ 33.275892] [] ? worker_thread+0x4a/0x440 > [ 33.275895] [] ? rescuer_thread+0x2e0/0x2e0 > [ 33.275898] [] ? kthread+0xc1/0xe0 > [ 33.275901] [] ? > kthread_create_on_node+0x190/0x190 > [ 33.275904] [] ? ret_from_fork+0x3f/0x70 > [ 33.275907] [] ? > kthread_create_on_node+0x190/0x190 > [ 33.275908] ---[ end trace e1c3eb5e163b3520 ]--- > [ 33.560558] PM: suspend of devices complete after 423.034 msecs > [ 33.577985] PM: late suspend of devices complete after 17.589 msecs > [ 33.579036] xhci_hcd :00:14.0: System wakeup enabled by ACPI > [ 33.594059] PM: noirq suspend of devices complete after 16.226 msecs > [ 33.594498] ACPI: Preparing to enter system sleep state S3 > [ 33.595066] ACPI : EC: EC stopped > ... > > -- > - Jeremiah Mahler I bisected the kernel and found that the following patch introduced the bug. From b47161858ba13c9c7e0132230d66e008dd55 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Mon, 27 Apr 2015 13:41:17 +0100 Subject: [PATCH] drm/i915: Implement inter-engine read-read optimisations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, we only track the last request globally across all engines. This prevents us from issuing concurrent read requests on e.g. the RCS and BCS engines (or more likely the render and media engines). Without semaphores, we incur costly stalls as we synchronise between rings - g
drm/i915: WARN_ON(dev_priv->mm.busy)
all, On all my machines with Intel graphics I get the following warning in the logs when the machine is suspended. Apparently some part of the graphics system is busy when it should be idle. This is present on the latest linux-next 20150604. ... [ 33.141747] Suspending console(s) (use no_console_suspend to debug) [ 33.142146] wlan0: deauthenticating from 00:1a:70:5a:6e:0b by local choice (Reason: 3=DEAUTH_LEAVING) [ 33.147395] queueing ieee80211 work while going to suspend [ 33.151597] cfg80211: Calling CRDA to update world regulatory domain [ 33.190430] sd 0:0:0:0: [sda] Synchronizing SCSI cache [ 33.190523] sd 0:0:0:0: [sda] Stopping disk [ 33.275743] [ cut here ] [ 33.275764] WARNING: CPU: 0 PID: 1617 at drivers/gpu/drm/i915/i915_gem.c:4808 i915_gem_suspend+0xe4/0xf0 [i915]() [ 33.275766] WARN_ON(dev_priv->mm.busy) [ 33.275811] Modules linked in: binfmt_misc snd_hda_codec_hdmi hid_generic isl29018(C) industrialio regmap_i2c cyapatp crc_itu_t usbhid hid arc4 x86_pkg_temp_thermal intel_powerclamp intel_rapl iosf_mbi coretemp ath9k tpm_infineon kvm_intel kvm ath9k_common ath9k_hw crct10dif_pclmul crc32_pclmul crc32c_intel chromeos_laptop ath mac80211 ghash_clmulni_intel cryptd i915 cfg80211 pcspkr serio_raw sg ath3k btusb btrtl lpc_ich snd_hda_codec_realtek shpchp i2c_i801 mfd_core snd_hda_codec_generic btbcm btintel bluetooth snd_hda_intel battery snd_hda_codec ac i2c_algo_bit drm_kms_helper tpm_tis snd_hwdep tpm snd_hda_core drm snd_pcm video rfkill processor button snd_timer snd soundcore i2c_designware_pci i2c_designware_core evdev uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core v4l2_common videodev [ 33.275825] media i2c_core fuse autofs4 ext4 crc16 mbcache jbd2 sd_mod fan xhci_pci sdhci_acpi sdhci xhci_hcd mmc_core thermal thermal_sys usbcore ahci libahci usb_common libata scsi_mod [ 33.275828] CPU: 0 PID: 1617 Comm: kworker/u4:4 Tainted: G C 4.1.0-rc6-next-20150604+ #207 [ 33.275829] Hardware name: Acer Peppy, BIOS 04/30/2014 [ 33.275834] Workqueue: events_unbound async_run_entry_fn [ 33.275838] a05b7908 8152ca4d 880035effc58 [ 33.275840] 8106bce1 880073587f20 88007358 [ 33.275842] 88003534f860 88007358 8106bd5a a05c74c1 [ 33.275843] Call Trace: [ 33.275849] [] ? dump_stack+0x40/0x50 [ 33.275853] [] ? warn_slowpath_common+0x81/0xb0 [ 33.275855] [] ? warn_slowpath_fmt+0x4a/0x50 [ 33.275865] [] ? i915_gem_suspend+0xe4/0xf0 [i915] [ 33.275872] [] ? i915_drm_suspend+0x61/0x1b0 [i915] [ 33.275876] [] ? pci_pm_suspend+0x71/0x140 [ 33.275878] [] ? pci_pm_freeze+0xd0/0xd0 [ 33.275881] [] ? dpm_run_callback+0x39/0xd0 [ 33.275883] [] ? __device_suspend+0xe4/0x300 [ 33.275884] [] ? async_suspend+0x1e/0x90 [ 33.275887] [] ? async_run_entry_fn+0x43/0x150 [ 33.275890] [] ? process_one_work+0x148/0x3b0 [ 33.275892] [] ? worker_thread+0x4a/0x440 [ 33.275895] [] ? rescuer_thread+0x2e0/0x2e0 [ 33.275898] [] ? kthread+0xc1/0xe0 [ 33.275901] [] ? kthread_create_on_node+0x190/0x190 [ 33.275904] [] ? ret_from_fork+0x3f/0x70 [ 33.275907] [] ? kthread_create_on_node+0x190/0x190 [ 33.275908] ---[ end trace e1c3eb5e163b3520 ]--- [ 33.560558] PM: suspend of devices complete after 423.034 msecs [ 33.577985] PM: late suspend of devices complete after 17.589 msecs [ 33.579036] xhci_hcd :00:14.0: System wakeup enabled by ACPI [ 33.594059] PM: noirq suspend of devices complete after 16.226 msecs [ 33.594498] ACPI: Preparing to enter system sleep state S3 [ 33.595066] ACPI : EC: EC stopped ... -- - Jeremiah Mahler
[Intel-gfx] [BUG, bisect] drm/i915: mouse pointer lags and overshoots
Daniel, Matt, On Sat, Jan 24, 2015 at 12:24:55PM +0100, Daniel Vetter wrote: > On Fri, Jan 23, 2015 at 10:57:32PM -0800, Jeremiah Mahler wrote: > > all, > > > > On Tue, Jan 20, 2015 at 06:48:42AM +0100, Daniel Vetter wrote: > > > On Mon, Jan 19, 2015 at 08:40:24AM -0800, Matt Roper wrote: > > > > On Mon, Jan 19, 2015 at 11:04:04AM +, Chris Wilson wrote: > > > > > On Mon, Jan 19, 2015 at 11:51:43AM +0100, Daniel Vetter wrote: [...] > drm/plane-helper: Skip prepare_fb/cleanup_fb when newfb==oldfb > > that should rectify the sluggish i915 cursor. But it's in a separate topic > branch which isn't in linux-next. Should show up in drm-next next week > though. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch I found the patch and tried it out. It does fix the mouse pointer problem. Thanks for the fix. -- - Jeremiah Mahler
[Intel-gfx] [BUG, bisect] drm/i915: mouse pointer lags and overshoots
all, On Tue, Jan 20, 2015 at 06:48:42AM +0100, Daniel Vetter wrote: > On Mon, Jan 19, 2015 at 08:40:24AM -0800, Matt Roper wrote: > > On Mon, Jan 19, 2015 at 11:04:04AM +, Chris Wilson wrote: > > > On Mon, Jan 19, 2015 at 11:51:43AM +0100, Daniel Vetter wrote: > > > > There's also an issue in (most) X drivers which exaberates this > > > > issues: When changing the cursor buffer the X cursor code does a a) > > > > disable cursor b) update cursor image c) enable cursor cycle. > > > > > > Notably not -intel on which the bug has been observed. And more > > > importantly, the slow downs don't seem to correlate with cursor change, > > > just cursor movement. > > > -Chris > > > > > > -- > > > Chris Wilson, Intel Open Source Technology Centre > > > > It seems that the simple fix for this case (movement only) is to just > > skip the prepare_fb/cleanup_fb calls (and the associated vblank wait) in > > the transitional plane helper when newfb == oldfb. I just posted a > > small patch that makes that change (and solves the cursor lag for me). > > > > This won't solve the case if userspace uses a different framebuffer for > > each update (while trying to update faster than the refresh rate). Is > > there any existing userspace that behaves this way that we can test > > with? > > Hm, I've thought I've merged that patch already: > > commit ab58e3384b9f9863bfd029b458ff337d381bf6d2 > Author: Daniel Vetter > Date: Mon Nov 24 20:42:42 2014 +0100 > > drm/atomic-helper: Skip vblank waits for unchanged fbs > > Or is the problem here that the transitional plane helpers aren't up to > the task? If so please reference that in your patch. > > And we still need a hack for the "changed fb cursor" issue, I'll whip > something up. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch Just checking if anyone has come up with a fix. I am still stuck at next-20150112 because of this bug. -- - Jeremiah Mahler
[BUG, bisect] drm/i915: mouse pointer lags and overshoots
Matt, all, Commit ea2c67bb4aff introduces a bug which causes the mouse to move in a very unusual way, as if it has a lot of inertia. It will lag behind and then overshoot the expected position. I reproduced this bug on all my machines which use the drm/i915 drivers and it affects all forms of mouse pointers including both touchpads and usb mice. The patch is present in linux-next 20150116. commit ea2c67bb4affa84080c616920f3899f123786e56 Author: Matt Roper Date: Tue Dec 23 10:41:52 2014 -0800 drm/i915: Move to atomic plane helpers (v9) Switch plane handling to use the atomic plane helpers. This means that rather than provide our own implementations of .update_plane() and .disable_plane(), we expose the lower-level check/prepare/commit/cleanup entrypoints and let the DRM core implement update/disable for us using those entrypoints. The other main change that falls out of this patch is that our drm_plane's will now always have a valid plane->state that contains the relevant plane state (initial state is allocated at plane creation). The base drm_plane_state pointed to holds the requested source/dest coordinates, and the subclassed intel_plane_state holds the adjusted values that our driver actually uses. v2: - Renamed file from intel_atomic.c to intel_atomic_plane.c (Daniel) - Fix a copy/paste comment mistake (Bob) v3: - Use prepare/cleanup functions that we've already factored out - Use newly refactored pre_commit/commit/post_commit to avoid sleeping during vblank evasion v4: - Rebase to latest di-nightly requires adding an 'old_state' parameter to atomic_update; v5: - Must have botched a rebase somewhere and lost some work. Restore state 'dirty' flag to let begin/end code know which planes to run the pre_commit/post_commit hooks for. This would have actually shown up as broken in the next commit rather than this one. v6: - Squash kerneldoc patch into this one. - Previous patches have now already taken care of most of the infrastructure that used to be in this patch. All we're adding here now is some thin wrappers. v7: - Check return of intel_plane_duplicate_state() for allocation failures. v8: - Drop unused drm_plane_state -> intel_plane_state cast. (Ander) - Squash in actual transition to plane helpers. Significant refactoring earlier in the patchset has made the combined prep+transition much easier to swallow than it was in earlier iterations. (Ander) v9: - s/track_fbs/disabled_planes/ in the atomic crtc flags. The only fb's we need to update frontbuffer tracking for are those on a plane about to be disabled (since the atomic helpers never call prepare_fb() when disabling a plane), so the new name more accurately describes what we're actually tracking. Testcase: igt/kms_plane Testcase: igt/kms_universal_plane Testcase: igt/kms_cursor_crc Signed-off-by: Matt Roper Reviewed-by: Ander Conselvan de Oliveira Signed-off-by: Daniel Vetter -- - Jeremiah Mahler
[PATCH v2] drm/i915: fix inconsistent brightness after resume
Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
brightness levels on different machines. If a suspended was entered
with the screen off some machines would resume with the screen
at minimum brightness and others at maximum brightness.
The following commands can be used to produce this behavior.
xset dpms force off
sleep 1
sudo systemctl suspend
(resume ...)
The root cause of this problem is a comparison which checks to see if
the backlight level is zero when the panel is enabled. If it is zero,
it is set to the maximum level. Unfortunately, not all machines have a
minimum level of zero. On those machines the level is left at the
minimum instead of begin set to the maximum.
Fix the bug by updating the comparison to check for the minimum
backlight level instead of zero. Also, expand the comparison for
the possible case when the level is less than the minimum.
Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
Signed-off-by: Jeremiah Mahler
---
Notes:
Changes in v2:
- Expand the comparision for the possible case when
the level is less than the minimum.
drivers/gpu/drm/i915/intel_panel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index 4d63839..dfb783a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector
*connector)
WARN_ON(panel->backlight.max == 0);
- if (panel->backlight.level == 0) {
+ if (panel->backlight.level <= panel->backlight.min) {
panel->backlight.level = panel->backlight.max;
if (panel->backlight.device)
panel->backlight.device->props.brightness =
--
2.1.4
[PATCH] drm/i915: fix inconsistent brightness after resume
Jani,
On Mon, Jan 12, 2015 at 12:31:09PM +0200, Jani Nikula wrote:
> On Sat, 10 Jan 2015, Jeremiah Mahler wrote:
[...]
>
> I think part of the problem is that the userspace sets brightness to
> minimum before suspend, but apparently does not restore it after
> resume. The dmesg would confirm this. But I guess it doesn't matter,
> since we're pretty much stuck with having to do this anyway.
>
I did notice it doing this. There were several calls to *_update_status
as it was entering suspend which set it to the minimum.
I am not familiar with the intricate details of this system but it seems
like there must be a way to fix this. If the backlight can be powered
off and back on with the correct level it seems like it should be
possible when a suspend/resume is involved.
[...]
> > - if (panel->backlight.level == 0) {
> > + if (panel->backlight.level == panel->backlight.min) {
>
> Perhaps <= instead of == would be safest?
>
We could do that too in case that corner case ever arises.
[...]
I will fix it up in v2.
--
- Jeremiah Mahler
[PATCH] drm/i915: fix inconsistent brightness after resume
Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
brightness levels on different machines. If a suspended was entered
with the screen off some machines would resume with the screen
at minimum brightness and others at maximum brightness.
The following commands can be used to produce this behavior.
xset dpms force off
sleep 1
sudo systemctl suspend
(resume ...)
The root cause of this problem is a comparison which checks to see if
the backlight level is zero when the panel is enabled. If it is zero,
it is set to the maximum level. Unfortunately, not all machines have a
minimum level of zero. On those machines the level is left at the
minimum instead of begin set to the maximum.
Fix the bug by updating the comparison to check for the minimum
backlight level instead of zero.
Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
Signed-off-by: Jeremiah Mahler
---
drivers/gpu/drm/i915/intel_panel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index 4d63839..4ef4d66 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector
*connector)
WARN_ON(panel->backlight.max == 0);
- if (panel->backlight.level == 0) {
+ if (panel->backlight.level == panel->backlight.min) {
panel->backlight.level = panel->backlight.max;
if (panel->backlight.device)
panel->backlight.device->props.brightness =
--
2.1.4
[BUG] drm/i915: backlight off after resume
Jani, all, On a Lenovo X1 Carbon if the display is off when suspend is entered it will be off when it is resumed. A key must be pressed to restore normal brightness. xset dpms force off sleep 1 sudo systemctl suspend (resume) (screen off, press any key) The behavior I am accustomed to is for it to resume with the screen on. All of my other machines behave this way and the X1 Carbon behaved this way in the past. I performed a bisect and found that the following commit introduced the problem. From 6dda730e55f412a6dfb181cae6784822ba463847 Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Tue, 24 Jun 2014 18:27:40 +0300 Subject: [PATCH] drm/i915: respect the VBT minimum backlight brightness Historically we've exposed the full backlight PWM duty cycle range to the userspace, in the name of "mechanism, not policy". However, it turns out there are both panels and board designs where there is a minimum duty cycle that is required for proper operation. The minimum duty cycle is available in the VBT. The backlight class sysfs interface does not make any promises to the userspace about the physical meaning of the range 0..max_brightness. Specifically there is no guarantee that 0 means off; indeed for acpi_backlight 0 usually is not off, but the minimum acceptable value. Respect the minimum backlight, and expose the range acceptable to the hardware as 0..max_brightness to the userspace via the backlight class device; 0 means the minimum acceptable enabled value. To switch off the backlight, the user must disable the encoder. As a side effect, make the backlight class device max brightness and physical PWM modulation frequency (i.e. max duty cycle) independent. This allows a follow-up patch to virtualize the max value exposed to the userspace. Signed-off-by: Jani Nikula Reviewed-by: Jesse Barnes [danvet: s/BUG_ON/WARN_ON/] Signed-off-by: Daniel Vetter -- - Jeremiah Mahler
[PATCH] drm: Move two seq_printf's outside of locked mutex
Jonas, On Wed, Dec 31, 2014 at 09:55:19AM +0100, Jonas Lundqvist wrote: > Hi Jeremiah, > > On 12/30/2014 11:52 PM, Jeremiah Mahler wrote: > > You changed 'i' but you didn't explain in your log message why you did this. > > I can change the commit message to something more generic. "Move code > outside of locked mutex" or similar. > That still doesn't explain why you changed the 'i' variable. > > Does this change really improve anything? It may work the same with the > > locks moved around. But if you look at the function as a whole, the > > locks encapsulate the body of this function nicely. I like the original > > design better. > > The locking was already done this way, ie after the seq_printf, in the > functions drm_clients_info() and drm_gem_name_info() in thr same file. > So this change is really more of an alignment. > Your right, those two have have the lock after the seq_printf. But the drm_bufs_info() function has its lock before the seq_printf. So before your change about half are one way and half are the other. I am still not convinced that either of these ways is better or makes any difference whatsoever. > Best regards > Jonas > -- - Jeremiah Mahler
[PATCH] drm: Move two seq_printf's outside of locked mutex
Jonas,
On Tue, Dec 30, 2014 at 10:54:26PM +0100, Jonas Lundqvist wrote:
> In drm_info.c: drm_bufs_info() and drm_vm_info() two seq_printf() was
> done unnecessarily while locking a mutex. This patch flips the order of
> the print and lock/unlock where applicable.
>
> Signed-off-by: Jonas Lundqvist
> ---
> drivers/gpu/drm/drm_info.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
> index 51efebd..981afe7 100644
> --- a/drivers/gpu/drm/drm_info.c
> +++ b/drivers/gpu/drm/drm_info.c
> @@ -81,11 +81,10 @@ int drm_vm_info(struct seq_file *m, void *data)
> _DRM_SCATTER_GATHER and _DRM_CONSISTENT */
> const char *types[] = { "FB", "REG", "SHM", "AGP", "SG", "PCI" };
> const char *type;
> - int i;
> + int i = 0;
>
> - mutex_lock(&dev->struct_mutex);
> seq_printf(m, "slot offset size type flagsaddress
> mtrr\n\n");
> - i = 0;
> + mutex_lock(&dev->struct_mutex);
> list_for_each_entry(r_list, &dev->maplist, head) {
> map = r_list->map;
> if (!map)
> @@ -147,8 +146,8 @@ int drm_bufs_info(struct seq_file *m, void *data)
> seq_printf(m, "\n");
> seq_printf(m, " %d", dma->buflist[i]->list);
> }
> - seq_printf(m, "\n");
> mutex_unlock(&dev->struct_mutex);
> + seq_printf(m, "\n");
> return 0;
> }
>
You changed 'i' but you didn't explain in your log message why you did this.
Does this change really improve anything? It may work the same with the
locks moved around. But if you look at the function as a whole, the
locks encapsulate the body of this function nicely. I like the original
design better.
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
- Jeremiah Mahler
[PATCH] nouveau: fix ambiguous backlight controls
Hans,
On Sat, Dec 27, 2014 at 02:39:42PM +0100, Hans de Goede wrote:
> Hi,
>
> On 27-12-14 00:51, Jeremiah Mahler wrote:
> >Ilia,
> >
> >On Fri, Dec 26, 2014 at 04:39:08PM -0500, Ilia Mirkin wrote:
> >>On Fri, Dec 26, 2014 at 4:26 PM, Jeremiah Mahler
> >>wrote:
> >>>If a display supports backlight control using the nouveau driver, and
> >>>also supports standard ACPI backlight control, there will be two sets of
> >>>controls.
> >>>
> >>>/sys/class/backlight/acpi_video0
> >>>/sys/class/backlight/nv_backlight
> >>>
> >>>This creates ambiguity because these controls can be out of sync with
> >>>each other. One could be at 100% while the other is at 0% and the
> >>>actual display brightness depends on which one was used last. This also
> >>>creates anomalies in Powertop which will show two values for brightness
> >>>with potentially different values.
> >>>
> >>>Fix this ambiguity by having the nouveau driver only enable its
> >>>backlight controls if the standard ACPI controls are not present.
> >>>
> >>>Signed-off-by: Jeremiah Mahler
> >>>---
> >>> drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +
> >>> 1 file changed, 5 insertions(+)
> >>>
> >>>diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> >>>b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> >>>index e566c5b..3a52bd4 100644
> >>>--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> >>>+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> >>>@@ -221,6 +221,11 @@ nouveau_backlight_init(struct drm_device *dev)
> >>> struct nvif_device *device = &drm->device;
> >>> struct drm_connector *connector;
> >>>
> >>>+ if (acpi_video_backlight_support()) {
> >>
> >>None of the other drivers have this. Is nouveau somehow different
> >>than, say, radeon in this respect?
> >>
> >>Unfortunately the backlight situation is pretty fubar'd... sometimes
> >>the acpi controls don't work, sometimes the card controls don't work,
> >>sometimes they both work but in different ways (and then everyone's
> >>favourite -- neither works, and there's some third platform thing).
> >>I'm pretty sure this code existed before, but got removed. See commit
> >>bee564430feec1175ee64bcfd4913cacc519f817 and the previous commit
> >>5bead799d3f8 before that. The ping-pong is probably not the right way
> >>to go.
> >>
> >
> >I was not aware of that change. But you are right, it took out what I
> >was trying to put back in.
> >
> >Thanks for the helpful information. I will have to rethink this fix.
>
> So first of all NAK to the original fix, but I think that much was
> already clear.
>
> Let me explain how this currently works, most laptops have up to 3
> backlight control interfaces (all talking to the same single backlight):
>
> acpi_video: a standardized acpi interface for backlight control, broken on
> most
> win8 ready laptops.
>
> vendor: e.g. asus_wmi, dell_laptop, etc. typically not much better on
> win8 ready laptops.
>
> native: e.g. intel_backlight, nv_backlight, usually your best bet on win8
> laptops, but not so much on older models.
>
> Before windows8 only 2 of these 3 get registered / exported to userspace,
> either you've:
>
> acpi_video + native:
>
> or:
>
> vendor + native:
>
> Since most vendor drivers contain:
>
> if (acpi_video_backlight_support())
> return 0;
>
> And userspace backlight control code knows the prefer the firmware interfaces
> over the native one and to simply ignore the native interface, unless there
> is no firmware interface, so having 2 interfaces present in sysfs is not
> really a problem as userspace knows how to deal with this.
>
> So along came Windows 8, breaking most acpi_video implementations. This got
> fixed by a new module parameter to the acpi_video driver called
> use_native_backlight,
> which now a days defaults to 1. When this parameter is true *and* the BIOS is
> a win8 ready bios, then acpi_video will not register a backlight interface
> itself,
> and acpi_video_backlight_support() will still return 1, causing the vendor
> interfaces
> to not register. Leaving only the native interface.
>
So acpi_video_backlight_support() will return true for win8 even when ACPI
isn't actually supported. Could this have been fixed by updating
acpi_v
[PATCH] nouveau: fix ambiguous backlight controls
Ilia,
On Fri, Dec 26, 2014 at 04:39:08PM -0500, Ilia Mirkin wrote:
> On Fri, Dec 26, 2014 at 4:26 PM, Jeremiah Mahler
> wrote:
> > If a display supports backlight control using the nouveau driver, and
> > also supports standard ACPI backlight control, there will be two sets of
> > controls.
> >
> > /sys/class/backlight/acpi_video0
> > /sys/class/backlight/nv_backlight
> >
> > This creates ambiguity because these controls can be out of sync with
> > each other. One could be at 100% while the other is at 0% and the
> > actual display brightness depends on which one was used last. This also
> > creates anomalies in Powertop which will show two values for brightness
> > with potentially different values.
> >
> > Fix this ambiguity by having the nouveau driver only enable its
> > backlight controls if the standard ACPI controls are not present.
> >
> > Signed-off-by: Jeremiah Mahler
> > ---
> > drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > index e566c5b..3a52bd4 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > @@ -221,6 +221,11 @@ nouveau_backlight_init(struct drm_device *dev)
> > struct nvif_device *device = &drm->device;
> > struct drm_connector *connector;
> >
> > + if (acpi_video_backlight_support()) {
>
> None of the other drivers have this. Is nouveau somehow different
> than, say, radeon in this respect?
>
> Unfortunately the backlight situation is pretty fubar'd... sometimes
> the acpi controls don't work, sometimes the card controls don't work,
> sometimes they both work but in different ways (and then everyone's
> favourite -- neither works, and there's some third platform thing).
> I'm pretty sure this code existed before, but got removed. See commit
> bee564430feec1175ee64bcfd4913cacc519f817 and the previous commit
> 5bead799d3f8 before that. The ping-pong is probably not the right way
> to go.
>
I was not aware of that change. But you are right, it took out what I
was trying to put back in.
Thanks for the helpful information. I will have to rethink this fix.
[...]
--
- Jeremiah Mahler
[PATCH] nouveau: fix ambiguous backlight controls
If a display supports backlight control using the nouveau driver, and
also supports standard ACPI backlight control, there will be two sets of
controls.
/sys/class/backlight/acpi_video0
/sys/class/backlight/nv_backlight
This creates ambiguity because these controls can be out of sync with
each other. One could be at 100% while the other is at 0% and the
actual display brightness depends on which one was used last. This also
creates anomalies in Powertop which will show two values for brightness
with potentially different values.
Fix this ambiguity by having the nouveau driver only enable its
backlight controls if the standard ACPI controls are not present.
Signed-off-by: Jeremiah Mahler
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index e566c5b..3a52bd4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -221,6 +221,11 @@ nouveau_backlight_init(struct drm_device *dev)
struct nvif_device *device = &drm->device;
struct drm_connector *connector;
+ if (acpi_video_backlight_support()) {
+ dev_info(dev->dev, "Standard ACPI backlight control supported,
disabling local control.\n");
+ return 0;
+ }
+
list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
connector->connector_type != DRM_MODE_CONNECTOR_eDP)
--
2.1.4
