| Message ID | 1302381082-3167-5-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 9 Apr 2011 13:31:22 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > + * > + * Intelligent users of the interface may do a force_wake_get() followed > + * by many register reads and writes, knowing that the reference count > + * is already incremented. So we do not want to warn on those. > */ Hmm, any place where we touch registers without holding a lock, unless under exceptional conditions such as !SYSTEM_RUNNING, is a bug waiting to happen. And probably already happened. Nasty, hard to reproduce race conditions. The complication is that some registers will be protected by dev->config.mode_lock rather than dev->struct_mutex (and a very rare set are protected by their own irq spinlocks). This sounds like a job for lockdep... And a lot of documentation. Fortunately, we only have those two conditions (KMS (detection and mode setting) vs GEM (execution and memory managment)) and their intersection to worry about. One of the first steps would be to review all the comments Jesse added to document the KMS locking and back those up with assertions and updates. Plenty of work to do before we can feel sure that the locking is sane. -Chris
On Sat, Apr 09, 2011 at 11:31:14PM +0100, Chris Wilson wrote: > On Sat, 9 Apr 2011 13:31:22 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > + * > > + * Intelligent users of the interface may do a force_wake_get() followed > > + * by many register reads and writes, knowing that the reference count > > + * is already incremented. So we do not want to warn on those. > > */ > > Hmm, any place where we touch registers without holding a lock, unless > under exceptional conditions such as !SYSTEM_RUNNING, is a bug waiting to > happen. And probably already happened. Nasty, hard to reproduce race > conditions. The only use I had in mind was doing a bunch of reads, or perhaps polling on a register. I didn't have a specific case but it seemed feasible in certain cases. > -Chris Ben
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 96b3bfc..6da7079 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -286,8 +286,13 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) * immediately. Not having the lock causes a race, but all bets are off * when using forced forcewake, which should only be touched through * root-only entry in debugfs. + * + * Intelligent users of the interface may do a force_wake_get() followed + * by many register reads and writes, knowing that the reference count + * is already incremented. So we do not want to warn on those. */ - WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); + WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex) && + !dev_priv->forcewake_count); if (dev_priv->forcewake_count++ == 0) __gen6_gt_force_wake_get(dev_priv); @@ -301,7 +306,8 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) { - WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); + WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex) && + !dev_priv->forcewake_count); if (--dev_priv->forcewake_count == 0) __gen6_gt_force_wake_put(dev_priv);
This patch will likely produce much fewer warnings, but perhaps hide some bugs in the driver. Any warnings while using this patch are extremely likely to cause problems, while warnings without this patch are also driver bugs, but much less likely to be causing issues. Without this patch we may also get false warnings for intelligent users of the new refcount mechanism. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)