Re: [PATCH] drm/i915/ehl: Add support for DPLL4 (v7)

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

On Fri, Jun 14, 2019 at 10:22:05PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 07, 2019 at 04:23:26PM -0700, Vivek Kasireddy wrote:
> > This patch adds support for DPLL4 on EHL that include the
> > following restrictions:
> > 
> > - DPLL4 cannot be used with DDIA (combo port A internal eDP usage).
> >   DPLL4 can be used with other DDIs, including DDID
> >   (combo port A external usage).
> > 
> > - DPLL4 cannot be enabled when DC5 or DC6 are enabled.
> > 
> > - The DPLL4 enable, lock, power enabled, and power state are connected
> >   to the MGPLL1_ENABLE register.
> > 
> > v2: (suggestions from Bob Paauwe)
> > - Rework ehl_get_dpll() function to call intel_find_shared_dpll() and
> >   iterate twice: once for Combo plls and once for MG plls.
> > 
> > - Use MG pll funcs for DPLL4 instead of creating new ones and modify
> >   mg_pll_enable to include the restrictions for EHL.
> > 
> > v3: Fix compilation error
> > 
> > v4: (suggestions from Lucas and Ville)
> > - Treat DPLL4 as a combo phy PLL and not as MG PLL
> > - Disable DC states when this DPLL is being enabled
> > - Reuse icl_get_dpll instead of creating a separate one for EHL
> > 
> > v5: (suggestion from Ville)
> > - Refcount the DC OFF power domains during the enabling and disabling
> >   of this DPLL.
> > 
> > v6: rebase
> > 
> > v7: (suggestion from Imre)
> > - Add a new power domain instead of iterating over the domains
> >   assoicated with DC OFF power well.
> > 
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display_power.c |  3 ++
> >  drivers/gpu/drm/i915/intel_display_power.h |  1 +
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c      | 44 ++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h      |  6 +++
> >  4 files changed, 51 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display_power.c b/drivers/gpu/drm/i915/intel_display_power.c
> > index 278a7edc94f5..2134d8b43f58 100644
> > --- a/drivers/gpu/drm/i915/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/intel_display_power.c
> > @@ -116,6 +116,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "MODESET";
> >  	case POWER_DOMAIN_GT_IRQ:
> >  		return "GT_IRQ";
> > +	case POWER_DOMAIN_DPLL4:
> > +		return "DPLL4";
> >  	default:
> >  		MISSING_CASE(domain);
> >  		return "?";
> > @@ -2357,6 +2359,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	ICL_PW_2_POWER_DOMAINS |			\
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_DPLL4) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  
> >  #define ICL_DDI_IO_A_POWER_DOMAINS (			\
> > diff --git a/drivers/gpu/drm/i915/intel_display_power.h b/drivers/gpu/drm/i915/intel_display_power.h
> > index ff57b0a7fe59..cccc47266279 100644
> > --- a/drivers/gpu/drm/i915/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/intel_display_power.h
> > @@ -59,6 +59,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_GMBUS,
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> > +	POWER_DOMAIN_DPLL4,
> 
> Probably should give this a bit more abstract name. _DPLL_NO_DC ?

I think DPLL_NO_DC or DPLL_DC_OFF is ok, we could rename it to be more
generic later if a new user similar to this one comes up.

> 
> Or should we just make it a generic _NO_DC power domain?
> 
> Though it's a bit annoying that we leak the DC yes vs. no into power
> domains which are supposed to be pretty abstract.
> 
> Hmm. Or we could just abuse the MODESET domain here as that's
> pretty much the "make sure my PLLs aren't shut down" domain?

I think it's better to keep it for the use of modeset alone, for
debugging purposes.

> 
> >  	POWER_DOMAIN_INIT,
> >  
> >  	POWER_DOMAIN_NUM,
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 69787f259677..3d712f54dc56 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2806,6 +2806,12 @@ icl_get_dpll(struct intel_crtc_state *crtc_state,
> >  	if (intel_port_is_combophy(dev_priv, port)) {
> >  		min = DPLL_ID_ICL_DPLL0;
> >  		max = DPLL_ID_ICL_DPLL1;
> > +
> > +		if (IS_ELKHARTLAKE(dev_priv)) {
> > +			if (encoder->type != INTEL_OUTPUT_EDP)
> > +				max = DPLL_ID_EHL_DPLL4;
> 
> I think we want
> 
> IS_EHL && port != PORT_A
> 
> 
> > +		}
> > +
> >  		ret = icl_calc_dpll_state(crtc_state, encoder);
> >  	} else if (intel_port_is_tc(dev_priv, port)) {
> >  		if (encoder->type == INTEL_OUTPUT_DP_MST) {
> > @@ -2945,8 +2951,14 @@ static bool combo_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >  				   struct intel_shared_dpll *pll,
> >  				   struct intel_dpll_hw_state *hw_state)
> >  {
> > -	return icl_pll_get_hw_state(dev_priv, pll, hw_state,
> > -				    CNL_DPLL_ENABLE(pll->info->id));
> > +	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> > +
> > +	if (IS_ELKHARTLAKE(dev_priv) &&
> > +	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > +		enable_reg = MG_PLL_ENABLE(0);
> > +	}
> > +
> > +	return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);
> >  }
> >  
> >  static bool tbt_pll_get_hw_state(struct drm_i915_private *dev_priv,
> > @@ -3057,6 +3069,19 @@ static void combo_pll_enable(struct drm_i915_private *dev_priv,
> >  {
> >  	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> >  
> > +	if (IS_ELKHARTLAKE(dev_priv) &&
> > +	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > +		enable_reg = MG_PLL_ENABLE(0);
> > +
> > +		/*
> > +		 * We need to disable DC states when this DPLL is enabled.
> > +		 * This can be done by taking a reference on DPLL4 power
> > +		 * domain.
> > +		 */
> > +		pll->wakeref = intel_display_power_get(dev_priv,
> > +						       POWER_DOMAIN_DPLL4);
> > +	}
> > +
> >  	icl_pll_power_enable(dev_priv, pll, enable_reg);
> >  
> >  	icl_dpll_write(dev_priv, pll);
> > @@ -3152,7 +3177,19 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
> >  static void combo_pll_disable(struct drm_i915_private *dev_priv,
> >  			      struct intel_shared_dpll *pll)
> >  {
> > -	icl_pll_disable(dev_priv, pll, CNL_DPLL_ENABLE(pll->info->id));
> > +	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> > +
> > +	if (IS_ELKHARTLAKE(dev_priv) &&
> > +	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > +		enable_reg = MG_PLL_ENABLE(0);
> > +		icl_pll_disable(dev_priv, pll, enable_reg);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_DPLL4,
> > +					pll->wakeref);
> 
> What happens if we didn't turn on the DPLL ourself?
> Ie. the BIOS did it?

Hm, yea, we'd need to get a reference for those cases from
intel_modeset_setup_hw_state().

> 
> > +		return;
> > +	}
> > +
> > +	icl_pll_disable(dev_priv, pll, enable_reg);
> >  }
> >  
> >  static void tbt_pll_disable(struct drm_i915_private *dev_priv,
> > @@ -3230,6 +3267,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
> >  static const struct dpll_info ehl_plls[] = {
> >  	{ "DPLL 0", &combo_pll_funcs, DPLL_ID_ICL_DPLL0, 0 },
> >  	{ "DPLL 1", &combo_pll_funcs, DPLL_ID_ICL_DPLL1, 0 },
> > +	{ "DPLL 4", &combo_pll_funcs, DPLL_ID_EHL_DPLL4, 0 },
> >  	{ },
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index b5dd9c7ad772..7358b1a525e1 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -28,6 +28,7 @@
> >  #include <linux/types.h>
> >  
> >  #include "intel_display.h"
> > +#include "intel_wakeref.h"
> >  
> >  /*FIXME: Move this to a more appropriate place. */
> >  #define abs_diff(a, b) ({			\
> > @@ -117,6 +118,10 @@ enum intel_dpll_id {
> >  	 * @DPLL_ID_ICL_DPLL1: ICL combo PHY DPLL1
> >  	 */
> >  	DPLL_ID_ICL_DPLL1 = 1,
> > +	/**
> > +	 * @DPLL_ID_EHL_DPLL4: EHL combo PHY DPLL4
> > +	 */
> > +	DPLL_ID_EHL_DPLL4 = 2,
> >  	/**
> >  	 * @DPLL_ID_ICL_TBTPLL: ICL TBT PLL
> >  	 */
> > @@ -312,6 +317,7 @@ struct intel_shared_dpll {
> >  	 * @info: platform specific info
> >  	 */
> >  	const struct dpll_info *info;
> > +	intel_wakeref_t wakeref;
> >  };
> >  
> >  #define SKL_DPLL0 0
> > -- 
> > 2.21.0
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> 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