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

[Dave Gordon <david.s.gordon@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx