| Message ID | 1415201452-28754-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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? Thanks, Dave
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
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 68e722b..6a0c3fb 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -194,15 +194,13 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv) static void __vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) { - /* - * WaRsDontPollForAckOnClearingFWBits:vlv - * Hardware clears ack bits lazily (only when all ack - * bits become 0) so don't poll for individiual ack - * bits to be clear here like on other platforms. - */ - /* Check for Render Engine */ if (FORCEWAKE_RENDER & fw_engine) { + if (wait_for_atomic((__raw_i915_read32(dev_priv, + FORCEWAKE_ACK_VLV) & + FORCEWAKE_KERNEL) == 0, + FORCEWAKE_ACK_TIMEOUT_MS)) + DRM_ERROR("Timed out: Render forcewake old ack to clear.\n"); __raw_i915_write32(dev_priv, FORCEWAKE_VLV, _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL)); @@ -216,6 +214,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv, /* Check for Media Engine */ if (FORCEWAKE_MEDIA & fw_engine) { + if (wait_for_atomic((__raw_i915_read32(dev_priv, + FORCEWAKE_ACK_MEDIA_VLV) & + FORCEWAKE_KERNEL) == 0, + FORCEWAKE_ACK_TIMEOUT_MS)) + DRM_ERROR("Timed out: Media forcewake old ack to clear.\n"); __raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV, _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));