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

[Gabriel Feceoru <gabriel.feceoru@xxxxxxxxx>]

On 16.11.2015 17:53, Jani Nikula wrote:
> On Mon, 16 Nov 2015, Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> 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.
> >
> > Changes since v1:
> > - Create a separate dpll_hw_state.spll for spll, and use
> >    separate pll functions for spll.
> >
> > Tested-by: Emil Renner Berthing <kernel@xxxxxxxx> #v1
> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > ---
> > Emil, can you retest?
>
> Gabriel, you too please!

I retested it and it's ok.

Regards,
Gabriel.

> BR,
> Jani.
>
> >   drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> >   drivers/gpu/drm/i915/intel_crt.c     | 31 ++-------------
> >   drivers/gpu/drm/i915/intel_ddi.c     | 75 +++++++++++++++++++++++++++++++-----
> >   drivers/gpu/drm/i915/intel_display.c | 15 ++++++--
> >   4 files changed, 83 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 85c84c624ed1..b6b62387cbe1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -351,6 +351,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,
> > @@ -367,6 +369,7 @@ struct intel_dpll_hw_state {
> >
> >   	/* hsw, bdw */
> >   	uint32_t wrpll;
> > +	uint32_t spll;
> >
> >   	/* skl */
> >   	/*
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index b84aaa0bb48a..6a2c76e367a5 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,10 @@ 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 = 0;
> > +		pipe_config->dpll_hw_state.spll =
> > +			SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
> >   	}
> >
> >   	return true;
> > @@ -860,8 +839,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 abb4a265a6df..90fad7ad12ac 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1286,6 +1286,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.spll != crtc_state->dpll_hw_state.spll))
> > +			return false;
> > +
> > +		crtc_state->shared_dpll = DPLL_ID_SPLL;
> > +		spll->hw_state.spll = crtc_state->dpll_hw_state.spll;
> > +		spll->crtc_mask |= 1 << intel_crtc->pipe;
> >   	}
> >
> >   	return true;
> > @@ -2446,7 +2458,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >   	}
> >   }
> >
> > -static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > +static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
> >   			       struct intel_shared_dpll *pll)
> >   {
> >   	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
> > @@ -2454,9 +2466,17 @@ static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >   	udelay(20);
> >   }
> >
> > -static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
> > +static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv,
> >   				struct intel_shared_dpll *pll)
> >   {
> > +	I915_WRITE(SPLL_CTL, pll->config.hw_state.spll);
> > +	POSTING_READ(SPLL_CTL);
> > +	udelay(20);
> > +}
> > +
> > +static void hsw_ddi_wrpll_disable(struct drm_i915_private *dev_priv,
> > +				  struct intel_shared_dpll *pll)
> > +{
> >   	uint32_t val;
> >
> >   	val = I915_READ(WRPLL_CTL(pll->id));
> > @@ -2464,9 +2484,19 @@ static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv,
> >   	POSTING_READ(WRPLL_CTL(pll->id));
> >   }
> >
> > -static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> > -				     struct intel_shared_dpll *pll,
> > -				     struct intel_dpll_hw_state *hw_state)
> > +static void hsw_ddi_spll_disable(struct drm_i915_private *dev_priv,
> > +				 struct intel_shared_dpll *pll)
> > +{
> > +	uint32_t val;
> > +
> > +	val = I915_READ(SPLL_CTL);
> > +	I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
> > +	POSTING_READ(SPLL_CTL);
> > +}
> > +
> > +static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv,
> > +				       struct intel_shared_dpll *pll,
> > +				       struct intel_dpll_hw_state *hw_state)
> >   {
> >   	uint32_t val;
> >
> > @@ -2479,25 +2509,50 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >   	return val & WRPLL_PLL_ENABLE;
> >   }
> >
> > +static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv,
> > +				      struct intel_shared_dpll *pll,
> > +				      struct intel_dpll_hw_state *hw_state)
> > +{
> > +	uint32_t val;
> > +
> > +	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
> > +		return false;
> > +
> > +	val = I915_READ(SPLL_CTL);
> > +	hw_state->spll = val;
> > +
> > +	return val & SPLL_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;
> >
> > -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> > +	for (i = 0; i < 2; i++) {
> >   		dev_priv->shared_dplls[i].id = i;
> >   		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> > -		dev_priv->shared_dplls[i].disable = hsw_ddi_pll_disable;
> > -		dev_priv->shared_dplls[i].enable = hsw_ddi_pll_enable;
> > +		dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable;
> > +		dev_priv->shared_dplls[i].enable = hsw_ddi_wrpll_enable;
> >   		dev_priv->shared_dplls[i].get_hw_state =
> > -			hsw_ddi_pll_get_hw_state;
> > +			hsw_ddi_wrpll_get_hw_state;
> >   	}
> > +
> > +	/* SPLL is special, but needs to be initialized anyway.. */
> > +	dev_priv->shared_dplls[i].id = i;
> > +	dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> > +	dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable;
> > +	dev_priv->shared_dplls[i].enable = hsw_ddi_spll_enable;
> > +	dev_priv->shared_dplls[i].get_hw_state = hsw_ddi_spll_get_hw_state;
> > +
> >   }
> >
> >   static const char * const skl_ddi_pll_names[] = {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3ca38fc493de..b383cbe3964c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4210,6 +4210,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);
> >
> > @@ -4244,9 +4245,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 */
> > @@ -9767,6 +9770,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;
> >   	}
> >   }
> >
> > @@ -12070,9 +12075,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >   			      pipe_config->dpll_hw_state.cfgcr1,
> >   			      pipe_config->dpll_hw_state.cfgcr2);
> >   	} else if (HAS_DDI(dev)) {
> > -		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x\n",
> > +		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
> >   			      pipe_config->ddi_pll_sel,
> > -			      pipe_config->dpll_hw_state.wrpll);
> > +			      pipe_config->dpll_hw_state.wrpll,
> > +			      pipe_config->dpll_hw_state.spll);
> >   	} else {
> >   		DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> >   			      "fp0: 0x%x, fp1: 0x%x\n",
> > @@ -12607,6 +12613,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.fp0);
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.fp1);
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.wrpll);
> > +	PIPE_CONF_CHECK_X(dpll_hw_state.spll);
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1);
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
> >   	PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx