On 07/11/14 15:46, Ville Syrjälä wrote: > On Wed, Sep 10, 2014 at 07:34:54PM +0100, Chris Wilson wrote: >> Introduce a structure to track the individual forcewake domains and use >> that to eliminated duplicate logic. >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 41 +++--- >> drivers/gpu/drm/i915/i915_drv.h | 32 +++-- >> drivers/gpu/drm/i915/intel_uncore.c | 265 >> ++++++++++++++++-------------------- >> 3 files changed, 157 insertions(+), 181 deletions(-)
Hi Chris, this looks useful -- I was looking at refactoring the VLV vs GEN9 versions of forcewake too. A few comments below: >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index 3b3d3e0..641950b 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -145,7 +145,7 @@ static void __gen6_gt_force_wake_put(struct >> drm_i915_private *dev_priv, >> } >> >> static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv, >> - int fw_engine) >> + int fw_engine) Here and in all the other versions: how about renaming the parameter to "fw_domains" in line with your new enum and struct naming? Thus making it clearer that it can have multiple bits set ... >> @@ -389,50 +358,60 @@ void intel_uncore_sanitize(struct drm_device *dev) >> * be called at the beginning of the sequence followed by a call to >> * gen6_gt_force_wake_put() at the end of the sequence. >> */ >> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int >> fw_engine) >> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, >> + unsigned fw_domains) >> { >> unsigned long irqflags; >> + int i; >> >> if (!dev_priv->uncore.funcs.force_wake_get) >> return; >> >> WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev)); >> >> + fw_domains &= dev_priv->uncore.fw_domains; >> + >> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); >> >> - /* Redirect to VLV specific routine */ >> - if (IS_VALLEYVIEW(dev_priv->dev)) { >> - vlv_force_wake_get(dev_priv, fw_engine); >> - } else { >> - if (dev_priv->uncore.forcewake_count++ == 0) >> - dev_priv->uncore.funcs.force_wake_get(dev_priv, >> - FORCEWAKE_ALL); >> + for (i = 0; i < FW_DOMAIN_COUNT; i++) { >> + if ((fw_domains & (1 << i)) == 0) >> + continue; >> + >> + if (dev_priv->uncore.fw_domain[i].wake_count++) >> + fw_domains &= ~(1 << i); >> } >> + >> + if (fw_domains) >> + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains); >> + >> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); >> } Nice to get rid of the "Redirect to XXX specific routine" stuff -- one of the things I don't like about the current forcewake code is that the VLV and GEN9 versions of the outer wrapper are inconsistent w.r.t. how (and how many times) to call the ->force_wait_get() vfunc -- vlv collects all the bits and makes one call, gen9 makes one call for each bit separately. This code would handle all gens consistently, which is much nicer. And I prefer a single call with multiple bits, so that the low-level code, which knows where the corresponding h/w bits are, can deal efficiently with multiple bits mapping to the same register, or (for example) issue all the forcewake writes first and then poll them all for completion, rather than doing the write/poll pairs sequentially. [snip] > > Also the code in nightly still has the runtime pm stuff in the forcewake > code. I guess you cleaned those out in your tree, but I don't remeber > seeing a recent patch on that front. Yes, this has been another area we wanted clarified and/or tidied up too - I posted a message earlier today asking about whether the PM calls were necessary and a patch that allowed them to be avoided. But if they can be removed completely that would be better still :) > Otherwise this lookd very nice IMO, so would be great to get it in > finally. Agreed - .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx