Re: [SKL-DMC-BUGFIX 3/5] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Imre Deak <imre.deak@intel.com>]

On ma, 2015-10-12 at 16:46 +0200, Patrik Jakobsson wrote:
> On Mon, Oct 12, 2015 at 05:07:13PM +0300, Imre Deak wrote:
> > On ma, 2015-10-12 at 16:37 +0300, Imre Deak wrote:
> > > On ma, 2015-08-03 at 21:55 +0530, Animesh Manna wrote:
> > > > While display engine entering into low power state no need to disable
> > > > cdclk pll as CSR firmware of dmc will take care. If pll is already
> > > > enabled firmware execution sequence will be blocked. This is one
> > > > of the criteria for dmc to work properly.
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
> > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index af0bcfe..ef2ef4d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > > >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> > > >  		DRM_ERROR("DBuf power disable timeout\n");
> > > 
> > > My understanding is that DBUF_CTL is also handled by the firmware and so
> > > we shouldn't need to disable it either manually. I guess that could be
> > > addressed as a follow-up.
> > > 
> > > >  
> > > > -	/* disable DPLL0 */
> > > > -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> > > > -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > > -		DRM_ERROR("Couldn't disable DPLL0\n");
> > > > +	if (dev_priv->csr.dmc_payload) {
> > > > +		/* disable DPLL0 */
> > > > +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> > > > +					~LCPLL_PLL_ENABLE);
> > > > +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> > > > +			DRM_ERROR("Couldn't disable DPLL0\n");
> > > > +	}
> > > >  
> > > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > 
> > Not introduced in this patch, but the above put looks incorrect. We get
> > here on the runtime suspend path, where all RPM and hence display power
> > domain references should be dropped already. So not sure how this can
> > even work atm. This is for someone to look into as a follow-up.
> 
> Hmm, I thought that was fixed already. This seems to be the last comment on
> Paulos attempt at fixing it:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/073122.html

Yes, that would solve this issue. One note about it is that we only want
to manually toggle PW1 and Misc IO if DC6 is disabled (via a module
option for example). And if so, toggling of PW1 and Misc IO would be
part of the bigger
"Sequence for Software to Allow/Disallow Package C9-C10".

> > > >  }
> > > 
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx