Re: [PATCH] Revert "drm/i915/vlv: Remove check for Old Ack during forcewake"

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 07, 2014 at 02:58:37PM +0000, Dave Gordon wrote:
> On 06/11/14 14:09, Daniel Vetter wrote:
> > On Wed, Nov 05, 2014 at 05:30:52PM +0200, Mika Kuoppala wrote:
> >> This reverts commit 5cb13c07dae73380d8b3ddc792740487b8742938.
> >>
> >> While the relevance for WaRsDontPollForAckOnClearingFWBits is under
> >> investigation, revert this as regression.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> >> Tested-by:   Tested-by: lu hua <huax.lu@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: S, Deepak <deepak.s@intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Queued for -next, thanks for the patch.
> > -Daniel
> > 
> 
> Hi,
> 
> while looking at the (semi-generic) force wake code in the function
> gen6_gt_force_wake_get(), I noticed that if the platform doesn't provide
> a low-level forcewake register-access function, then we omit the PM
> get/put calls as well.
> 
> > void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> > {
> >         unsigned long irqflags;
> > 
> >         if (!dev_priv->uncore.funcs.force_wake_get)
> >                 return;
> > 
> >         intel_runtime_pm_get(dev_priv);
> > 
> >         /* Redirect to Gen9 specific routine */
> >         if (IS_GEN9(dev_priv->dev))
> >                 return gen9_force_wake_get(dev_priv, fw_engine);
> > 
> >         /* Redirect to VLV specific routine */
> >         if (IS_VALLEYVIEW(dev_priv->dev))
> >                 return vlv_force_wake_get(dev_priv, fw_engine);
> > 
> >         spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >         if (dev_priv->uncore.forcewake_count++ == 0)
> >                 dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> >         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> 
> Does anyone know whether this is intentional, or just an accident of the
> current implementation? Presumably we don't have any platforms where
> this applies, but we is it plausible that some future device might want
> the runtime PM effects without actually having h/w support
> for forcewake?
> 
> Alternatively, why do we need the runtime PM calls in here? I can see
> Paolo's commit "c8c8fb3 drm/i915: add some runtime PM get/put calls"
> added lots of get/put pairs round various sequences of operations in
> debugfs etc, so why do we also need them at this inner level? Is it just
> to catch any cases where the caller should have (but didn't) deal with
> runtime PM already?

The idea is that power domains nest, and the inner power domains grab the
reference for the outer domains for you.

All the places paulo sprinkled runtime pm over don't need anything else
than the device out of D3, or at least that was the idea. So if you want
to wake the gt we'll automatically get the device out of D3. And on the
display side where power domains can nest even more it'll wake up the
entire chain. Without this the entire abstraction of mapping generic power
domains onto what the hw actually looks like falls apart. And without that
our driver would be a pretty crazy mess ;-)
-Daniel
-- 
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