[PATCH 1/2] drm/i915: Use intel_pipe_will_have_type() in ironlake_crtc_compute_clock()

[PATCH 1/2] drm/i915: Use intel_pipe_will_have_type() in ironlake_crtc_compute_clock()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

ironlake_crtc_compute_clock() gets called during atomic compute phase,
so we must check the future pipe type instead of the current type.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a5669a6613e8..26cafeea2845 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8988,7 +8988,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
 	memset(&crtc_state->dpll_hw_state, 0,
 	       sizeof(crtc_state->dpll_hw_state));
 
-	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
+	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
 
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
-- 
2.4.10

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

[PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
forbidden to set it for LVDS/CRT as well. So let's move it out of the
ironlake_compute_dpll() and just do it on demand in the pll enable hook.
This allows the PLL to be shared in more cases when dealing with
different output types.

Note that we must now call the pll enable hook regarless of the current
pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.

FIXME: maybe better to add a separate hook for the "pll already enabled"
case?

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26cafeea2845..ae58f1105458 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (pll->active++) {
 		WARN_ON(!pll->on);
 		assert_shared_dpll_enabled(dev_priv, pll);
+		/* to update high speed IO clock state */
+		pll->enable(dev_priv, pll);
 		return;
 	}
 	WARN_ON(pll->on);
@@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	dpll |= (crtc_state->pixel_multiplier - 1)
 		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
 
-	if (is_sdvo)
-		dpll |= DPLL_SDVO_HIGH_SPEED;
-	if (crtc_state->has_dp_encoder)
-		dpll |= DPLL_SDVO_HIGH_SPEED;
-
 	/* compute bitmask from p1 value */
 	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	/* also FPA1 */
@@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 		return false;
 
 	val = I915_READ(PCH_DPLL(pll->id));
-	hw_state->dpll = val;
+	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
 	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
 	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
 
@@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
 static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc;
+	u32 dpll = pll->config.hw_state.dpll;
+
+	/* Configure high speed IO clock as needed */
+	for_each_intel_crtc(dev, crtc) {
+		if (intel_crtc_to_shared_dpll(crtc) == pll &&
+		    crtc->config->base.active &&
+		    (crtc->config->has_dp_encoder ||
+		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
+		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
+			dpll |= DPLL_SDVO_HIGH_SPEED;
+			break;
+		}
+	}
+
 	/* PCH refclock must be enabled first */
 	ibx_assert_pch_refclk_enabled(dev_priv);
 
-	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), dpll);
 
 	/* Wait for the clocks to stabilize. */
 	POSTING_READ(PCH_DPLL(pll->id));
@@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 	 *
 	 * So write it again.
 	 */
-	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), dpll);
 	POSTING_READ(PCH_DPLL(pll->id));
 	udelay(200);
 }
-- 
2.4.10

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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Daniel Vetter]

On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> forbidden to set it for LVDS/CRT as well. So let's move it out of the
> ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> This allows the PLL to be shared in more cases when dealing with
> different output types.

Nowhere is it forbidden feels a bit weak ... what's the benefits? Do we
really care?
-Daniel

> 
> Note that we must now call the pll enable hook regarless of the current
> pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> 
> FIXME: maybe better to add a separate hook for the "pll already enabled"
> case?
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 26cafeea2845..ae58f1105458 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	if (pll->active++) {
>  		WARN_ON(!pll->on);
>  		assert_shared_dpll_enabled(dev_priv, pll);
> +		/* to update high speed IO clock state */
> +		pll->enable(dev_priv, pll);
>  		return;
>  	}
>  	WARN_ON(pll->on);
> @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	dpll |= (crtc_state->pixel_multiplier - 1)
>  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
>  
> -	if (is_sdvo)
> -		dpll |= DPLL_SDVO_HIGH_SPEED;
> -	if (crtc_state->has_dp_encoder)
> -		dpll |= DPLL_SDVO_HIGH_SPEED;
> -
>  	/* compute bitmask from p1 value */
>  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	/* also FPA1 */
> @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  		return false;
>  
>  	val = I915_READ(PCH_DPLL(pll->id));
> -	hw_state->dpll = val;
> +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
>  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
>  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
>  
> @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
>  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc;
> +	u32 dpll = pll->config.hw_state.dpll;
> +
> +	/* Configure high speed IO clock as needed */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> +		    crtc->config->base.active &&
> +		    (crtc->config->has_dp_encoder ||
> +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> +			dpll |= DPLL_SDVO_HIGH_SPEED;
> +			break;
> +		}
> +	}
> +
>  	/* PCH refclock must be enabled first */
>  	ibx_assert_pch_refclk_enabled(dev_priv);
>  
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), dpll);
>  
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(PCH_DPLL(pll->id));
> @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	 *
>  	 * So write it again.
>  	 */
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), dpll);
>  	POSTING_READ(PCH_DPLL(pll->id));
>  	udelay(200);
>  }
> -- 
> 2.4.10
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 09:41:36AM +0100, Daniel Vetter wrote:
> On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> > forbidden to set it for LVDS/CRT as well. So let's move it out of the
> > ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> > This allows the PLL to be shared in more cases when dealing with
> > different output types.
> 
> Nowhere is it forbidden feels a bit weak ... what's the benefits? Do we
> really care?

We get to share the PLL eg. between HDMI and VGA. We already do that
anyway if we clone HDMI+VGA. Othwerwise we always run out of PLLs when
trying to do three pipes with something like VGA+HDMI+DP.

> -Daniel
> 
> > 
> > Note that we must now call the pll enable hook regarless of the current
> > pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> > 
> > FIXME: maybe better to add a separate hook for the "pll already enabled"
> > case?
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 26cafeea2845..ae58f1105458 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> >  	if (pll->active++) {
> >  		WARN_ON(!pll->on);
> >  		assert_shared_dpll_enabled(dev_priv, pll);
> > +		/* to update high speed IO clock state */
> > +		pll->enable(dev_priv, pll);
> >  		return;
> >  	}
> >  	WARN_ON(pll->on);
> > @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> >  	dpll |= (crtc_state->pixel_multiplier - 1)
> >  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> >  
> > -	if (is_sdvo)
> > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > -	if (crtc_state->has_dp_encoder)
> > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > -
> >  	/* compute bitmask from p1 value */
> >  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> >  	/* also FPA1 */
> > @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> >  		return false;
> >  
> >  	val = I915_READ(PCH_DPLL(pll->id));
> > -	hw_state->dpll = val;
> > +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
> >  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
> >  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
> >  
> > @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> >  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> >  				struct intel_shared_dpll *pll)
> >  {
> > +	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *crtc;
> > +	u32 dpll = pll->config.hw_state.dpll;
> > +
> > +	/* Configure high speed IO clock as needed */
> > +	for_each_intel_crtc(dev, crtc) {
> > +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> > +		    crtc->config->base.active &&
> > +		    (crtc->config->has_dp_encoder ||
> > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> > +			dpll |= DPLL_SDVO_HIGH_SPEED;
> > +			break;
> > +		}
> > +	}
> > +
> >  	/* PCH refclock must be enabled first */
> >  	ibx_assert_pch_refclk_enabled(dev_priv);
> >  
> > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> >  
> >  	/* Wait for the clocks to stabilize. */
> >  	POSTING_READ(PCH_DPLL(pll->id));
> > @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> >  	 *
> >  	 * So write it again.
> >  	 */
> > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> >  	POSTING_READ(PCH_DPLL(pll->id));
> >  	udelay(200);
> >  }
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > 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

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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 01:42:10PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 26, 2015 at 09:41:36AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> > > forbidden to set it for LVDS/CRT as well. So let's move it out of the
> > > ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> > > This allows the PLL to be shared in more cases when dealing with
> > > different output types.
> > 
> > Nowhere is it forbidden feels a bit weak ... what's the benefits? Do we
> > really care?
> 
> We get to share the PLL eg. between HDMI and VGA. We already do that
> anyway if we clone HDMI+VGA. Othwerwise we always run out of PLLs when
> trying to do three pipes with something like VGA+HDMI+DP.

I read through the spec a bit more, and while they don't say things
entirely clearly, I think this bit just enables the x5 clock output
from the DPLL into the port. CRT doesn't need to deal with individual
bits, and so doesn't need this clock. And maybe gating it off at the
DPLL saves a bit of power.

Based on that I'm thinking that in theory we should be able to shared
the DPLL even between CRT and LVDS since the LVDS x7 clock is simply not
relevant for CRT. However the spec clearly groups the DAC with
SDVO/HDMI/DP when talking about the DPLL. Not entirely sure why that is.
If I had a machine with LVDS+CRT I would definitely test out this theory.

Anyways, I still think we want this patch.

> 
> > -Daniel
> > 
> > > 
> > > Note that we must now call the pll enable hook regarless of the current
> > > pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> > > 
> > > FIXME: maybe better to add a separate hook for the "pll already enabled"
> > > case?
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
> > >  1 file changed, 21 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 26cafeea2845..ae58f1105458 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> > >  	if (pll->active++) {
> > >  		WARN_ON(!pll->on);
> > >  		assert_shared_dpll_enabled(dev_priv, pll);
> > > +		/* to update high speed IO clock state */
> > > +		pll->enable(dev_priv, pll);
> > >  		return;
> > >  	}
> > >  	WARN_ON(pll->on);
> > > @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> > >  	dpll |= (crtc_state->pixel_multiplier - 1)
> > >  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> > >  
> > > -	if (is_sdvo)
> > > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > > -	if (crtc_state->has_dp_encoder)
> > > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > > -
> > >  	/* compute bitmask from p1 value */
> > >  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> > >  	/* also FPA1 */
> > > @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> > >  		return false;
> > >  
> > >  	val = I915_READ(PCH_DPLL(pll->id));
> > > -	hw_state->dpll = val;
> > > +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
> > >  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
> > >  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
> > >  
> > > @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> > >  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> > >  				struct intel_shared_dpll *pll)
> > >  {
> > > +	struct drm_device *dev = dev_priv->dev;
> > > +	struct intel_crtc *crtc;
> > > +	u32 dpll = pll->config.hw_state.dpll;
> > > +
> > > +	/* Configure high speed IO clock as needed */
> > > +	for_each_intel_crtc(dev, crtc) {
> > > +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> > > +		    crtc->config->base.active &&
> > > +		    (crtc->config->has_dp_encoder ||
> > > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> > > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> > > +			dpll |= DPLL_SDVO_HIGH_SPEED;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > >  	/* PCH refclock must be enabled first */
> > >  	ibx_assert_pch_refclk_enabled(dev_priv);
> > >  
> > > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> > >  
> > >  	/* Wait for the clocks to stabilize. */
> > >  	POSTING_READ(PCH_DPLL(pll->id));
> > > @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> > >  	 *
> > >  	 * So write it again.
> > >  	 */
> > > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> > >  	POSTING_READ(PCH_DPLL(pll->id));
> > >  	udelay(200);
> > >  }
> > > -- 
> > > 2.4.10
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Daniel Vetter]

On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> forbidden to set it for LVDS/CRT as well. So let's move it out of the
> ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> This allows the PLL to be shared in more cases when dealing with
> different output types.
> 
> Note that we must now call the pll enable hook regarless of the current
> pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> 
> FIXME: maybe better to add a separate hook for the "pll already enabled"
> case?
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 26cafeea2845..ae58f1105458 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	if (pll->active++) {
>  		WARN_ON(!pll->on);
>  		assert_shared_dpll_enabled(dev_priv, pll);
> +		/* to update high speed IO clock state */
> +		pll->enable(dev_priv, pll);
>  		return;
>  	}
>  	WARN_ON(pll->on);
> @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	dpll |= (crtc_state->pixel_multiplier - 1)
>  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
>  
> -	if (is_sdvo)
> -		dpll |= DPLL_SDVO_HIGH_SPEED;
> -	if (crtc_state->has_dp_encoder)
> -		dpll |= DPLL_SDVO_HIGH_SPEED;
> -
>  	/* compute bitmask from p1 value */
>  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	/* also FPA1 */
> @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  		return false;
>  
>  	val = I915_READ(PCH_DPLL(pll->id));
> -	hw_state->dpll = val;
> +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
>  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
>  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
>  
> @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
>  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc;
> +	u32 dpll = pll->config.hw_state.dpll;
> +
> +	/* Configure high speed IO clock as needed */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> +		    crtc->config->base.active &&
> +		    (crtc->config->has_dp_encoder ||
> +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> +			dpll |= DPLL_SDVO_HIGH_SPEED;
> +			break;
> +		}
> +	}

This is a bit ugly. What about adding a needs_5x_clock to struct
intel_shared_dpll_config (i.e. outside of the hw pll state to avoid
unsharing and pipe checker warnings)? ilk_compute_dpll would set that, and
this function here obeys.

Clearing would only happen when we completely disable a pll after the last
user is gone.

I just find looping over anything in modeset code offensive ;-)

Also please add your extended bspec research to the commit message.
-Daniel

> +
>  	/* PCH refclock must be enabled first */
>  	ibx_assert_pch_refclk_enabled(dev_priv);
>  
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), dpll);
>  
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(PCH_DPLL(pll->id));
> @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	 *
>  	 * So write it again.
>  	 */
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), dpll);
>  	POSTING_READ(PCH_DPLL(pll->id));
>  	udelay(200);
>  }
> -- 
> 2.4.10
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Thu, Dec 03, 2015 at 09:02:12AM +0100, Daniel Vetter wrote:
> On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> > forbidden to set it for LVDS/CRT as well. So let's move it out of the
> > ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> > This allows the PLL to be shared in more cases when dealing with
> > different output types.
> > 
> > Note that we must now call the pll enable hook regarless of the current
> > pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> > 
> > FIXME: maybe better to add a separate hook for the "pll already enabled"
> > case?
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 26cafeea2845..ae58f1105458 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> >  	if (pll->active++) {
> >  		WARN_ON(!pll->on);
> >  		assert_shared_dpll_enabled(dev_priv, pll);
> > +		/* to update high speed IO clock state */
> > +		pll->enable(dev_priv, pll);
> >  		return;
> >  	}
> >  	WARN_ON(pll->on);
> > @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> >  	dpll |= (crtc_state->pixel_multiplier - 1)
> >  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> >  
> > -	if (is_sdvo)
> > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > -	if (crtc_state->has_dp_encoder)
> > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > -
> >  	/* compute bitmask from p1 value */
> >  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> >  	/* also FPA1 */
> > @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> >  		return false;
> >  
> >  	val = I915_READ(PCH_DPLL(pll->id));
> > -	hw_state->dpll = val;
> > +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
> >  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
> >  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
> >  
> > @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> >  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> >  				struct intel_shared_dpll *pll)
> >  {
> > +	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *crtc;
> > +	u32 dpll = pll->config.hw_state.dpll;
> > +
> > +	/* Configure high speed IO clock as needed */
> > +	for_each_intel_crtc(dev, crtc) {
> > +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> > +		    crtc->config->base.active &&
> > +		    (crtc->config->has_dp_encoder ||
> > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> > +			dpll |= DPLL_SDVO_HIGH_SPEED;
> > +			break;
> > +		}
> > +	}
> 
> This is a bit ugly. What about adding a needs_5x_clock to struct
> intel_shared_dpll_config (i.e. outside of the hw pll state to avoid
> unsharing and pipe checker warnings)? ilk_compute_dpll would set that, and
> this function here obeys.
> 
> Clearing would only happen when we completely disable a pll after the last
> user is gone.

I don't really like potentially wasting power like that. I suppose it's
a miniscule amount most likely, but still feels a bit wrong.

> 
> I just find looping over anything in modeset code offensive ;-)
> 
> Also please add your extended bspec research to the commit message.
> -Daniel
> 
> > +
> >  	/* PCH refclock must be enabled first */
> >  	ibx_assert_pch_refclk_enabled(dev_priv);
> >  
> > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> >  
> >  	/* Wait for the clocks to stabilize. */
> >  	POSTING_READ(PCH_DPLL(pll->id));
> > @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> >  	 *
> >  	 * So write it again.
> >  	 */
> > -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> > +	I915_WRITE(PCH_DPLL(pll->id), dpll);
> >  	POSTING_READ(PCH_DPLL(pll->id));
> >  	udelay(200);
> >  }
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > 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

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

