diff mbox

[1/2] drm/i915: Send a DPIO cmnreset during driver load or system resume.

Message ID 1380006867-20771-1-git-send-email-chon.ming.lee@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chon Ming Lee Sept. 24, 2013, 7:14 a.m. UTC 
Without the DPIO cmnreset, the PLL fail to lock.  This should have
done by BIOS.

v2: Move this to intel_uncore_sanitize to allow it to get call during
resume path. (Daniel)
v3: Remove redundant write 0 to DPIO_CTL, and use DPIO_RESET instead of
just 0x1 (Ville)
    Without BIOS, DPIO/render well/media well may still power gated.
Turn it off.

Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |    9 +++++++++
 drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

Comments

Ville Syrjälä Sept. 24, 2013, 3:18 p.m. UTC | #1 
On Tue, Sep 24, 2013 at 03:14:27PM +0800, Chon Ming Lee wrote:
> Without the DPIO cmnreset, the PLL fail to lock.  This should have
> done by BIOS.
> 
> v2: Move this to intel_uncore_sanitize to allow it to get call during
> resume path. (Daniel)
> v3: Remove redundant write 0 to DPIO_CTL, and use DPIO_RESET instead of
> just 0x1 (Ville)
>     Without BIOS, DPIO/render well/media well may still power gated.
> Turn it off.
> 
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |    9 +++++++++
>  drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c4f9bef..c02f893 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -361,6 +361,15 @@
>  #define PUNIT_OPCODE_REG_READ			6
>  #define PUNIT_OPCODE_REG_WRITE			7
>  
> +#define PUNIT_REG_PWRGT_CTRL			0x60
> +#define PUNIT_REG_PWRGT_STATUS			0x61
> +#define	  PUNIT_CLK_GATE			1
> +#define	  PUNIT_PWR_RESET			2
> +#define	  PUNIT_PWR_GATE			3
> +#define	  RENDER_PWRGT				(PUNIT_PWR_GATE << 0)
> +#define	  MEDIA_PWRGT				(PUNIT_PWR_GATE << 2)
> +#define	  DPIO_PWRGT				(PUNIT_PWR_GATE << 6)

Subsys 6 seems to be one of four TX lanes, and there's also the common
lane subsys, and the disp2d is one as well. RX supposedly is not relevant
for display PHY, not sure why it has subsys allocated too.

Anyways my point would be that shouldn't we check all subsys ie render + media +
disp2d + common lane + all tx lanes?

And should we maybe power gate the RX lanes always as those shouldn't be needed
for display?

> +
>  #define PUNIT_REG_GPU_LFM			0xd3
>  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
>  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8649f1c..6923b4d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -276,10 +276,33 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
>  
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 reg_val;
> +
>  	intel_uncore_forcewake_reset(dev);
>  
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
>  	intel_disable_gt_powersave(dev);
> +
> +	/* Trigger DPIO CMN RESET and turn off power gate, require
> +	 * especially in BIOS less system
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +
> +		mutex_lock(&dev_priv->rps.hw_lock);
> +		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
> +
> +		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DPIO_PWRGT))
> +			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
> +
> +		mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +		reg_val = I915_READ(DPIO_CTL);
> +		if (!(reg_val & DPIO_RESET)) {
> +			I915_WRITE(DPIO_CTL, DPIO_RESET);
> +			POSTING_READ(DPIO_CTL);
> +		}
> +	}
>  }
>  
>  /*
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chon Ming Lee Sept. 24, 2013, 11:14 p.m. UTC | #2 
On 09/24 18:18, Ville Syrjälä wrote:
> On Tue, Sep 24, 2013 at 03:14:27PM +0800, Chon Ming Lee wrote:
> > Without the DPIO cmnreset, the PLL fail to lock.  This should have
> > done by BIOS.
> > 
> > v2: Move this to intel_uncore_sanitize to allow it to get call during
> > resume path. (Daniel)
> > v3: Remove redundant write 0 to DPIO_CTL, and use DPIO_RESET instead of
> > just 0x1 (Ville)
> >     Without BIOS, DPIO/render well/media well may still power gated.
> > Turn it off.
> > 
> > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h     |    9 +++++++++
> >  drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++++++++++++++
> >  2 files changed, 32 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c4f9bef..c02f893 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -361,6 +361,15 @@
> >  #define PUNIT_OPCODE_REG_READ			6
> >  #define PUNIT_OPCODE_REG_WRITE			7
> >  
> > +#define PUNIT_REG_PWRGT_CTRL			0x60
> > +#define PUNIT_REG_PWRGT_STATUS			0x61
> > +#define	  PUNIT_CLK_GATE			1
> > +#define	  PUNIT_PWR_RESET			2
> > +#define	  PUNIT_PWR_GATE			3
> > +#define	  RENDER_PWRGT				(PUNIT_PWR_GATE << 0)
> > +#define	  MEDIA_PWRGT				(PUNIT_PWR_GATE << 2)
> > +#define	  DPIO_PWRGT				(PUNIT_PWR_GATE << 6)
> 
> Subsys 6 seems to be one of four TX lanes, and there's also the common
> lane subsys, and the disp2d is one as well. RX supposedly is not relevant
> for display PHY, not sure why it has subsys allocated too.
> 
> Anyways my point would be that shouldn't we check all subsys ie render + media +
> disp2d + common lane + all tx lanes?
> 
By default, the common lane + all tx lanes are not power gated during cold boot
or system resume.  Unless S0ix entry actually power gate it by driver, then,
this will need to add to turn off it.  

> And should we maybe power gate the RX lanes always as those shouldn't be needed
> for display?

Yes, you are correct.  I believe there should be another patch to do it, to
enable power gate the VLV correctly for SOix entry or exit.  
> 
> > +
> >  #define PUNIT_REG_GPU_LFM			0xd3
> >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8649f1c..6923b4d 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -276,10 +276,33 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
> >  
> >  void intel_uncore_sanitize(struct drm_device *dev)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u32 reg_val;
> > +
> >  	intel_uncore_forcewake_reset(dev);
> >  
> >  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> >  	intel_disable_gt_powersave(dev);
> > +
> > +	/* Trigger DPIO CMN RESET and turn off power gate, require
> > +	 * especially in BIOS less system
> > +	 */
> > +	if (IS_VALLEYVIEW(dev)) {
> > +
> > +		mutex_lock(&dev_priv->rps.hw_lock);
> > +		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
> > +
> > +		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DPIO_PWRGT))
> > +			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
> > +
> > +		mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +		reg_val = I915_READ(DPIO_CTL);
> > +		if (!(reg_val & DPIO_RESET)) {
> > +			I915_WRITE(DPIO_CTL, DPIO_RESET);
> > +			POSTING_READ(DPIO_CTL);
> > +		}
> > +	}
> >  }
> >  
> >  /*
> > -- 
> > 1.7.7.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
Ville Syrjälä Sept. 25, 2013, 7:25 a.m. UTC | #3 
On Wed, Sep 25, 2013 at 07:14:34AM +0800, Lee, Chon Ming wrote:
> On 09/24 18:18, Ville Syrjälä wrote:
> > On Tue, Sep 24, 2013 at 03:14:27PM +0800, Chon Ming Lee wrote:
> > > Without the DPIO cmnreset, the PLL fail to lock.  This should have
> > > done by BIOS.
> > > 
> > > v2: Move this to intel_uncore_sanitize to allow it to get call during
> > > resume path. (Daniel)
> > > v3: Remove redundant write 0 to DPIO_CTL, and use DPIO_RESET instead of
> > > just 0x1 (Ville)
> > >     Without BIOS, DPIO/render well/media well may still power gated.
> > > Turn it off.
> > > 
> > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h     |    9 +++++++++
> > >  drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++++++++++++++
> > >  2 files changed, 32 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index c4f9bef..c02f893 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -361,6 +361,15 @@
> > >  #define PUNIT_OPCODE_REG_READ			6
> > >  #define PUNIT_OPCODE_REG_WRITE			7
> > >  
> > > +#define PUNIT_REG_PWRGT_CTRL			0x60
> > > +#define PUNIT_REG_PWRGT_STATUS			0x61
> > > +#define	  PUNIT_CLK_GATE			1
> > > +#define	  PUNIT_PWR_RESET			2
> > > +#define	  PUNIT_PWR_GATE			3
> > > +#define	  RENDER_PWRGT				(PUNIT_PWR_GATE << 0)
> > > +#define	  MEDIA_PWRGT				(PUNIT_PWR_GATE << 2)
> > > +#define	  DPIO_PWRGT				(PUNIT_PWR_GATE << 6)
> > 
> > Subsys 6 seems to be one of four TX lanes, and there's also the common
> > lane subsys, and the disp2d is one as well. RX supposedly is not relevant
> > for display PHY, not sure why it has subsys allocated too.
> > 
> > Anyways my point would be that shouldn't we check all subsys ie render + media +
> > disp2d + common lane + all tx lanes?
> > 
> By default, the common lane + all tx lanes are not power gated during cold boot
> or system resume.  Unless S0ix entry actually power gate it by driver, then,
> this will need to add to turn off it.  

OK. And as Imre pointed out to me the '<< 6' isn't a TX lane as I
claimed but the display subsystems (3). I assume that's the same thing
as the disp2d block, ie. the pipes. So the DPIO in the name is
wrong. It should be called display or disp2d I think.

If you say the PHY side isn't power gated during cold boot, I think we
can ignore it for now.

So if you rename the DPIO thing, this patch should be OK.

> 
> > And should we maybe power gate the RX lanes always as those shouldn't be needed
> > for display?
> 
> Yes, you are correct.  I believe there should be another patch to do it, to
> enable power gate the VLV correctly for SOix entry or exit.  

My plan is to power gate everything we can during runtime, not just s0ix.
But that's a biger topic we can discuss once Imre gets some relevant
patches ready.

> > 
> > > +
> > >  #define PUNIT_REG_GPU_LFM			0xd3
> > >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> > >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 8649f1c..6923b4d 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -276,10 +276,33 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
> > >  
> > >  void intel_uncore_sanitize(struct drm_device *dev)
> > >  {
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	u32 reg_val;
> > > +
> > >  	intel_uncore_forcewake_reset(dev);
> > >  
> > >  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> > >  	intel_disable_gt_powersave(dev);
> > > +
> > > +	/* Trigger DPIO CMN RESET and turn off power gate, require
> > > +	 * especially in BIOS less system
> > > +	 */
> > > +	if (IS_VALLEYVIEW(dev)) {
> > > +
> > > +		mutex_lock(&dev_priv->rps.hw_lock);
> > > +		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
> > > +
> > > +		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DPIO_PWRGT))
> > > +			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
> > > +
> > > +		mutex_unlock(&dev_priv->rps.hw_lock);
> > > +
> > > +		reg_val = I915_READ(DPIO_CTL);
> > > +		if (!(reg_val & DPIO_RESET)) {
> > > +			I915_WRITE(DPIO_CTL, DPIO_RESET);
> > > +			POSTING_READ(DPIO_CTL);
> > > +		}
> > > +	}
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 1.7.7.6
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c4f9bef..c02f893 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -361,6 +361,15 @@ 
 #define PUNIT_OPCODE_REG_READ			6
 #define PUNIT_OPCODE_REG_WRITE			7
 
+#define PUNIT_REG_PWRGT_CTRL			0x60
+#define PUNIT_REG_PWRGT_STATUS			0x61
+#define	  PUNIT_CLK_GATE			1
+#define	  PUNIT_PWR_RESET			2
+#define	  PUNIT_PWR_GATE			3
+#define	  RENDER_PWRGT				(PUNIT_PWR_GATE << 0)
+#define	  MEDIA_PWRGT				(PUNIT_PWR_GATE << 2)
+#define	  DPIO_PWRGT				(PUNIT_PWR_GATE << 6)
+
 #define PUNIT_REG_GPU_LFM			0xd3
 #define PUNIT_REG_GPU_FREQ_REQ			0xd4
 #define PUNIT_REG_GPU_FREQ_STS			0xd8
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8649f1c..6923b4d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -276,10 +276,33 @@  static void intel_uncore_forcewake_reset(struct drm_device *dev)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg_val;
+
 	intel_uncore_forcewake_reset(dev);
 
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
+
+	/* Trigger DPIO CMN RESET and turn off power gate, require
+	 * especially in BIOS less system
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+
+		mutex_lock(&dev_priv->rps.hw_lock);
+		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
+
+		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DPIO_PWRGT))
+			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
+
+		mutex_unlock(&dev_priv->rps.hw_lock);
+
+		reg_val = I915_READ(DPIO_CTL);
+		if (!(reg_val & DPIO_RESET)) {
+			I915_WRITE(DPIO_CTL, DPIO_RESET);
+			POSTING_READ(DPIO_CTL);
+		}
+	}
 }
 
 /*