Re: [Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended

2014-07-24 Thread Daniel Vetter 
On Wed, Jul 23, 2014 at 08:38:19PM -0300, Paulo Zanoni wrote:
 2014-07-23 19:41 GMT-03:00 Daniel Vetter dan...@ffwll.ch:
  On Thu, Jul 24, 2014 at 12:35:25AM +0200, Daniel Vetter wrote:
  On Wed, Jul 23, 2014 at 06:30:59PM -0300, Paulo Zanoni wrote:
   From: Paulo Zanoni paulo.r.zan...@intel.com
  
   If we're runtime suspended and try to use the cursor interfaces, we
   will get a lot of WARNs saying we did the wrong thing.
  
   For intel_crtc_update_cursor(), all we need to do is return if the
   CRTC is not active, since writing the registers won't really have any
   effect if the screen is not visible, and we will write the registers
   later when enabling the screen.
  
   For intel_crtc_cursor_set_obj(), we just get the proper power domain
   reference, since this function does a lot of stuff.
  
   Testcase: igt/pm_rpm/cursor
   Testcase: igt/pm_rpm/cursor-dpms
   Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
   Cc: sta...@vger.kernel.org
   Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
   ---
drivers/gpu/drm/i915/intel_display.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
  
   diff --git a/drivers/gpu/drm/i915/intel_display.c 
   b/drivers/gpu/drm/i915/intel_display.c
   index d1e9570..c8f36b0 100644
   --- a/drivers/gpu/drm/i915/intel_display.c
   +++ b/drivers/gpu/drm/i915/intel_display.c
   @@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct 
   drm_crtc *crtc,
   if (base == 0  intel_crtc-cursor_base == 0)
   return;
  
   +   if (!intel_crtc-active)
   +   return;
 
  Don't we need the same trick in intel_crtc_cursor_set_obj? This gets
  called if the cursor object changes (instead of just moving it around).
 
  Rechecked and realized the only I915_WRITE in there is for gen2. I guess
  we don't care ;-)
 
 Nope. You need to look at the subfunctions and their subsubfunctions
 and their subsubsubfunctions and so on. This is what happens when I
 remove just the display_power_get/put calls:
 
 [   35.762635] [drm:intel_crtc_cursor_set_obj] cursor off
 [   35.762665] [drm:add_framebuffer_internal] [FB:63]
 [   35.762685] [ cut here ]
 [   35.762714] WARNING: CPU: 0 PID: 3169 at
 drivers/gpu/drm/i915/i915_gem_gtt.c:1480
 gen6_ggtt_insert_entries+0x116/0x120 [i915]()
 [   35.762716] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal
 intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm
 i2c_i801 mei_me mei i2c_designware_platform i2c_designware_core sg
 sd_mod ehci_pci ehci_hcd ahci libahci e1000e xhci_hcd sdhci_acpi sdhci
 [   35.762748] CPU: 0 PID: 3169 Comm: pm_rpm Not tainted
 3.16.0-rc6.1407232028+ #695
 [   35.762751] Hardware name: Intel Corporation Shark Bay Client
 platform/WhiteTip Mountain 1, BIOS HSWLPTU1.86C.0137.R00.1403031632
 03/03/2014
 [   35.762754]  0009 88009ce139c8 816b6a61
 
 [   35.762760]  88009ce13a00 81075d88 92946000
 
 [   35.762765]  c90010d04d74 88009d938bb8 c90010d04d68
 88009ce13a10
 [   35.762770] Call Trace:
 [   35.762782]  [816b6a61] dump_stack+0x4d/0x66
 [   35.762789]  [81075d88] warn_slowpath_common+0x78/0xa0
 [   35.762793]  [81075e65] warn_slowpath_null+0x15/0x20
 [   35.762814]  [a0169716] gen6_ggtt_insert_entries+0x116/0x120 
 [i915]
 [   35.762831]  [a0168ce9] ggtt_bind_vma+0xd9/0x100 [i915]
 [   35.762850]  [a0172583] i915_gem_object_pin+0x683/0x750 [i915]
 [   35.762869]  [a0173cd7]
 i915_gem_object_pin_to_display_plane+0x97/0x1d0 [i915]
 [   35.762894]  [a01a891c]
 intel_crtc_cursor_set_obj+0x16c/0x520 [i915]
 [   35.762916]  [a01a8df5] intel_cursor_plane_update+0xe5/0x120 
 [i915]
 [   35.762937]  [a00d83a4] setplane_internal+0x264/0x2b0 [drm]
 [   35.762952]  [a00d850e] drm_mode_cursor_common+0x11e/0x320 [drm]
 [   35.762968]  [a00dbb0c] drm_mode_cursor_ioctl+0x3c/0x40 [drm]
 [   35.762978]  [a00cb87f] drm_ioctl+0x1df/0x6a0 [drm]
 [   35.762983]  [816be9b9] ? mutex_unlock+0x9/0x10
 [   35.762988]  [811eaae6] ? seq_read+0xb6/0x3e0
 [   35.762994]  [811d97e0] do_vfs_ioctl+0x2e0/0x4e0
 [   35.762998]  [816c0bf7] ? sysret_check+0x1b/0x56
 [   35.763004]  [810c47fd] ? trace_hardirqs_on_caller+0x15d/0x200
 [   35.763008]  [811d9a61] SyS_ioctl+0x81/0xa0
 [   35.763013]  [816c0bd2] system_call_fastpath+0x16/0x1b
 [   35.763015] ---[ end trace 189706dc7c79e8d7 ]---
 [   35.763018] [ cut here ]
 [   35.763039] WARNING: CPU: 0 PID: 3169 at
 drivers/gpu/drm/i915/intel_uncore.c:47
 assert_device_not_suspended.isra.8+0x43/0x50 [i915]()
 [   35.763041] Device suspended
 [   35.763043] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal
 intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm
 i2c_i801 mei_me mei i2c_designware_platform

[Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended

2014-07-23 Thread Paulo Zanoni 
From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the cursor interfaces, we
will get a lot of WARNs saying we did the wrong thing.

For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.

For intel_crtc_cursor_set_obj(), we just get the proper power domain
reference, since this function does a lot of stuff.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index d1e9570..c8f36b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
*crtc,
if (base == 0  intel_crtc-cursor_base == 0)
return;
 
+   if (!intel_crtc-active)
+   return;
+
I915_WRITE(CURPOS(pipe), pos);
 
if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8181,6 +8184,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
uint32_t addr;
int ret;
 
+   intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE(pipe));
+
/* if we want to turn off the cursor ignore width and height */
if (!obj) {
DRM_DEBUG_KMS(cursor off\n);
@@ -8195,13 +8200,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
(width == 128  height == 128  !IS_GEN2(dev)) ||
(width == 256  height == 256  !IS_GEN2(dev {
DRM_DEBUG(Cursor dimension not supported\n);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto fail;
}
 
if (obj-base.size  width * height * 4) {
DRM_DEBUG_KMS(buffer is too small\n);
ret = -ENOMEM;
-   goto fail;
+   goto fail_unref;
}
 
/* we only need to pin inside GTT if cursor is non-phy */
@@ -8275,13 +8281,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 
+   intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return 0;
 fail_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
mutex_unlock(dev-struct_mutex);
-fail:
+fail_unref:
drm_gem_object_unreference_unlocked(obj-base);
+fail:
+   intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return ret;
 }
 
-- 
2.0.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended

2014-07-23 Thread Daniel Vetter 
On Wed, Jul 23, 2014 at 06:30:59PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 If we're runtime suspended and try to use the cursor interfaces, we
 will get a lot of WARNs saying we did the wrong thing.
 
 For intel_crtc_update_cursor(), all we need to do is return if the
 CRTC is not active, since writing the registers won't really have any
 effect if the screen is not visible, and we will write the registers
 later when enabling the screen.
 
 For intel_crtc_cursor_set_obj(), we just get the proper power domain
 reference, since this function does a lot of stuff.
 
 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index d1e9570..c8f36b0 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
 *crtc,
   if (base == 0  intel_crtc-cursor_base == 0)
   return;
  
 + if (!intel_crtc-active)
 + return;

Don't we need the same trick in intel_crtc_cursor_set_obj? This gets
called if the cursor object changes (instead of just moving it around).
 +
   I915_WRITE(CURPOS(pipe), pos);
  
   if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
 @@ -8181,6 +8184,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   uint32_t addr;
   int ret;
  
 + intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE(pipe));

This shouldn't be needed - if intel_crtc-active is set, we do hold a
reference to the pipe power domain already. There's no way the pipe would
be able to run otherwise ;-)
-Daniel

 +
   /* if we want to turn off the cursor ignore width and height */
   if (!obj) {
   DRM_DEBUG_KMS(cursor off\n);
 @@ -8195,13 +8200,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   (width == 128  height == 128  !IS_GEN2(dev)) ||
   (width == 256  height == 256  !IS_GEN2(dev {
   DRM_DEBUG(Cursor dimension not supported\n);
 - return -EINVAL;
 + ret = -EINVAL;
 + goto fail;
   }
  
   if (obj-base.size  width * height * 4) {
   DRM_DEBUG_KMS(buffer is too small\n);
   ret = -ENOMEM;
 - goto fail;
 + goto fail_unref;
   }
  
   /* we only need to pin inside GTT if cursor is non-phy */
 @@ -8275,13 +8281,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  
   intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
  
 + intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
   return 0;
  fail_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  fail_locked:
   mutex_unlock(dev-struct_mutex);
 -fail:
 +fail_unref:
   drm_gem_object_unreference_unlocked(obj-base);
 +fail:
 + intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
   return ret;
  }
  
 -- 
 2.0.1
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended

2014-07-23 Thread Paulo Zanoni 
2014-07-23 19:41 GMT-03:00 Daniel Vetter dan...@ffwll.ch:
 On Thu, Jul 24, 2014 at 12:35:25AM +0200, Daniel Vetter wrote:
 On Wed, Jul 23, 2014 at 06:30:59PM -0300, Paulo Zanoni wrote:
  From: Paulo Zanoni paulo.r.zan...@intel.com
 
  If we're runtime suspended and try to use the cursor interfaces, we
  will get a lot of WARNs saying we did the wrong thing.
 
  For intel_crtc_update_cursor(), all we need to do is return if the
  CRTC is not active, since writing the registers won't really have any
  effect if the screen is not visible, and we will write the registers
  later when enabling the screen.
 
  For intel_crtc_cursor_set_obj(), we just get the proper power domain
  reference, since this function does a lot of stuff.
 
  Testcase: igt/pm_rpm/cursor
  Testcase: igt/pm_rpm/cursor-dpms
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
  Cc: sta...@vger.kernel.org
  Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 15 ---
   1 file changed, 12 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index d1e9570..c8f36b0 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
  *crtc,
  if (base == 0  intel_crtc-cursor_base == 0)
  return;
 
  +   if (!intel_crtc-active)
  +   return;

 Don't we need the same trick in intel_crtc_cursor_set_obj? This gets
 called if the cursor object changes (instead of just moving it around).

 Rechecked and realized the only I915_WRITE in there is for gen2. I guess
 we don't care ;-)

Nope. You need to look at the subfunctions and their subsubfunctions
and their subsubsubfunctions and so on. This is what happens when I
remove just the display_power_get/put calls:

[   35.762635] [drm:intel_crtc_cursor_set_obj] cursor off
[   35.762665] [drm:add_framebuffer_internal] [FB:63]
[   35.762685] [ cut here ]
[   35.762714] WARNING: CPU: 0 PID: 3169 at
drivers/gpu/drm/i915/i915_gem_gtt.c:1480
gen6_ggtt_insert_entries+0x116/0x120 [i915]()
[   35.762716] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal
intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm
i2c_i801 mei_me mei i2c_designware_platform i2c_designware_core sg
sd_mod ehci_pci ehci_hcd ahci libahci e1000e xhci_hcd sdhci_acpi sdhci
[   35.762748] CPU: 0 PID: 3169 Comm: pm_rpm Not tainted
3.16.0-rc6.1407232028+ #695
[   35.762751] Hardware name: Intel Corporation Shark Bay Client
platform/WhiteTip Mountain 1, BIOS HSWLPTU1.86C.0137.R00.1403031632
03/03/2014
[   35.762754]  0009 88009ce139c8 816b6a61

[   35.762760]  88009ce13a00 81075d88 92946000

[   35.762765]  c90010d04d74 88009d938bb8 c90010d04d68
88009ce13a10
[   35.762770] Call Trace:
[   35.762782]  [816b6a61] dump_stack+0x4d/0x66
[   35.762789]  [81075d88] warn_slowpath_common+0x78/0xa0
[   35.762793]  [81075e65] warn_slowpath_null+0x15/0x20
[   35.762814]  [a0169716] gen6_ggtt_insert_entries+0x116/0x120 [i915]
[   35.762831]  [a0168ce9] ggtt_bind_vma+0xd9/0x100 [i915]
[   35.762850]  [a0172583] i915_gem_object_pin+0x683/0x750 [i915]
[   35.762869]  [a0173cd7]
i915_gem_object_pin_to_display_plane+0x97/0x1d0 [i915]
[   35.762894]  [a01a891c]
intel_crtc_cursor_set_obj+0x16c/0x520 [i915]
[   35.762916]  [a01a8df5] intel_cursor_plane_update+0xe5/0x120 [i915]
[   35.762937]  [a00d83a4] setplane_internal+0x264/0x2b0 [drm]
[   35.762952]  [a00d850e] drm_mode_cursor_common+0x11e/0x320 [drm]
[   35.762968]  [a00dbb0c] drm_mode_cursor_ioctl+0x3c/0x40 [drm]
[   35.762978]  [a00cb87f] drm_ioctl+0x1df/0x6a0 [drm]
[   35.762983]  [816be9b9] ? mutex_unlock+0x9/0x10
[   35.762988]  [811eaae6] ? seq_read+0xb6/0x3e0
[   35.762994]  [811d97e0] do_vfs_ioctl+0x2e0/0x4e0
[   35.762998]  [816c0bf7] ? sysret_check+0x1b/0x56
[   35.763004]  [810c47fd] ? trace_hardirqs_on_caller+0x15d/0x200
[   35.763008]  [811d9a61] SyS_ioctl+0x81/0xa0
[   35.763013]  [816c0bd2] system_call_fastpath+0x16/0x1b
[   35.763015] ---[ end trace 189706dc7c79e8d7 ]---
[   35.763018] [ cut here ]
[   35.763039] WARNING: CPU: 0 PID: 3169 at
drivers/gpu/drm/i915/intel_uncore.c:47
assert_device_not_suspended.isra.8+0x43/0x50 [i915]()
[   35.763041] Device suspended
[   35.763043] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal
intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm
i2c_i801 mei_me mei i2c_designware_platform i2c_designware_core sg
sd_mod ehci_pci ehci_hcd ahci libahci e1000e xhci_hcd sdhci_acpi sdhci
[   35.763070] CPU: 0 PID: 3169 Comm: pm_rpm Tainted: GW