[PATCH 03/10] drm/i915: fix intel_init_power_wells

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 21, 2013 at 03:37:42PM +0200, Ville Syrj?l? wrote:
> On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > 
> > The current code was wrong in many different ways, so this is a full
> > rewrite. We don't have "different power wells for different parts of
> > the GPU", we have a single power well, but we have multiple registers
> > that can be used to request enabling/disabling the power well. So
> > let's be a good citizen and only use the register we're supposed to
> > use, except when we're loading the driver, where we clear the request
> > made by the BIOS.
> > 
> > If any of the registers is requesting the power well to be enabled, it
> > will be enabled. If none of the registers is requesting the power well
> > to be enabled, it will be disabled.
> > 
> > For now we're just forcing the power well to be enabled, but in the
> > next commits we'll change this.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
> >  drivers/gpu/drm/i915/intel_display.c |    5 +--
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
> >  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
> >  4 files changed, 59 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 2521617..f054554 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4414,10 +4414,10 @@
> >  #define   AUDIO_CP_READY_C		(1<<9)
> >  
> >  /* HSW Power Wells */
> > -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> > -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> > -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> > -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> > +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> > +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> > +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> > +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
> >  #define   HSW_PWR_WELL_ENABLE			(1<<31)
> >  #define   HSW_PWR_WELL_STATE			(1<<30)
> >  #define HSW_PWR_WELL_CTL5			0x45410
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b35902e..4a9f048 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
> >  
> >  void intel_modeset_init_hw(struct drm_device *dev)
> >  {
> > -	/* We attempt to init the necessary power wells early in the initialization
> > -	 * time, so the subsystems that expect power to be enabled can work.
> > -	 */
> > -	intel_init_power_wells(dev);
> > +	intel_init_power_well(dev);
> >  
> >  	intel_prepare_ddi(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index aeff0d1..8cfad75 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -666,7 +666,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 void intel_init_power_wells(struct drm_device *dev);
> > +extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> >  extern void intel_disable_gt_powersave(struct drm_device *dev);
> >  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5a8a72c..2273b9c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
> >  	dev_priv->display.init_clock_gating(dev);
> >  }
> >  
> > -/* Starting with Haswell, we have different power wells for
> > - * different parts of the GPU. This attempts to enable them all.
> > +static void intel_set_power_well(struct drm_device *dev, bool enable)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	bool is_enabled, enable_requested;
> > +	uint32_t tmp;
> > +
> > +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> > +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> > +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> > +
> > +	if (enable) {
> > +		if (!enable_requested)
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> > +
> > +		if (!is_enabled) {
> > +			DRM_DEBUG_KMS("Enabling power well\n");
> > +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > +				      HSW_PWR_WELL_STATE), 20))
> > +				DRM_ERROR("Timeout enabling power well\n");
> > +		}
> > +	} else {
> > +		if (enable_requested)
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > +
> > +		if (is_enabled) {
> > +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> > +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> > +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> > +			else {
> > +				DRM_DEBUG_KMS("Disabling power well\n");
> > +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > +					      HSW_PWR_WELL_STATE) == 0, 20))
> > +					DRM_ERROR("Timeout disabling power well\n");
> > +			}
> > +		}
> > +	}
> 
> The documentation says not to touch the enable bits if there's a state
> transition already in progress. I have no idea how we're supposed to do
> that without all kinds of race conditions due multiple entities making
> requests simultaneosly. Do you know something about this that's not in
> the documentation?
> 
> Apart from that the patch looks good to me.

We should be protected by dev->mode_config.mutex in all callsite (safe for
maybe setup/resume, but that's just cosmetics). The dev->struct_mutex
locking we already have seems to be cargo-culting afaict. Paulo, can you
add a follow-up patch to unconfuse matters here a bit?

Thanks, Daniel
> 
> > +}
> > +
> > +/*
> > + * Starting with Haswell, we have a "Power Down Well" that can be turned off
> > + * when not needed anymore. We have 4 registers that can request the power well
> > + * to be enabled, and it will only be disabled if none of the registers is
> > + * requesting it to be enabled.
> >   */
> > -void intel_init_power_wells(struct drm_device *dev)
> > +void intel_init_power_well(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	unsigned long power_wells[] = {
> > -		HSW_PWR_WELL_CTL1,
> > -		HSW_PWR_WELL_CTL2,
> > -		HSW_PWR_WELL_CTL4
> > -	};
> > -	int i;
> >  
> >  	if (!IS_HASWELL(dev))
> >  		return;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> >  
> > -	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
> > -		int well = I915_READ(power_wells[i]);
> > +	/* For now, we need the power well to be always enabled. */
> > +	intel_set_power_well(dev, true);
> >  
> > -		if ((well & HSW_PWR_WELL_STATE) == 0) {
> > -			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
> > -			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
> > -				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
> > -		}
> > -	}
> > +	/* We're taking over the BIOS, so clear any requests made by it since
> > +	 * the driver is in charge now. */
> > +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> > +		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> >  
> >  	mutex_unlock(&dev->struct_mutex);
> >  }
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrj?l?
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux