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

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

On Tue, Jul 16, 2019 at 06:54:39PM -0700, Vivek Kasireddy wrote:
> On Wed, 10 Jul 2019 21:47:52 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> Hi Ville,
> 
> > On Wed, Jul 03, 2019 at 04:03:53PM -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.
> > > 
> > > v8: (Ville and Imre)
> > > - Rename POWER_DOMAIN_DPLL4 TO POWER_DOMAIN_DPLL_DC_OFF
> > > - Grab a reference in intel_modeset_setup_hw_state() if this
> > >   DPLL was already enabled perhaps by BIOS.
> > > - Check for the port type instead of the encoder
> > > 
> > > v9: (Ville)
> > > - Move the block of code that grabs a reference to the power domain
> > >   POWER_DOMAIN_DPLL_DC_OFF to intel_modeset_readout_hw_state() to
> > > ensure that there is a reference present before this DPLL might get
> > > disabled.
> > > 
> > > v10: rebase
> > > 
> > > 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>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  7 +++
> > >  .../drm/i915/display/intel_display_power.c    |  3 ++
> > >  .../drm/i915/display/intel_display_power.h    |  1 +
> > >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 47
> > > +++++++++++++++++-- drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > > |  6 +++ 5 files changed, 60 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c index
> > > 919f5ac844c8..557462208462 100644 ---
> > > a/drivers/gpu/drm/i915/display/intel_display.c +++
> > > b/drivers/gpu/drm/i915/display/intel_display.c @@ -16653,6
> > > +16653,13 @@ static void intel_modeset_readout_hw_state(struct
> > > drm_device *dev) pll->on = pll->info->funcs->get_hw_state(dev_priv,
> > > pll, &pll->state.hw_state);
> > > +
> > > +		if (IS_ELKHARTLAKE(dev_priv) && pll->on &&
> > > +		    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > > +			pll->wakeref =
> > > intel_display_power_get(dev_priv,
> > > +
> > > POWER_DOMAIN_DPLL_DC_OFF);
> > > +		}
> > > +
> > >  		pll->state.crtc_mask = 0;
> > >  		for_each_intel_crtc(dev, crtc) {
> > >  			struct intel_crtc_state *crtc_state =
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c index
> > > c19b958461ca..7437fc71d289 100644 ---
> > > a/drivers/gpu/drm/i915/display/intel_display_power.c +++
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -118,6
> > > +118,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_DPLL_DC_OFF:
> > > +		return "DPLL_DC_OFF";
> > >  	default:
> > >  		MISSING_CASE(domain);
> > >  		return "?";
> > > @@ -2455,6 +2457,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_DPLL_DC_OFF) |
> > > \ BIT_ULL(POWER_DOMAIN_INIT))
> > >  
> > >  #define ICL_DDI_IO_A_POWER_DOMAINS (			\
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > b/drivers/gpu/drm/i915/display/intel_display_power.h index
> > > ff57b0a7fe59..8f43f7051a16 100644 ---
> > > a/drivers/gpu/drm/i915/display/intel_display_power.h +++
> > > b/drivers/gpu/drm/i915/display/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_DPLL_DC_OFF,
> > >  	POWER_DOMAIN_INIT,
> > >  
> > >  	POWER_DOMAIN_NUM,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index
> > > f953971e7c3b..67cfe836286e 100644 ---
> > > a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++
> > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -2905,6 +2905,9
> > > @@ static bool icl_get_combo_phy_dpll(struct intel_atomic_state
> > > *state, intel_atomic_get_new_crtc_state(state, crtc); struct
> > > icl_port_dpll *port_dpll =
> > > &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> > > +	struct drm_i915_private *dev_priv =
> > > to_i915(crtc->base.dev);
> > > +	enum port port = encoder->port;
> > > +	bool has_dpll4 = false;
> > >  
> > >  	if (!icl_calc_dpll_state(crtc_state, encoder,
> > > &port_dpll->hw_state)) { DRM_DEBUG_KMS("Could not calculate combo
> > > PHY PLL state.\n"); @@ -2912,10 +2915,14 @@ static bool
> > > icl_get_combo_phy_dpll(struct intel_atomic_state *state, return
> > > false; }
> > >  
> > > +	if (IS_ELKHARTLAKE(dev_priv) && port != PORT_A)
> > > +		has_dpll4 = true;
> > > +
> > >  	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> > >  						&port_dpll->hw_state,
> > >  						DPLL_ID_ICL_DPLL0,
> > > -						DPLL_ID_ICL_DPLL1);
> > > +						has_dpll4 ?
> > > DPLL_ID_EHL_DPLL4
> > > +							  :
> > > DPLL_ID_ICL_DPLL1); if (!port_dpll->pll) {
> > >  		DRM_DEBUG_KMS("No combo PHY PLL found for port
> > > %c\n", port_name(encoder->port));
> > > @@ -3119,8 +3126,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,
> > > @@ -3231,6 +3244,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_DPLL_DC_OFF);
> > > +	}
> > > +
> > >  	icl_pll_power_enable(dev_priv, pll, enable_reg);
> > >  
> > >  	icl_dpll_write(dev_priv, pll);
> > > @@ -3326,7 +3352,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_DPLL_DC_OFF,
> > > +					pll->wakeref);
> > > +		return;
> > > +	}
> > > +
> > > +	icl_pll_disable(dev_priv, pll, enable_reg);
> > >  }
> > >  
> > >  static void tbt_pll_disable(struct drm_i915_private *dev_priv,
> > > @@ -3406,6 +3444,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/display/intel_dpll_mgr.h
> > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h index
> > > 1668f8116908..4c2c5e93aff3 100644 ---
> > > a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h +++
> > > b/drivers/gpu/drm/i915/display/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) ({			\
> > > @@ -118,6 +119,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,  
> > 
> > While looking at the TGL stuff I noticed that this ID seems to be
> > wrong. We use this to generate the register offsets but the docs show
> > the following:
> > 
> > DPLL0_CFGCR0  0x164000
> > DPLL1_CFGCR0  0x164080
> > TBTPLL_CFGCR0 0x164100
> > DPLL4_CFGCR0  0x164200
> > 
> > So the DPLL4 accesses will now land on TBTPLL. The ID if DPLL4 should
> > really be 4 I guess.
> Yes, looks like it should be 4 indeed. However, I was wondering what is
> the best way to address this issue. Simply changing the id to 4 isn't
> going to work as there are many places in intel_dpll_mgr.c that assume
> the dpll id to be the index in the dev_priv->shared_dplls array as
> stated in the definition of dpll_info:
>         /**
>          * @id: unique indentifier for this DPLL; should match the
> index in the
>          * dev_priv->shared_dplls array
>          */
>         enum intel_dpll_id id;

Ugh. That's a bit unfortunate. I guess it would be nice to abstrace that
away and either allow the array(s) to be sparsely populated, or decouple 
the index from the id.

> 
> Should I change all these places that make this assumption and fix them?
> Or, should I just simply add an if condition around cr0 and cr1 accesses
> in icl_dpll_write and icl_pll_get_hw_state?

I guess we can go with the simple solution as a quick fix.

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