[Intel-gfx] [PATCH] drm/i915/ehl: Add support for DPLL4 (v3)

[Vivek Kasireddy]

On Fri, 5 Apr 2019 21:39:11 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
Hi Ville,

> On Fri, Apr 05, 2019 at 09:33:56PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 05, 2019 at 10:59:53AM -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
> > > 
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> > > Reviewed-by: Bob Paauwe <bob.j.paauwe at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 60
> > > ++++++++++++++++++++++++++++++++++- 1 file changed, 59
> > > insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c index
> > > e01c057ce50b..c3f0b9720c54 100644 ---
> > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@
> > > icl_get_dpll(struct intel_crtc_state *crtc_state, return pll;
> > >  }
> > >  
> > > +static struct intel_shared_dpll *
> > > +ehl_get_dpll(struct intel_crtc_state *crtc_state,
> > > +	     struct intel_encoder *encoder)
> > > +{
> > > +	struct drm_i915_private *dev_priv =
> > > to_i915(crtc_state->base.crtc->dev);
> > > +	struct intel_shared_dpll *pll;
> > > +	enum port port = encoder->port;
> > > +	enum intel_dpll_id min, max;
> > > +	bool ret;
> > > +
> > > +	if (!intel_port_is_combophy(dev_priv, port)) {
> > > +		MISSING_CASE(port);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	min = DPLL_ID_ICL_DPLL0;
> > > +	max = DPLL_ID_ICL_DPLL1;
> > > +	ret = icl_calc_dpll_state(crtc_state, encoder);
> > > +	if (ret) {
> > > +		pll = intel_find_shared_dpll(crtc_state, min,
> > > max);
> > > +		if (pll) {
> > > +			intel_reference_shared_dpll(pll,
> > > crtc_state);
> > > +			return pll;
> > > +		}
> > > +	} else {
> > > +		DRM_DEBUG_KMS("Could not calculate PLL
> > > state.\n");
> > > +	}
> > > +
> > > +	if (encoder->type == INTEL_OUTPUT_EDP) {
> > > +		DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	min = max = DPLL_ID_ICL_MGPLL1;
> > > +	ret = icl_calc_mg_pll_state(crtc_state);
> > > +	if (!ret) {
> > > +		DRM_DEBUG_KMS("Could not calculate PLL
> > > state.\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	pll = intel_find_shared_dpll(crtc_state, min, max);
> > > +	if (!pll) {
> > > +		DRM_DEBUG_KMS("No PLL selected\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	intel_reference_shared_dpll(pll, crtc_state);
> > > +	return pll;
> > > +}
> > > +
> > >  static bool mg_pll_get_hw_state(struct drm_i915_private
> > > *dev_priv, struct intel_shared_dpll *pll,
> > >  				struct intel_dpll_hw_state
> > > *hw_state) @@ -3115,6 +3165,13 @@ static void
> > > mg_pll_enable(struct drm_i915_private *dev_priv, i915_reg_t
> > > enable_reg = MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id));
> > >  
> > > +	if (IS_ELKHARTLAKE(dev_priv) &&
> > > +	   (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 ||
> > > +	    I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) {
> > > +		DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are
> > > enabled\n");
> > > +		return;
> > > +	}  
> > 
> > This looks like dead code, or we messed up earlier already (in which
> > case it should just be some kind of assert IMO).  
Are you suggesting that I put this in a ->prepare() hook or much earlier
in the atomic check/prepare phase? FYI, I am just adding a restriction 
mentioned in the spec concerning this DPLL.

> 
> Does DMC handle other MG PLLs? I would have thought not, so if we keep
> the assert then it should perhaps be unconditional. Hmm. But aren't we
> still hodling the modeset power domain when this gets called? If so
> this is definitely dead code.
I am not sure whether DMC handles other MG PLLs but the spec explicitly
says that DMC will not handle this PLL. Where do you suggest is an
appropriate place to put this assert?

> 
> > 
> > Also what is this EHL thing anyway? I can't even find it in the
> > spec. 
EHL = Elkhart Lake, a new Gen 11 platform.

Thanks,
Vivek

> > > +
> > >  	icl_pll_power_enable(dev_priv, pll, enable_reg);
> > >  
> > >  	icl_mg_pll_write(dev_priv, pll);
> > > @@ -3249,12 +3306,13 @@ 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", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 },
> > >  	{ },
> > >  };
> > >  
> > >  static const struct intel_dpll_mgr ehl_pll_mgr = {
> > >  	.dpll_info = ehl_plls,
> > > -	.get_dpll = icl_get_dpll,
> > > +	.get_dpll = ehl_get_dpll,
> > >  	.dump_hw_state = icl_dump_hw_state,
> > >  };
> > >  
> > > -- 
> > > 2.14.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx  
> > 
> > -- 
> > Ville Syrjälä
> > Intel  
>