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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 8 Apr 2019 12:11:15 +0300
Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
Hi,

> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@xxxxxxxxx>
> > > > > ---
> > > > >  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.
As I mentioned earlier, the spec says we cannot enable this DPLL for EHL
if DC5 or DC6 is enabled. Do you think we should disable DC5/DC6 on EHL
in order to enable this DPLL on EHL? Should the user/system integrator 
be making that decision via a module parameter?

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

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux