| Message ID | 1387461309-2756-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2013-12-19 at 11:54 -0200, 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. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/intel_uncore.c | 30 ++++++++++++++++++++---------- > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index cc8afff..7f8ec08 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2533,6 +2533,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 646fecf..6118d2c 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -368,15 +368,11 @@ 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_no_rpm(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 VLV specific routine */ > if (IS_VALLEYVIEW(dev_priv->dev)) > return vlv_force_wake_get(dev_priv, fw_engine); > @@ -387,16 +383,23 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > > +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) > +{ > + if (!dev_priv->uncore.funcs.force_wake_get) > + return; > + > + intel_runtime_pm_get(dev_priv); > + gen6_gt_force_wake_get_no_rpm(dev_priv, fw_engine); > +} > + > /* > * see gen6_gt_force_wake_get() > */ > -void gen6_gt_force_wake_put(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) > { > 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); > @@ -410,7 +413,14 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) > 1); > } > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > +} > + > +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) > +{ > + if (!dev_priv->uncore.funcs.force_wake_put) > + return; > > + gen6_gt_force_wake_put_no_rpm(dev_priv, fw_engine); > intel_runtime_pm_put(dev_priv); > } >
Hi I'm sorry, I forgot to say. This series is quite old, and I changed it a little bit since then (since I found one or two problems), including this patch. I think that, to avoid wasting your time reviewing old patches, I should resend the new series. The problem is that this series should be on top of the 11 patches I recently sent (with PC8/runtime PM fixes), so if we could review those first, it would be better. We also need to decide the relative order of merging your recent series and these patches, because they have some conflicts. Thanks, Paulo 2014-02-27 11:43 GMT-03:00 Imre Deak <imre.deak@intel.com>: > On Thu, 2013-12-19 at 11:54 -0200, 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. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Reviewed-by: Imre Deak <imre.deak@intel.com> > >> --- >> drivers/gpu/drm/i915/i915_drv.h | 4 ++++ >> drivers/gpu/drm/i915/intel_uncore.c | 30 ++++++++++++++++++++---------- >> 2 files changed, 24 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index cc8afff..7f8ec08 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2533,6 +2533,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 646fecf..6118d2c 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -368,15 +368,11 @@ 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_no_rpm(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 VLV specific routine */ >> if (IS_VALLEYVIEW(dev_priv->dev)) >> return vlv_force_wake_get(dev_priv, fw_engine); >> @@ -387,16 +383,23 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) >> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); >> } >> >> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) >> +{ >> + if (!dev_priv->uncore.funcs.force_wake_get) >> + return; >> + >> + intel_runtime_pm_get(dev_priv); >> + gen6_gt_force_wake_get_no_rpm(dev_priv, fw_engine); >> +} >> + >> /* >> * see gen6_gt_force_wake_get() >> */ >> -void gen6_gt_force_wake_put(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) >> { >> 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); >> @@ -410,7 +413,14 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) >> 1); >> } >> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); >> +} >> + >> +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) >> +{ >> + if (!dev_priv->uncore.funcs.force_wake_put) >> + return; >> >> + gen6_gt_force_wake_put_no_rpm(dev_priv, fw_engine); >> intel_runtime_pm_put(dev_priv); >> } >> >
On Thu, 2014-02-27 at 11:48 -0300, Paulo Zanoni wrote: > Hi > > I'm sorry, I forgot to say. This series is quite old, and I changed it > a little bit since then (since I found one or two problems), including > this patch. I think that, to avoid wasting your time reviewing old > patches, I should resend the new series. Ah, ok. Np, I can re-check the updated version once you submitted it. > The problem is that this series should be on top of the 11 patches I > recently sent (with PC8/runtime PM fixes), so if we could review those > first, it would be better. We also need to decide the relative order > of merging your recent series and these patches, because they have > some conflicts. Wrt. "Make PC8 become part of runtime PM" and "vlv power domains support", I'd merge first the latter to reduce code churn, since you're adding the RPM get/put calls on the power domain enable/disable path which has changed somewhat after my patchset. As we agreed I'm ok if we merge "PC8/runtime PM fixes" before the power domains patchset, I can rebase my patchset on top. --Imre
On Thu, Feb 27, 2014 at 11:48:30AM -0300, Paulo Zanoni wrote: > Hi > > I'm sorry, I forgot to say. This series is quite old, and I changed it > a little bit since then (since I found one or two problems), including > this patch. I think that, to avoid wasting your time reviewing old > patches, I should resend the new series. > > The problem is that this series should be on top of the 11 patches I > recently sent (with PC8/runtime PM fixes), so if we could review those > first, it would be better. We also need to decide the relative order > of merging your recent series and these patches, because they have > some conflicts. Hm, I've merged the first two patches of this series already, hope that doesn't cause a fuzz ;-) Now looking closer I'm puzzled by the pm_get/put calls in the forcewake get/put functions. Imo any place which needs to have a power well up and runtime (I include runtime pm as the overall power well here) should grab a runtime pm reference for the entire access, not each register cycle. The patch which added this was quite old, so probably before we've fixed the bunch of runtime pm issues around gem batch buffers. And it was part of a big patch which added get/puts mostly over debugfs files and similar places. So I wonder whether we really still need this? I'd much prefer if we could remove it, and if that's not possible fix up the (hopefully few) places where we currently don't grab a runtime pm ref, but should. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index cc8afff..7f8ec08 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2533,6 +2533,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 646fecf..6118d2c 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -368,15 +368,11 @@ 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_no_rpm(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 VLV specific routine */ if (IS_VALLEYVIEW(dev_priv->dev)) return vlv_force_wake_get(dev_priv, fw_engine); @@ -387,16 +383,23 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); } +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) +{ + if (!dev_priv->uncore.funcs.force_wake_get) + return; + + intel_runtime_pm_get(dev_priv); + gen6_gt_force_wake_get_no_rpm(dev_priv, fw_engine); +} + /* * see gen6_gt_force_wake_get() */ -void gen6_gt_force_wake_put(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) { 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); @@ -410,7 +413,14 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) 1); } spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); +} + +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) +{ + if (!dev_priv->uncore.funcs.force_wake_put) + return; + gen6_gt_force_wake_put_no_rpm(dev_priv, fw_engine); intel_runtime_pm_put(dev_priv); }