| Message ID | 1362611003-4823-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > It returns true if we're not supposed to touch the registers on the > power down well. > > For now there's just one caller, but I'm going to add more. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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 502cb28..bd27336 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_power_well_is_down(dev_priv->dev) && The name here feels a bit too generic given that we already have on hsw different display c states with different requirements and different pieces of hw not working. Can't thinkg of anything better than intel_display_power_well_is_down though ... -Daniel > + 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 010e998..28c4789 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_power_well_is_down(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 5479363..90562bc 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev) > dev_priv->display.init_clock_gating(dev); > } > > +/** > + * Returns true if we're not supposed to touch the registers on the power down > + * well. Notice that we don't check whether the power well is actually off, we > + * just check if our driver told the hardware that it doesn't need the power > + * well enabled. > + */ > +bool intel_power_well_is_down(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); > + else > + return false; > +} > + > 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 Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote: > On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > It returns true if we're not supposed to touch the registers on the > > power down well. > > > > For now there's just one caller, but I'm going to add more. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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 502cb28..bd27336 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_power_well_is_down(dev_priv->dev) && > > The name here feels a bit too generic given that we already have on hsw > different display c states with different requirements and different > pieces of hw not working. > > Can't thinkg of anything better than intel_display_power_well_is_down > though ... Also maybe make the function assure that the power well is on i.e. power_well_enabled to avoid double-negation logic in a bunch of places. -Daniel
On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote: > On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > It returns true if we're not supposed to touch the registers on the > > power down well. > > > > For now there's just one caller, but I'm going to add more. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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 502cb28..bd27336 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_power_well_is_down(dev_priv->dev) && > > The name here feels a bit too generic given that we already have on hsw > different display c states with different requirements and different > pieces of hw not working. > > Can't thinkg of anything better than intel_display_power_well_is_down > though ... > -Daniel > > > + 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 010e998..28c4789 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_power_well_is_down(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 5479363..90562bc 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev) > > dev_priv->display.init_clock_gating(dev); > > } > > > > +/** > > + * Returns true if we're not supposed to touch the registers on the power down > > + * well. Notice that we don't check whether the power well is actually off, we > > + * just check if our driver told the hardware that it doesn't need the power > > + * well enabled. > > + */ I agree with Denial that the name sucks because your comment clearly contradicts what the function is actually called. Can't think of anything better either. In the bikeshed realm, I think it makes more sense to do the IS_HASWELL check in your pipe assertion, but I'll assume that you have a good usage as you mention later. Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > +bool intel_power_well_is_down(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); > > + else > > + return false; > > +} > > + > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi 2013/3/15 Ben Widawsky <ben@bwidawsk.net>: > On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote: >> On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote: >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > It returns true if we're not supposed to touch the registers on the >> > power down well. >> > >> > For now there's just one caller, but I'm going to add more. >> > >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > --- >> > 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 502cb28..bd27336 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_power_well_is_down(dev_priv->dev) && >> >> The name here feels a bit too generic given that we already have on hsw >> different display c states with different requirements and different >> pieces of hw not working. >> >> Can't thinkg of anything better than intel_display_power_well_is_down >> though ... >> -Daniel >> >> > + 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 010e998..28c4789 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_power_well_is_down(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 5479363..90562bc 100644 >> > --- a/drivers/gpu/drm/i915/intel_pm.c >> > +++ b/drivers/gpu/drm/i915/intel_pm.c >> > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev) >> > dev_priv->display.init_clock_gating(dev); >> > } >> > >> > +/** >> > + * Returns true if we're not supposed to touch the registers on the power down >> > + * well. Notice that we don't check whether the power well is actually off, we >> > + * just check if our driver told the hardware that it doesn't need the power >> > + * well enabled. >> > + */ > > I agree with Denial that the name sucks because your comment clearly > contradicts what the function is actually called. Can't think of > anything better either. intel_using_power_well? > > In the bikeshed realm, I think it makes more sense to do the IS_HASWELL > check in your pipe assertion, but I'll assume that you have a good usage > as you mention later. I want to make all the power well functions work correctly on Gens that don't have a power well. We already introduced bugs related to this in the past, so let's be defensive. > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > >> > +bool intel_power_well_is_down(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); >> > + else >> > + return false; >> > +} >> > + >> > 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 >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ben Widawsky, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 502cb28..bd27336 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_power_well_is_down(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 010e998..28c4789 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_power_well_is_down(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 5479363..90562bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating(dev); } +/** + * Returns true if we're not supposed to touch the registers on the power down + * well. Notice that we don't check whether the power well is actually off, we + * just check if our driver told the hardware that it doesn't need the power + * well enabled. + */ +bool intel_power_well_is_down(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); + else + return false; +} + void intel_set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private;