| Message ID | 1308761701-1751-2-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > Get some more useful info for debugging backtraces on forcewake > problems. Appropriate debug flags will be required for the drm module. Wouldn't it be neater to pass the reg down to gen6_gt_force_wake_get() so that we only have a single debug message and not an additional one for every inlined read/write? -Chris
On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote: > On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > Get some more useful info for debugging backtraces on forcewake > > problems. Appropriate debug flags will be required for the drm module. > > Wouldn't it be neater to pass the reg down to > gen6_gt_force_wake_get() so that we only have a single debug message and > not an additional one for every inlined read/write? > -Chris > Urg... having some trouble with my email right now, so you have to deal with my gmail/webmail... 1. Yes, but I was lazy. 2. People call force_wake_get() without actually specifying a register, so I didn't want to enforce people entering bogus info on those calls. > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, 22 Jun 2011 10:07:50 -0700, Ben Widawsky <bwidawsk@gmail.com> wrote: > On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote: > > > On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > > Get some more useful info for debugging backtraces on forcewake > > > problems. Appropriate debug flags will be required for the drm module. > > > > Wouldn't it be neater to pass the reg down to > > gen6_gt_force_wake_get() so that we only have a single debug message and > > not an additional one for every inlined read/write? > > -Chris > > > > Urg... having some trouble with my email right now, so you have to deal with > my gmail/webmail... > > 1. Yes, but I was lazy. > 2. People call force_wake_get() without actually specifying a register, so I > didn't want to enforce people entering bogus info on those calls. We could always go for the traditional file:line automagic macros... But we already have that from the stacktrace if people are good enough to provide symbols. -Chris
On Wed, Jun 22, 2011 at 06:15:40PM +0100, Chris Wilson wrote: > On Wed, 22 Jun 2011 10:07:50 -0700, Ben Widawsky <bwidawsk@gmail.com> wrote: > > On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote: > > > > > On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > Get some more useful info for debugging backtraces on forcewake > > > > problems. Appropriate debug flags will be required for the drm module. > > > > > > Wouldn't it be neater to pass the reg down to > > > gen6_gt_force_wake_get() so that we only have a single debug message and > > > not an additional one for every inlined read/write? > > > -Chris > > > > > > > Urg... having some trouble with my email right now, so you have to deal with > > my gmail/webmail... > > > > 1. Yes, but I was lazy. > > 2. People call force_wake_get() without actually specifying a register, so I > > didn't want to enforce people entering bogus info on those calls. > > We could always go for the traditional file:line automagic macros... > But we already have that from the stacktrace if people are good enough to > provide symbols. Yeah, I figured this would be easiest as it somewhat removes variance between commits, I can just search for the unprotected register access on the branch du jour (especially with you guys refactoring the code all time time... JBARNES). I'm fine if the consensus is to throw this one out. Seemed helpful at the time, so I thought I'd post it. > -Chris Ben
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0defd42..af59811 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -317,13 +317,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) * be called at the beginning of the sequence followed by a call to * gen6_gt_force_wake_put() at the end of the sequence. */ -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) +int gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) { - WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); + int ret = WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); /* Forcewake is atomic in case we get in here without the lock */ if (atomic_add_return(1, &dev_priv->forcewake_count) == 1) __gen6_gt_force_wake_get(dev_priv); + + return ret; } static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) @@ -335,12 +337,14 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) /* * see gen6_gt_force_wake_get() */ -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) +int gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) { - WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); + int ret = WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); if (atomic_dec_and_test(&dev_priv->forcewake_count)) __gen6_gt_force_wake_put(dev_priv); + + return ret; } void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8a9fd91..7421eb9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1362,8 +1362,8 @@ extern void intel_display_print_error_state(struct seq_file *m, * must be set to prevent GT core from power down and stale values being * returned. */ -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv); -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv); +int gen6_gt_force_wake_get(struct drm_i915_private *dev_priv); +int gen6_gt_force_wake_put(struct drm_i915_private *dev_priv); void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv); /* We give fast paths for the really cool registers */ @@ -1376,7 +1376,9 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv); static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \ u##x val = 0; \ if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ - gen6_gt_force_wake_get(dev_priv); \ + if (gen6_gt_force_wake_get(dev_priv)) { \ + DRM_DEBUG_DRIVER("unlocked forcewake for 0x%08x\n", reg); \ + } \ val = read##y(dev_priv->regs + reg); \ gen6_gt_force_wake_put(dev_priv); \ } else { \
Get some more useful info for debugging backtraces on forcewake problems. Appropriate debug flags will be required for the drm module. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.c | 12 ++++++++---- drivers/gpu/drm/i915/i915_drv.h | 8 +++++--- 2 files changed, 13 insertions(+), 7 deletions(-)