Re: [PATCH 09/13] drm/i915/cfl: Basic PM plumbing for Coffee Lake.

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 09/13] drm/i915/cfl: Basic PM plumbing for Coffee Lake.
Date: Fri, 2 Jun 2017 21:31:59 +0000	[thread overview]
Message-ID: <1496440249.6959.24.camel@dk-H97M-D3H> (raw)
In-Reply-To: <1496439852.6959.19.camel@dk-H97M-D3H>

On Fri, 2017-06-02 at 21:25 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2017-05-30 at 15:42 -0700, Rodrigo Vivi wrote:
> > All here is pretty much like Kabylake, expect the PCH.
> > 
> > This patch exclude the addition of DMC, GuC and most workardounds since
> > they might have changes/updates.
> > 
> > v2: Take advantage of IS_GEN9_BC minimizing the needed plumbing.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 6 +++---
> >  drivers/gpu/drm/i915/intel_pm.c  | 6 +++---
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 0914ad9..e364814 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> 
> DDI changes look like they should be a separate patch.  
> 
> > @@ -429,7 +429,7 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
> >  		}
> >  	}
> >  
> > -	if (IS_KABYLAKE(dev_priv))
> > +	if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
> >  		return kbl_get_buf_trans_dp(dev_priv, n_entries);
> >  	else
> >  		return skl_get_buf_trans_dp(dev_priv, n_entries);
> > @@ -458,7 +458,7 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por
> >  	if (IS_GEN9_LP(dev_priv))
> >  		return hdmi_level;
> >  
> > -	if (IS_GEN9_BC(dev_priv)) {
> > +	if (IS_GEN9_BC(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> 
> Shouldn't GEN9_BC already include COFFEELAKE?
> 
> >  		skl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> >  		hdmi_default_entry = 8;
> >  	} else if (IS_BROADWELL(dev_priv)) {
> > @@ -1478,7 +1478,7 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, u32 level)
> >  		if (dp_iboost) {
> >  			iboost = dp_iboost;
> >  		} else {
> > -			if (IS_KABYLAKE(dev_priv))
> > +			if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
> >  				ddi_translations = kbl_get_buf_trans_dp(dev_priv,
> >  									&n_entries);
> >  			else
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 936eef1..3e762b1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3549,7 +3549,7 @@ static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
> >  static bool
> >  intel_has_sagv(struct drm_i915_private *dev_priv)
> >  {
> > -	if (IS_KABYLAKE(dev_priv))
> > +	if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
> >  		return true;
> >  
> >  	if (IS_SKYLAKE(dev_priv) &&
> > @@ -8139,7 +8139,7 @@ static void kabylake_init_clock_gating(struct drm_i915_private *dev_priv)
> >  		I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> >  			   GEN6_GAMUNIT_CLOCK_GATE_DISABLE);
> >  
> > -	/* WaFbcNukeOnHostModify:kbl */
> > +	/* WaFbcNukeOnHostModify:kbl,cfl */
> 
> I could not verify if this workaround is applicable to CFL.
> 
> 
> 
> >  	I915_WRITE(ILK_DPFC_CHICKEN, I915_READ(ILK_DPFC_CHICKEN) |
> >  		   ILK_DPFC_NUKE_ON_ANY_MODIFICATION);
> >  }
> > @@ -8607,7 +8607,7 @@ void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
> >  {
> >  	if (IS_SKYLAKE(dev_priv))
> >  		dev_priv->display.init_clock_gating = skylake_init_clock_gating;
> > -	else if (IS_KABYLAKE(dev_priv))
> > +	else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv))
> >  		dev_priv->display.init_clock_gating = kabylake_init_clock_gating;

We could perhaps add a distinct hook for CFL here. That way, all the
workarounds can be applied in the following patch, instead of
implementing one wa here and the rest in the next patch. It'd also make
it cleaner to add new wa's for CFL parts. Let me know what you think.

> >  	else if (IS_BROXTON(dev_priv))
> >  		dev_priv->display.init_clock_gating = bxt_init_clock_gating;
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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