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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC