| Message ID | 1393540010-1582-8-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > To solve a chicken-and-egg problem. Currently when we get/put > forcewake we also get/put runtime PM and this works fine because the > runtime PM code doesn't need forcewake. But we're going to merge PC8 > and runtime PM into a single feature, and the PC8 code (the LCPLL > code) does need the forcewake, so that specific piece of code needs to > call the _no_rpm version so it doesn't try to get runtime PM in the > code that gets runtime PM. > > For now the new functions are unused, we'll use them on the patch that > merges PC8 with runtime PM. > > Also notice that, so simplify things, the put() function doesn't use > the workqueue, since the workqueue also puts runtime PM. Where are these routines called? The names are awful but not quite awful enough to avoid confusion. Is it possible to hide these to a single .c? The workqueue doesn't touch rpm here, so there routines could be simplified if they remain in intel_uncore.c. -Chris
2014-02-28 5:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> To solve a chicken-and-egg problem. Currently when we get/put >> forcewake we also get/put runtime PM and this works fine because the >> runtime PM code doesn't need forcewake. But we're going to merge PC8 >> and runtime PM into a single feature, and the PC8 code (the LCPLL >> code) does need the forcewake, so that specific piece of code needs to >> call the _no_rpm version so it doesn't try to get runtime PM in the >> code that gets runtime PM. >> >> For now the new functions are unused, we'll use them on the patch that >> merges PC8 with runtime PM. >> >> Also notice that, so simplify things, the put() function doesn't use >> the workqueue, since the workqueue also puts runtime PM. > > Where are these routines called? The names are awful but not quite awful > enough to avoid confusion. Is it possible to hide these to a single .c? They are called on patch 09/23, inside function hsw_restore_lcpll(), from intel_display.c. I'm always open to naming suggestions. I could try gen6_gt_force_wake_get_no_runtime_pm. I could also just try to put all this code inline in the caller, but IMHO that wouldn't be an improvement over this. Another thing worth mentioning, is that at hsw_restore_lcpll we expect the forcewake count to be always zero. So we could do some other kind of trick relying on that, but I don't think it's very future-proof. > > The workqueue doesn't touch rpm here, so there routines could be > simplified if they remain in intel_uncore.c. I don't understand what you mean with the sentence above. Could you please clarify? Thanks for the review! Paulo > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Feb 28, 2014 at 04:38:28PM -0300, Paulo Zanoni wrote: > Another thing worth mentioning, is that at hsw_restore_lcpll we expect > the forcewake count to be always zero. So we could do some other kind > of trick relying on that, but I don't think it's very future-proof. Actually, having read how these new functions were used, I have further doubts that they are being used correctly. I think adding comments to the callsites should answer most of my questions, or help show the weakness in the current code. > 2014-02-28 5:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > The workqueue doesn't touch rpm here, so there routines could be > > simplified if they remain in intel_uncore.c. > > I don't understand what you mean with the sentence above. Could you > please clarify? I was reading the patches out of order, so hadn't seen the addition of the rpm into the workqueue. -Chris
On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > To solve a chicken-and-egg problem. Currently when we get/put > forcewake we also get/put runtime PM and this works fine because the > runtime PM code doesn't need forcewake. But we're going to merge PC8 > and runtime PM into a single feature, and the PC8 code (the LCPLL > code) does need the forcewake, so that specific piece of code needs to > call the _no_rpm version so it doesn't try to get runtime PM in the > code that gets runtime PM. > > For now the new functions are unused, we'll use them on the patch that > merges PC8 with runtime PM. > > Also notice that, so simplify things, the put() function doesn't use > the workqueue, since the workqueue also puts runtime PM. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> See my previous mail, but I'm still freaked out about this. Doing runtime pm hidden deep down in our register I/O functions is imo rather deeply troubling ... I'll punt for now on this. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2a2a3a9..dcaa130 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2604,6 +2604,10 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, */ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine); void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine); +void gen6_gt_force_wake_get_no_rpm(struct drm_i915_private *dev_priv, + int fw_engine); +void gen6_gt_force_wake_put_no_rpm(struct drm_i915_private *dev_priv, + int fw_engine); int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val); int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 6a21f43..d0ec32a 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -422,6 +422,46 @@ out: intel_runtime_pm_put(dev_priv); } +/* + * These two _no_rpm functions below should only be used by the code that runs + * while we are enabling/disabling runtiem PM. See gen6_gt_force_wake_get(). + */ +void gen6_gt_force_wake_get_no_rpm(struct drm_i915_private *dev_priv, + int fw_engine) +{ + unsigned long irqflags; + + if (!dev_priv->uncore.funcs.force_wake_get) + return; + + /* 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); +} + +void gen6_gt_force_wake_put_no_rpm(struct drm_i915_private *dev_priv, + int fw_engine) +{ + unsigned long irqflags; + + if (!dev_priv->uncore.funcs.force_wake_put) + return; + + /* Redirect to VLV specific routine */ + if (IS_VALLEYVIEW(dev_priv->dev)) + return vlv_force_wake_put(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_put(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); +} + /* We give fast paths for the really cool registers */ #define NEEDS_FORCE_WAKE(dev_priv, reg) \ ((reg) < 0x40000 && (reg) != FORCEWAKE)