Re: [RFC PATCH] drm/i915: Consider SPLL as another shared pll.

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 21 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote:
>> When diagnosing a unrelated bug for someone on irc, it would seem the hardware can
>> be brought up by the BIOS with the embedded displayport using the SPLL for spread spectrum.
>> 
>> Right now this is not handled well in i915, and it calculates the crtc needs to
>> be reprogrammed on the first modeset without SSC, but  the SPLL itself was kept
>> active. Fix this by exposing SPLL as a shared pll that will not be returned
>> by intel_get_shared_dpll; you have to know it exists to use it. ;-)
>> 
>> Cc: Emil Renner Berthing <kernel@esmil.dk>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> RFC because I haven't tested it with VGA, but it seems to work according to fix
>> the problem mentioned above.
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 81adf89b92f1..cacdac67d9ba 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -345,6 +345,8 @@ enum intel_dpll_id {
>>  	/* hsw/bdw */
>>  	DPLL_ID_WRPLL1 = 0,
>>  	DPLL_ID_WRPLL2 = 1,
>> +	DPLL_ID_SPLL = 2,
>> +
>>  	/* skl */
>>  	DPLL_ID_SKL_DPLL1 = 0,
>>  	DPLL_ID_SKL_DPLL2 = 1,
>> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
>> index af5e43bef4a4..592d8fe9f991 100644
>> --- a/drivers/gpu/drm/i915/intel_crt.c
>> +++ b/drivers/gpu/drm/i915/intel_crt.c
>> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
>>  	pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder);
>>  }
>>  
>> -static void hsw_crt_pre_enable(struct intel_encoder *encoder)
>> -{
>> -	struct drm_device *dev = encoder->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -
>> -	WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n");
>> -	I915_WRITE(SPLL_CTL,
>> -		   SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC);
>> -	POSTING_READ(SPLL_CTL);
>> -	udelay(20);
>> -}
>> -
>>  /* Note: The caller is required to filter out dpms modes not supported by the
>>   * platform. */
>>  static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode)
>> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
>>  	intel_disable_crt(encoder);
>>  }
>>  
>> -static void hsw_crt_post_disable(struct intel_encoder *encoder)
>> -{
>> -	struct drm_device *dev = encoder->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	uint32_t val;
>> -
>> -	DRM_DEBUG_KMS("Disabling SPLL\n");
>> -	val = I915_READ(SPLL_CTL);
>> -	WARN_ON(!(val & SPLL_PLL_ENABLE));
>> -	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
>> -	POSTING_READ(SPLL_CTL);
>> -}
>> -
>>  static void intel_enable_crt(struct intel_encoder *encoder)
>>  {
>>  	struct intel_crt *crt = intel_encoder_to_crt(encoder);
>> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
>>  	if (HAS_DDI(dev)) {
>>  		pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>>  		pipe_config->port_clock = 135000 * 2;
>> +		pipe_config->dpll_hw_state.wrpll =
>> +			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
>>  	}
>>  
>>  	return true;
>> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev)
>>  	if (HAS_DDI(dev)) {
>>  		crt->base.get_config = hsw_crt_get_config;
>>  		crt->base.get_hw_state = intel_ddi_get_hw_state;
>> -		crt->base.pre_enable = hsw_crt_pre_enable;
>> -		crt->base.post_disable = hsw_crt_post_disable;
>>  	} else {
>>  		crt->base.get_config = intel_crt_get_config;
>>  		crt->base.get_hw_state = intel_crt_get_hw_state;
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 61575f67a626..dabd903147fa 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  		}
>>  
>>  		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +	} else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) {
>> +		struct drm_atomic_state *state = crtc_state->base.state;
>> +		struct intel_shared_dpll_config *spll =
>> +			&intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL];
>> +
>> +		if (spll->crtc_mask &&
>> +		    WARN_ON(spll->hw_state.wrpll != crtc_state->dpll_hw_state.wrpll))
>> +			return false;
>> +
>> +		crtc_state->shared_dpll = DPLL_ID_SPLL;
>> +		spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll;
>> +		spll->crtc_mask |= 1 << intel_crtc->pipe;
>>  	}
>>  
>>  	return true;
>> @@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>>  static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>  			       struct intel_shared_dpll *pll)
>>  {
>> -	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
>> -	POSTING_READ(WRPLL_CTL(pll->id));
>> +	uint32_t reg;
>> +
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		reg = SPLL_CTL;
>> +	else
>> +		reg = WRPLL_CTL(pll->id);
>
> This is a bit ugly, and reusing hw_sate.wrpll is also a bit fragile due to
> the overlaps. I think it'd be better to add hw_state.spll (including hw
> state cross-checking) and specialising the enable/disable/get_hw_state
> functions.
>
> With that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Maarten, please update the patch according to Daniel's comments, as this
fixes [1].

BR,
Jani.

[1] http://mid.gmane.org/5644DBE8.2090404@intel.com



>
>> +
>> +	I915_WRITE(reg, pll->config.hw_state.wrpll);
>> +	POSTING_READ(reg);
>>  	udelay(20);
>>  }
>>  
>>  static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
>>  				struct intel_shared_dpll *pll)
>>  {
>> -	uint32_t val;
>> +	uint32_t reg, val;
>>  
>> -	val = I915_READ(WRPLL_CTL(pll->id));
>> -	I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE);
>> -	POSTING_READ(WRPLL_CTL(pll->id));
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		reg = SPLL_CTL;
>> +	else
>> +		reg = WRPLL_CTL(pll->id);
>> +
>> +	val = I915_READ(reg);
>> +	I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE);
>> +	POSTING_READ(reg);
>>  }
>>  
>>  static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> @@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>  	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
>>  		return false;
>>  
>> -	val = I915_READ(WRPLL_CTL(pll->id));
>> -	hw_state->wrpll = val;
>> +	if (pll->id == DPLL_ID_SPLL)
>> +		val = I915_READ(SPLL_CTL);
>> +	else
>> +		val = I915_READ(WRPLL_CTL(pll->id));
>>  
>> +	hw_state->wrpll = val;
>>  	return val & WRPLL_PLL_ENABLE;
>>  }
>>  
>>  static const char * const hsw_ddi_pll_names[] = {
>>  	"WRPLL 1",
>>  	"WRPLL 2",
>> +	"SPLL"
>>  };
>>  
>>  static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>>  {
>>  	int i;
>>  
>> -	dev_priv->num_shared_dpll = 2;
>> +	dev_priv->num_shared_dpll = 3;
>>  
>> +	/* SPLL is special, but needs to be initialized anyway.. */
>>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>  		dev_priv->shared_dplls[i].id = i;
>>  		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ca9278be49f7..582360d2fa08 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  	struct intel_shared_dpll *pll;
>>  	struct intel_shared_dpll_config *shared_dpll;
>>  	enum intel_dpll_id i;
>> +	int max = dev_priv->num_shared_dpll;
>>  
>>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
>>  
>> @@ -4242,9 +4243,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  		WARN_ON(shared_dpll[i].crtc_mask);
>>  
>>  		goto found;
>> -	}
>> +	} else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv))
>> +		/* Do not consider SPLL */
>> +		max = 2;
>>  
>> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +	for (i = 0; i < max; i++) {
>>  		pll = &dev_priv->shared_dplls[i];
>>  
>>  		/* Only want to check enabled timings first */
>> @@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>>  	case PORT_CLK_SEL_WRPLL2:
>>  		pipe_config->shared_dpll = DPLL_ID_WRPLL2;
>>  		break;
>> +	case PORT_CLK_SEL_SPLL:
>> +		pipe_config->shared_dpll = DPLL_ID_SPLL;
>>  	}
>>  }
>>  
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx