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

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

On Fri, Apr 05, 2019 at 04:33:30PM -0700, Vivek Kasireddy wrote:
> On Fri, 5 Apr 2019 21:39:11 +0300
> Ville Syrjälä <ville.syrjala@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@intel.com>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@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?

DC5/6 enable probably is the best, if we even want the assert. And I'm
not convinced that we do since we don't have it for any other PLL
either.

> 
> > 
> > > 
> > > 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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx  
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel  
> > 

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