| Message ID | 1363972453-2726-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paulo, On Fri, Mar 22, 2013 at 02:14:13PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > It returns true if we've requested to turn the power well on and it's > really on. It also returns true for all the previous gens. > > For now there's just one caller, but I'm going to add more. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Yeah, I've merged this but just stumbled over it again while rebasing the -internal tree. And I'm still unhappy with the name a bit, since power_well is a bit generic. I know it's what bspec uses, but still I'd like to have some notion in it that this is about display stuff The other thing which always irked me is that sprinkling power wells checks all over the place just feels ugly. What we actually want to check is whether the display hw is powered on, which feels much less platform-specific. So what about a s/intel_using_power_well/intel_display_power_enabled? It's not perfect since the actual piece of hw we care about is still platform specific, so I'd suggest to throw an enum on top: enum intel_display_power_domains { POWER_DOMAIN_EDP, POWER_DOMAIN_EDP_PFIT, POWER_DOMAIN_OTHER }; bool intel_display_power_enabled(struct drm_device *dev, enum intel_display_power_domain domain); We could easily add new domains for e.g. the pc8 stuff with a POWER_DOMAIN_CONNECTOR_AUX or so if we need to work around more unclaimed register warnings. With that piece of infrastructure I think I'll stop being grumpy about power wells checks and unclaimed register fixup patches and just merge them all. Comments? Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++++++ > 3 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 188e31f..be70f2d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv, > if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) > state = true; > > - if (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP && > - !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) { > + if (!intel_using_power_well(dev_priv->dev) && > + cpu_transcoder != TRANSCODER_EDP) { > cur_state = false; > } else { > reg = PIPECONF(cpu_transcoder); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e6f84d0..40733d9 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -671,6 +671,7 @@ extern void intel_update_fbc(struct drm_device *dev); > extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); > extern void intel_gpu_ips_teardown(void); > > +extern bool intel_using_power_well(struct drm_device *dev); > extern void intel_init_power_well(struct drm_device *dev); > extern void intel_set_power_well(struct drm_device *dev, bool enable); > extern void intel_enable_gt_powersave(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2de6da6..13404f6 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4086,6 +4086,22 @@ void intel_init_clock_gating(struct drm_device *dev) > dev_priv->display.init_clock_gating(dev); > } > > +/** > + * We should only use the power well if we explicitly asked the hardware to > + * enable it, so check if it's enabled and also check if we've requested it to > + * be enabled. > + */ > +bool intel_using_power_well(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (IS_HASWELL(dev)) > + return I915_READ(HSW_PWR_WELL_DRIVER) == > + (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE); > + else > + return true; > +} > + > void intel_set_power_well(struct drm_device *dev, bool enable) > { > struct drm_i915_private *dev_priv = dev->dev_private; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 17, 2013 at 11:04:23AM +0200, Daniel Vetter wrote: > Hi Paulo, > > On Fri, Mar 22, 2013 at 02:14:13PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > It returns true if we've requested to turn the power well on and it's > > really on. It also returns true for all the previous gens. > > > > For now there's just one caller, but I'm going to add more. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Yeah, I've merged this but just stumbled over it again while rebasing the > -internal tree. And I'm still unhappy with the name a bit, since > power_well is a bit generic. I know it's what bspec uses, but still I'd > like to have some notion in it that this is about display stuff > > The other thing which always irked me is that sprinkling power wells > checks all over the place just feels ugly. What we actually want to check > is whether the display hw is powered on, which feels much less > platform-specific. > > So what about a s/intel_using_power_well/intel_display_power_enabled? It's > not perfect since the actual piece of hw we care about is still platform > specific, so I'd suggest to throw an enum on top: > > enum intel_display_power_domains { > POWER_DOMAIN_EDP, > POWER_DOMAIN_EDP_PFIT, > POWER_DOMAIN_OTHER > }; > > bool intel_display_power_enabled(struct drm_device *dev, > enum intel_display_power_domain domain); > > We could easily add new domains for e.g. the pc8 stuff with a > POWER_DOMAIN_CONNECTOR_AUX or so if we need to work around more unclaimed > register warnings. > > With that piece of infrastructure I think I'll stop being grumpy about > power wells checks and unclaimed register fixup patches and just merge > them all. Also, that function should probably use HAS_POWER_WELL instead of the manual IS_HASWELL check. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 188e31f..be70f2d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv, if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) state = true; - if (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP && - !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) { + if (!intel_using_power_well(dev_priv->dev) && + cpu_transcoder != TRANSCODER_EDP) { cur_state = false; } else { reg = PIPECONF(cpu_transcoder); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e6f84d0..40733d9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -671,6 +671,7 @@ extern void intel_update_fbc(struct drm_device *dev); extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); extern void intel_gpu_ips_teardown(void); +extern bool intel_using_power_well(struct drm_device *dev); extern void intel_init_power_well(struct drm_device *dev); extern void intel_set_power_well(struct drm_device *dev, bool enable); extern void intel_enable_gt_powersave(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2de6da6..13404f6 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4086,6 +4086,22 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating(dev); } +/** + * We should only use the power well if we explicitly asked the hardware to + * enable it, so check if it's enabled and also check if we've requested it to + * be enabled. + */ +bool intel_using_power_well(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + if (IS_HASWELL(dev)) + return I915_READ(HSW_PWR_WELL_DRIVER) == + (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE); + else + return true; +} + void intel_set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private;