Re: [PATCH v3 13/13] drm/i915: Make SKL/KBL DPLL0 managed by the shared dpll code

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

 



Op 03-03-16 om 14:40 schreef Ander Conselvan De Oliveira:
> On Thu, 2016-03-03 at 14:33 +0100, Maarten Lankhorst wrote:
>> Op 29-02-16 om 10:08 schreef Ander Conselvan de Oliveira:
>>> Include DPLL0 in the managed dplls for SKL/KBL. While it has to be kept
>>> enabled because of it driving CDCLK, it is better to special case that
>>> inside the DPLL code than in the higher level.
>>>
>>> v2: Use INTEL_DPLL_ALWAYS_ON flag. (Ander)
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <
>>> ander.conselvan.de.oliveira@xxxxxxxxx>
>>>
>>> ---
>>> I wasn't able to test this patch. Would really appreciate a tested by
>>> from someone.
>> Unfortunately no skl with eDP here either. :(
>>>  drivers/gpu/drm/i915/intel_ddi.c      |  21 ------
>>>  drivers/gpu/drm/i915/intel_dp.c       |  52 +-------------
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 131 +++++++++++++++++++++++++++----
>>> ---
>>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |   7 +-
>>>  4 files changed, 109 insertions(+), 102 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>>> b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 3cb9f36..8ef4d2e 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1008,9 +1008,6 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>>>  {
>>>  	struct intel_shared_dpll *pll;
>>>  
>>> -	if (intel_encoder->type == INTEL_OUTPUT_EDP)
>>> -		return true;
>>> -
>>>  	pll = intel_get_shared_dpll(intel_crtc, crtc_state, intel_encoder);
>>>  	if (pll == NULL) {
>>>  		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>>> @@ -1570,24 +1567,6 @@ void intel_ddi_clk_select(struct intel_encoder
>>> *encoder,
>>>  		uint32_t dpll = pipe_config->ddi_pll_sel;
>>>  		uint32_t val;
>>>  
>>> -		/*
>>> -		 * DPLL0 is used for eDP and is the only "private" DPLL (as
>>> -		 * opposed to shared) on SKL
>>> -		 */
>>> -		if (encoder->type == INTEL_OUTPUT_EDP) {
>>> -			WARN_ON(dpll != SKL_DPLL0);
>>> -
>>> -			val = I915_READ(DPLL_CTRL1);
>>> -
>>> -			val &= ~(DPLL_CTRL1_HDMI_MODE(dpll) |
>>> -				 DPLL_CTRL1_SSC(dpll) |
>>> -				 DPLL_CTRL1_LINK_RATE_MASK(dpll));
>>> -			val |= pipe_config->dpll_hw_state.ctrl1 << (dpll *
>>> 6);
>>> -
>>> -			I915_WRITE(DPLL_CTRL1, val);
>>> -			POSTING_READ(DPLL_CTRL1);
>>> -		}
>>> -
>>>  		/* DDI -> PLL mapping  */
>>>  		val = I915_READ(DPLL_CTRL2);
>>>  
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 5be6892..9fef877 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1229,52 +1229,6 @@ intel_dp_connector_unregister(struct intel_connector
>>> *intel_connector)
>>>  	intel_connector_unregister(intel_connector);
>>>  }
>>>  
>>> -static void
>>> -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>>> -{
>>> -	u32 ctrl1;
>>> -
>>> -	memset(&pipe_config->dpll_hw_state, 0,
>>> -	       sizeof(pipe_config->dpll_hw_state));
>>> -
>>> -	pipe_config->ddi_pll_sel = SKL_DPLL0;
>>> -	pipe_config->dpll_hw_state.cfgcr1 = 0;
>>> -	pipe_config->dpll_hw_state.cfgcr2 = 0;
>>> -
>>> -	ctrl1 = DPLL_CTRL1_OVERRIDE(SKL_DPLL0);
>>> -	switch (pipe_config->port_clock / 2) {
>>> -	case 81000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -	case 135000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1350,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -	case 270000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2700,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -	case 162000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -	/* TBD: For DP link rates 2.16 GHz and 4.32 GHz, VCO is 8640 which
>>> -	results in CDCLK change. Need to handle the change of CDCLK by
>>> -	disabling pipes and re-enabling them */
>>> -	case 108000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -	case 216000:
>>> -		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
>>> -					      SKL_DPLL0);
>>> -		break;
>>> -
>>> -	}
>>> -	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>>> -}
>>> -
>>>  static int
>>>  intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
>>>  {
>>> @@ -1632,11 +1586,7 @@ found:
>>>  				&pipe_config->dp_m2_n2);
>>>  	}
>>>  
>>> -	if ((IS_SKYLAKE(dev)  || IS_KABYLAKE(dev)) && is_edp(intel_dp))
>>> -		skl_edp_set_pll_config(pipe_config);
>>> -	else if (IS_BROXTON(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
>>> -		/* handled in ddi */;
>>> -	else
>>> +	if (!HAS_DDI(dev))
>>>  		intel_dp_set_clock(encoder, pipe_config);
>>>  
>>>  	return true;
>>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> index d371680..6ec7836 100644
>>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>> @@ -785,7 +785,12 @@ struct skl_dpll_regs {
>>>  };
>>>  
>>>  /* this array is indexed by the *shared* pll id */
>>> -static const struct skl_dpll_regs skl_dpll_regs[3] = {
>>> +static const struct skl_dpll_regs skl_dpll_regs[4] = {
>>> +	{
>>> +		/* DPLL 0 */
>>> +		.ctl = LCPLL1_CTL,
>>> +		/* DPLL 0 doesn't support HDMI mode */
>>> +	},
>>>  	{
>>>  		/* DPLL 1 */
>>>  		.ctl = LCPLL2_CTL,
>>> @@ -806,24 +811,29 @@ static const struct skl_dpll_regs skl_dpll_regs[3] = {
>>>  	},
>>>  };
>>>  
>>> -static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>> -			       struct intel_shared_dpll *pll)
>>> +static void skl_ddi_pll_write_ctrl1(struct drm_i915_private *dev_priv,
>>> +				    struct intel_shared_dpll *pll)
>>>  {
>>>  	uint32_t val;
>>> -	unsigned int dpll;
>>> -	const struct skl_dpll_regs *regs = skl_dpll_regs;
>>> -
>>> -	/* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1
>>> */
>>> -	dpll = pll->id + 1;
>>>  
>>>  	val = I915_READ(DPLL_CTRL1);
>>>  
>>> -	val &= ~(DPLL_CTRL1_HDMI_MODE(dpll) | DPLL_CTRL1_SSC(dpll) |
>>> -		 DPLL_CTRL1_LINK_RATE_MASK(dpll));
>>> -	val |= pll->config.hw_state.ctrl1 << (dpll * 6);
>>> +	val &= ~(DPLL_CTRL1_HDMI_MODE(pll->id) | DPLL_CTRL1_SSC(pll->id) |
>>> +		 DPLL_CTRL1_LINK_RATE_MASK(pll->id));
>>> +	val |= pll->config.hw_state.ctrl1 << (pll->id * 6);
>>>  
>>>  	I915_WRITE(DPLL_CTRL1, val);
>>>  	POSTING_READ(DPLL_CTRL1);
>>> +}
>>> +
>>> +static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>> +			       struct intel_shared_dpll *pll)
>>> +{
>>> +	const struct skl_dpll_regs *regs = skl_dpll_regs;
>>> +
>>> +	WARN_ON(pll->id == DPLL_ID_SKL_DPLL0);
>>> +
>>> +	skl_ddi_pll_write_ctrl1(dev_priv, pll);
>>>  
>>>  	I915_WRITE(regs[pll->id].cfgcr1, pll->config.hw_state.cfgcr1);
>>>  	I915_WRITE(regs[pll->id].cfgcr2, pll->config.hw_state.cfgcr2);
>>> @@ -834,8 +844,15 @@ static void skl_ddi_pll_enable(struct drm_i915_private
>>> *dev_priv,
>>>  	I915_WRITE(regs[pll->id].ctl,
>>>  		   I915_READ(regs[pll->id].ctl) | LCPLL_PLL_ENABLE);
>>>  
>>> -	if (wait_for(I915_READ(DPLL_STATUS) & DPLL_LOCK(dpll), 5))
>>> -		DRM_ERROR("DPLL %d not locked\n", dpll);
>>> +	if (wait_for(I915_READ(DPLL_STATUS) & DPLL_LOCK(pll->id), 5))
>>> +		DRM_ERROR("DPLL %d not locked\n", pll->id);
>>> +}
>>> +
>>> +static void skl_ddi_dpll0_enable(struct drm_i915_private *dev_priv,
>>> +				 struct intel_shared_dpll *pll)
>>> +{
>>> +	WARN_ON(pll->id != DPLL_ID_SKL_DPLL0);
>>> +	skl_ddi_pll_write_ctrl1(dev_priv, pll);
>>>  }
>>>  
>>>  static void skl_ddi_pll_disable(struct drm_i915_private *dev_priv,
>>> @@ -849,32 +866,35 @@ static void skl_ddi_pll_disable(struct
>>> drm_i915_private *dev_priv,
>>>  	POSTING_READ(regs[pll->id].ctl);
>>>  }
>>>  
>>> +static void skl_ddi_dpll0_disable(struct drm_i915_private *dev_priv,
>>> +				  struct intel_shared_dpll *pll)
>>> +{
>>> +}
>>> +
>>>  static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>>  				     struct intel_shared_dpll *pll,
>>>  				     struct intel_dpll_hw_state *hw_state)
>>>  {
>>>  	uint32_t val;
>>> -	unsigned int dpll;
>>>  	const struct skl_dpll_regs *regs = skl_dpll_regs;
>>>  	bool ret;
>>>  
>>> +	WARN_ON(pll->id == DPLL_ID_SKL_DPLL0);
>>> +
>>>  	if (!intel_display_power_get_if_enabled(dev_priv,
>>> POWER_DOMAIN_PLLS))
>>>  		return false;
>>>  
>>>  	ret = false;
>>>  
>>> -	/* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1
>>> */
>>> -	dpll = pll->id + 1;
>>> -
>>>  	val = I915_READ(regs[pll->id].ctl);
>>>  	if (!(val & LCPLL_PLL_ENABLE))
>>>  		goto out;
>>>  
>>>  	val = I915_READ(DPLL_CTRL1);
>>> -	hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f;
>>> +	hw_state->ctrl1 = (val >> (pll->id * 6)) & 0x3f;
>>>  
>>>  	/* avoid reading back stale values if HDMI mode is not enabled */
>>> -	if (val & DPLL_CTRL1_HDMI_MODE(dpll)) {
>>> +	if (val & DPLL_CTRL1_HDMI_MODE(pll->id)) {
>>>  		hw_state->cfgcr1 = I915_READ(regs[pll->id].cfgcr1);
>>>  		hw_state->cfgcr2 = I915_READ(regs[pll->id].cfgcr2);
>>>  	}
>>> @@ -886,6 +906,37 @@ out:
>>>  	return ret;
>>>  }
>>>  
>>> +static bool skl_ddi_dpll0_get_hw_state(struct drm_i915_private *dev_priv,
>>> +				       struct intel_shared_dpll *pll,
>>> +				       struct intel_dpll_hw_state
>>> *hw_state)
>>> +{
>>> +	uint32_t val;
>>> +	const struct skl_dpll_regs *regs = skl_dpll_regs;
>>> +	bool ret;
>>> +
>>> +	WARN_ON(pll->id != DPLL_ID_SKL_DPLL0);
>> I'm all for paranoia, but how can this ever happen?
>>
>> Seems pll0 has its own function pointers, so you'd have to do something like
>> pll1->funcs->get_hw_state(pll0);
>>
>> I don't think this will ever happen, so all the paranoia from these functions
>> that are called from the function pointer can be removed.
> I went a bit overboard. Will respin.
>
>>> +	if (!intel_display_power_get_if_enabled(dev_priv,
>>> POWER_DOMAIN_PLLS))
>>> +		return false;
>>> +
>>> +	ret = false;
>>> +
>>> +	/* DPLL0 is always enabled since it drives CDCLK */
>>> +	val = I915_READ(regs[pll->id].ctl);
>>> +	if (WARN_ON(!(val & LCPLL_PLL_ENABLE)))
>>> +		goto out;
>>> +
>>> +	val = I915_READ(DPLL_CTRL1);
>>> +	hw_state->ctrl1 = (val >> (pll->id * 6)) & 0x3f;
>>> +
>>> +	ret = true;
>>> +
>>> +out:
>>> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  struct skl_wrpll_context {
>>>  	uint64_t min_deviation;		/* current minimal deviation
>>> */
>>>  	uint64_t central_freq;		/* chosen central freq */
>>> @@ -1165,7 +1216,8 @@ skl_get_dpll(struct intel_crtc *crtc, struct
>>> intel_crtc_state *crtc_state,
>>>  			 DPLL_CFGCR2_PDIV(wrpll_params.pdiv) |
>>>  			 wrpll_params.central_freq;
>>>  	} else if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
>>> -		   encoder->type == INTEL_OUTPUT_DP_MST) {
>>> +		   encoder->type == INTEL_OUTPUT_DP_MST ||
>>> +		   encoder->type == INTEL_OUTPUT_EDP) {
>>>  		switch (crtc_state->port_clock / 2) {
>>>  		case 81000:
>>>  			ctrl1 |=
>>> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810, 0);
>>> @@ -1176,6 +1228,19 @@ skl_get_dpll(struct intel_crtc *crtc, struct
>>> intel_crtc_state *crtc_state,
>>>  		case 270000:
>>>  			ctrl1 |=
>>> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2700, 0);
>>>  			break;
>>> +		/* eDP 1.4 rates */
>>> +		case 162000:
>>> +			ctrl1 |=
>>> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620, 0);
>>> +			break;
>>> +		/* TBD: For DP link rates 2.16 GHz and 4.32 GHz, VCO is
>>> 8640 which
>>> +		results in CDCLK change. Need to handle the change of CDCLK
>>> by
>>> +		disabling pipes and re-enabling them */
>> Sounds like we really need to start supporting cdclk changes on SKL, if it's
>> based on this pll?
>> modeset_calc_cdclk and modeset_commit_cdclk.
> Clint Taylor has been working on it:
>
> https://lists.freedesktop.org/archives/intel-gfx/2016-February/087796.html
>
Ah indeed, that would definitely help with this TBD!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux