drm/i915: Do a lightweight pm_get() from intel_mark_busy()
diff mbox

Message ID 1435133343-15563-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson June 24, 2015, 8:09 a.m. UTC
Akash noticed that we were recursing from the call to
intel_runtime_pm_get() inside intel_mark_busy() when we were already
waking the device (through another intel_runtime_pm_get()). In
intel_mark_busy() we know the device is awake and purpose of the
reference here is to simply keep the device awake until the GPU is idle
again. As such we do not need the full resume, and can call the lighter
intel_runtime_pm_get_noresume() instead.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Akash Goel <akash.goel@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Vetter June 24, 2015, 8:40 a.m. UTC | #1
On Wed, Jun 24, 2015 at 09:09:03AM +0100, Chris Wilson wrote:
> Akash noticed that we were recursing from the call to
> intel_runtime_pm_get() inside intel_mark_busy() when we were already
> waking the device (through another intel_runtime_pm_get()). In
> intel_mark_busy() we know the device is awake and purpose of the
> reference here is to simply keep the device awake until the GPU is idle
> again. As such we do not need the full resume, and can call the lighter
> intel_runtime_pm_get_noresume() instead.

How does that happen? I only see a mark_busy in add_request, and we
shouldn't call that from intel_rpm_get(). Or do we? Calltrace of how this
happens would be great.
-Daniel

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a43abc38af90..da0d882cc71e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10668,7 +10668,7 @@ void intel_mark_busy(struct drm_device *dev)
>  	if (dev_priv->mm.busy)
>  		return;
>  
> -	intel_runtime_pm_get(dev_priv);
> +	intel_runtime_pm_get_noresume(dev_priv);
>  	intel_rps_busy(dev_priv);
>  	dev_priv->mm.busy = true;
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson June 24, 2015, 8:43 a.m. UTC | #2
On Wed, Jun 24, 2015 at 10:40:02AM +0200, Daniel Vetter wrote:
> On Wed, Jun 24, 2015 at 09:09:03AM +0100, Chris Wilson wrote:
> > Akash noticed that we were recursing from the call to
> > intel_runtime_pm_get() inside intel_mark_busy() when we were already
> > waking the device (through another intel_runtime_pm_get()). In
> > intel_mark_busy() we know the device is awake and purpose of the
> > reference here is to simply keep the device awake until the GPU is idle
> > again. As such we do not need the full resume, and can call the lighter
> > intel_runtime_pm_get_noresume() instead.
> 
> How does that happen? I only see a mark_busy in add_request, and we
> shouldn't call that from intel_rpm_get(). Or do we? Calltrace of how this
> happens would be great.

We are looking at VLV workarounds for which i915_gem_init_hw() is acting
very fishy. The recursion is a result of that, but that is just a
demonstration that we didn't need the full pm_get() here.
-Chris
Chris Wilson June 24, 2015, 9:26 a.m. UTC | #3
On Wed, Jun 24, 2015 at 10:40:02AM +0200, Daniel Vetter wrote:
> On Wed, Jun 24, 2015 at 09:09:03AM +0100, Chris Wilson wrote:
> > Akash noticed that we were recursing from the call to
> > intel_runtime_pm_get() inside intel_mark_busy() when we were already
> > waking the device (through another intel_runtime_pm_get()). In
> > intel_mark_busy() we know the device is awake and purpose of the
> > reference here is to simply keep the device awake until the GPU is idle
> > again. As such we do not need the full resume, and can call the lighter
> > intel_runtime_pm_get_noresume() instead.
> 
> How does that happen? I only see a mark_busy in add_request, and we
> shouldn't call that from intel_rpm_get(). Or do we? Calltrace of how this
> happens would be great.

From the vlv wa:

[<ffffffff81b2d426>] dump_stack+0x4e/0x7a
[<ffffffff8108c40d>] warn_slowpath_common+0x7d/0xa0
[<ffffffff8108c4ea>] warn_slowpath_null+0x1a/0x20
[<ffffffff815656e5>] intel_runtime_pm_get+0xc5/0xf0
[<ffffffff81583f2e>] __i915_add_request+0x5e/0x340
[<ffffffff815772a3>] i915_gem_context_enable+0x93/0xb0
[<ffffffff81586e19>] i915_gem_init_hw+0xd9/0x270
[<ffffffff81552585>] intel_runtime_resume+0x105/0x370
[<ffffffff8146e569>] ? pci_restore_state.part.36+0xc9/0x1f0
[<ffffffff8147103b>] pci_pm_runtime_resume+0x7b/0xc0
[<ffffffff81470fc0>] ? pci_restore_standard_config+0x60/0x60
[<ffffffff8162a7f2>] __rpm_callback+0x32/0x70
[<ffffffff8162a856>] rpm_callback+0x26/0xa0
[<ffffffff8162b0f6>] rpm_resume+0x496/0x6f0
[<ffffffff8162c33f>] __pm_runtime_resume+0x4f/0x90
[<ffffffff815656a6>] intel_runtime_pm_get+0x86/0xf0
[<ffffffff81565865>] intel_display_power_rpm_get+0x125/0x170
[<ffffffff815658c3>] intel_display_power_get+0x13/0x20
[<ffffffff815d66a7>] intel_connector_dpms+0x57/0x120
[<ffffffff815394e9>] drm_mode_obj_set_property_ioctl+0x399/0x3a0
[<ffffffff815281c9>] drm_ioctl+0x1e9/0x5a0
[<ffffffff81539150>] ? drm_mode_obj_get_properties_ioctl+0x140/0x140
[<ffffffff813ccb57>] ? inode_has_perm.isra.32+0x27/0x40
[<ffffffff813ccc86>] ? file_has_perm+0x86/0xa0
[<ffffffff815698d5>] i915_compat_ioctl+0x45/0x50
[<ffffffff8120cb30>] compat_sys_ioctl+0xc0/0x13e0
[<ffffffff811d517e>] ? __fget+0x6e/0xb0
[<ffffffff81436a8b>] ? trace_hardirqs_on_thunk+0x3a/0x3c
[<ffffffff81b3c189>] ia32_do_call+0x13/0x1
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a43abc38af90..da0d882cc71e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10668,7 +10668,7 @@  void intel_mark_busy(struct drm_device *dev)
 	if (dev_priv->mm.busy)
 		return;
 
-	intel_runtime_pm_get(dev_priv);
+	intel_runtime_pm_get_noresume(dev_priv);
 	intel_rps_busy(dev_priv);
 	dev_priv->mm.busy = true;
 }