Re: [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Thu, Dec 03, 2015 at 12:37:20PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 03, 2015 at 09:02:12AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it
> > > forbidden to set it for LVDS/CRT as well. So let's move it out of the
> > > ironlake_compute_dpll() and just do it on demand in the pll enable hook.
> > > This allows the PLL to be shared in more cases when dealing with
> > > different output types.
> > > 
> > > Note that we must now call the pll enable hook regarless of the current
> > > pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately.
> > > 
> > > FIXME: maybe better to add a separate hook for the "pll already enabled"
> > > case?
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
> > >  1 file changed, 21 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 26cafeea2845..ae58f1105458 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> > >  	if (pll->active++) {
> > >  		WARN_ON(!pll->on);
> > >  		assert_shared_dpll_enabled(dev_priv, pll);
> > > +		/* to update high speed IO clock state */
> > > +		pll->enable(dev_priv, pll);
> > >  		return;
> > >  	}
> > >  	WARN_ON(pll->on);
> > > @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> > >  	dpll |= (crtc_state->pixel_multiplier - 1)
> > >  		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> > >  
> > > -	if (is_sdvo)
> > > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > > -	if (crtc_state->has_dp_encoder)
> > > -		dpll |= DPLL_SDVO_HIGH_SPEED;
> > > -
> > >  	/* compute bitmask from p1 value */
> > >  	dpll |= (1 << (crtc_state->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> > >  	/* also FPA1 */
> > > @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> > >  		return false;
> > >  
> > >  	val = I915_READ(PCH_DPLL(pll->id));
> > > -	hw_state->dpll = val;
> > > +	hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED;
> > >  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
> > >  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
> > >  
> > > @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> > >  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> > >  				struct intel_shared_dpll *pll)
> > >  {
> > > +	struct drm_device *dev = dev_priv->dev;
> > > +	struct intel_crtc *crtc;
> > > +	u32 dpll = pll->config.hw_state.dpll;
> > > +
> > > +	/* Configure high speed IO clock as needed */
> > > +	for_each_intel_crtc(dev, crtc) {
> > > +		if (intel_crtc_to_shared_dpll(crtc) == pll &&
> > > +		    crtc->config->base.active &&
> > > +		    (crtc->config->has_dp_encoder ||
> > > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> > > +		     intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) {
> > > +			dpll |= DPLL_SDVO_HIGH_SPEED;
> > > +			break;
> > > +		}
> > > +	}
> > 
> > This is a bit ugly. What about adding a needs_5x_clock to struct
> > intel_shared_dpll_config (i.e. outside of the hw pll state to avoid
> > unsharing and pipe checker warnings)? ilk_compute_dpll would set that, and
> > this function here obeys.
> > 
> > Clearing would only happen when we completely disable a pll after the last
> > user is gone.
> 
> I don't really like potentially wasting power like that. I suppose it's
> a miniscule amount most likely, but still feels a bit wrong.

Hmm. I wonder if I could make it a bitmask of some sort...

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

Re: [PATCH 1/2] drm/i915: Use intel_pipe_will_have_type() in ironlake_crtc_compute_clock()

[Daniel Vetter]

On Wed, Nov 25, 2015 at 04:35:30PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ironlake_crtc_compute_clock() gets called during atomic compute phase,
> so we must check the future pipe type instead of the current type.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a5669a6613e8..26cafeea2845 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8988,7 +8988,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
>  	memset(&crtc_state->dpll_hw_state, 0,
>  	       sizeof(crtc_state->dpll_hw_state));
>  
> -	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
> +	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
>  
>  	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
>  	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
> -- 
> 2.4.10
> 
> _______________________________________________
> 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

Re: [PATCH 1/2] drm/i915: Use intel_pipe_will_have_type() in ironlake_crtc_compute_clock()

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 09:40:26AM +0100, Daniel Vetter wrote:
> On Wed, Nov 25, 2015 at 04:35:30PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > ironlake_crtc_compute_clock() gets called during atomic compute phase,
> > so we must check the future pipe type instead of the current type.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed this patch to dinq. Thanks for the review.

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a5669a6613e8..26cafeea2845 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8988,7 +8988,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
> >  	memset(&crtc_state->dpll_hw_state, 0,
> >  	       sizeof(crtc_state->dpll_hw_state));
> >  
> > -	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
> > +	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
> >  
> >  	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> >  	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > 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

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