[Intel-gfx] [PATCH 1/8] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter]

On Thu, Dec 11, 2014 at 10:15:58AM +0000, Chris Wilson wrote:
> On Fri, Dec 12, 2014 at 03:30:14PM +0530, Deepak S wrote:
> > 
> > On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote:
> > >From: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > >Calling intel_runtime_pm_put() is illegal from a soft-irq context, so
> > >revert the crude hack
> > >
> > >commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c
> > >Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > >Date:   Tue Apr 1 14:55:07 2014 -0300
> > >
> > >     drm/i915: don't schedule force_wake_timer at gen6_read
> > >
> > >and apply the single line corrective instead.
> > >
> > >References: https://bugs.freedesktop.org/show_bug.cgi?id=80913
> > >Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > >Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > >---
> > >@@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> > >  	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> > >  		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> > >  						      FORCEWAKE_ALL); \
> > >-		val = __raw_i915_read##x(dev_priv, reg); \
> > >-		dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> > >-						      FORCEWAKE_ALL); \
> > >-	} else { \
> > >-		val = __raw_i915_read##x(dev_priv, reg); \
> > >+		dev_priv->uncore.forcewake_count++; \
> > >+		mod_timer_pinned(&dev_priv->uncore. , \
> > >+				 jiffies + 1); \
> > 
> > why timer, we can do a put after register read right?
> 
> The presumption is that we will do another mmio access requiring the
> forcewake very shortly, and we want to avoid the forcewake clear/ack
> cycle. So we defer dropping the forcewake until the end of the kernel
> context on this cpu (the goal being that as the scheduler switches back
> to the userspace context, we release the wakelock, it would be great if
> there was an explicit callback for that...).

task_work might be what you're looking for. It's a horribly crude
interface though, so we'd need a small wrapper which either picks the task
work or timer. Which complicates the logic further since we then have
either or both lazy cleanup tasks pending.

Probably not worth the trouble.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch