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

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

On Tue, Apr 09, 2019 at 05:00:44PM -0700, Vivek Kasireddy wrote:
> On Mon, 8 Apr 2019 12:11:15 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> 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@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.
> 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?

If there is some way that could happen currently, then we need to
prevent DC5/6 via some power domain stuff.

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