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

[PATCH] drm/i915: Allow PCH DPLL sharing regardless 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 also set it on
CRT to make it possible to share the DPLL between HDMI and CRT.

What that bit apparently does is enable the x5 clock to the port,
which then pumps out the bits on both edges of the clock. The DAC
doesn't need that clock since it's not pumping out bits, but I don't
think it hurts to have the DPLL output that clock anyway.

This is fairly important on IVB since it has only two DPLLs with three
pipes. So trying to drive three or more PCH ports with three pipes
is only possible when at least one of the DPLLs gets shared between
two of the pipes.

SNB doesn't really need to do this since it has only two pipes. It could
be done to avoid enabling the second DPLL at all in certain cases, but
I'm not sure that's such a huge win. So let's not do it for SNB, at
least for now. On ILK it never makes sense as the DPLLs can't be shared.

v2: Just always enable the high speed clock to keep things simple (Daniel)
    Beef up the commit message a bit (Daniel)

Cc: Nick Yamane <nick.diego@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Tested-by: Nick Yamane <nick.diego@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5ad1010c8b1..45ff5007544c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (intel_crtc_has_dp_encoder(crtc_state))
 		dpll |= DPLL_SDVO_HIGH_SPEED;
 
+	/*
+	 * The high speed IO clock is only really required for
+	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
+	 * possible to share the DPLL between CRT and HDMI. Enabling
+	 * the clock needlessly does no real harm, except use up a
+	 * bit of power potentially.
+	 *
+	 * We'll limit this to IVB with 3 pipes, since it has only two
+	 * DPLLs and so DPLL sharing is the only way to get three pipes
+	 * driving PCH ports at the same time. On SNB we could do this,
+	 * and potentially avoid enabling the second DPLL, but it's not
+	 * clear if it''s a win or loss power wise. No point in doing
+	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
+	 */
+	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
+	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
+		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 */
-- 
2.7.4

[PATCH] drm/i915: Allow PCH DPLL sharing regardless 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 also set it on
CRT to make it possible to share the DPLL between HDMI and CRT.

What that bit apparently does is enable the x5 clock to the port,
which then pumps out the bits on both edges of the clock. The DAC
doesn't need that clock since it's not pumping out bits, but I don't
think it hurts to have the DPLL output that clock anyway.

This is fairly important on IVB since it has only two DPLLs with three
pipes. So trying to drive three or more PCH ports with three pipes
is only possible when at least one of the DPLLs gets shared between
two of the pipes.

SNB doesn't really need to do this since it has only two pipes. It could
be done to avoid enabling the second DPLL at all in certain cases, but
I'm not sure that's such a huge win. So let's not do it for SNB, at
least for now. On ILK it never makes sense as the DPLLs can't be shared.

v2: Just always enable the high speed clock to keep things simple (Daniel)
    Beef up the commit message a bit (Daniel)

Cc: Nick Yamane <nick.diego@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Tested-by: Nick Yamane <nick.diego@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5ad1010c8b1..45ff5007544c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (intel_crtc_has_dp_encoder(crtc_state))
 		dpll |= DPLL_SDVO_HIGH_SPEED;
 
+	/*
+	 * The high speed IO clock is only really required for
+	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
+	 * possible to share the DPLL between CRT and HDMI. Enabling
+	 * the clock needlessly does no real harm, except use up a
+	 * bit of power potentially.
+	 *
+	 * We'll limit this to IVB with 3 pipes, since it has only two
+	 * DPLLs and so DPLL sharing is the only way to get three pipes
+	 * driving PCH ports at the same time. On SNB we could do this,
+	 * and potentially avoid enabling the second DPLL, but it's not
+	 * clear if it''s a win or loss power wise. No point in doing
+	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
+	 */
+	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
+	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
+		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 */
-- 
2.7.4

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

✗ Fi.CI.BAT: warning for drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Patchwork]

== Series Details ==

Series: drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED
URL   : https://patchwork.freedesktop.org/series/12918/
State : warning

== Summary ==

Series 12918v1 drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED
https://patchwork.freedesktop.org/api/1.0/series/12918/revisions/1/mbox/

Test gem_exec_gttfill:
        Subgroup basic:
                skip       -> PASS       (fi-snb-2520m)
                skip       -> PASS       (fi-hsw-4770r)
                skip       -> PASS       (fi-byt-n2820)
                skip       -> PASS       (fi-ivb-3520m)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
fi-byt-n2820     total:244  pass:208  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:221  dwarn:1   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:227  dwarn:2   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:206  dwarn:0   dfail:0   fail:0   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_2574/

3c9e639197bb52280334830e611082d5b6bfaceb drm-intel-nightly: 2016y-09m-25d-21h-07m-07s UTC integration manifest
92451b7 drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Mon, Sep 26, 2016 at 09:23:55AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED
> URL   : https://patchwork.freedesktop.org/series/12918/
> State : warning
> 
> == Summary ==
> 
> Series 12918v1 drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED
> https://patchwork.freedesktop.org/api/1.0/series/12918/revisions/1/mbox/
> 
> Test gem_exec_gttfill:
>         Subgroup basic:
>                 skip       -> PASS       (fi-snb-2520m)
>                 skip       -> PASS       (fi-hsw-4770r)
>                 skip       -> PASS       (fi-byt-n2820)
>                 skip       -> PASS       (fi-ivb-3520m)
> Test kms_pipe_crc_basic:
>         Subgroup read-crc-pipe-c-frame-sequence:
>                 pass       -> DMESG-WARN (fi-skl-6770hq)

[  593.241193] [drm:skl_set_cdclk] *ERROR* failed to inform PCU about cdclk change

Filed a bug:
https://bugs.freedesktop.org/show_bug.cgi?id=97929

> 
> fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
> fi-byt-n2820     total:244  pass:208  dwarn:0   dfail:0   fail:1   skip:35 
> fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
> fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
> fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
> fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
> fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-6700hq    total:244  pass:221  dwarn:1   dfail:0   fail:0   skip:22 
> fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
> fi-skl-6770hq    total:244  pass:227  dwarn:2   dfail:0   fail:1   skip:14 
> fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
> fi-snb-2600      total:244  pass:206  dwarn:0   dfail:0   fail:0   skip:38 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_2574/
> 
> 3c9e639197bb52280334830e611082d5b6bfaceb drm-intel-nightly: 2016y-09m-25d-21h-07m-07s UTC integration manifest
> 92451b7 drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

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

Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Ander Conselvan De Oliveira]

On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> CRT to make it possible to share the DPLL between HDMI and CRT.
> 
> What that bit apparently does is enable the x5 clock to the port,
> which then pumps out the bits on both edges of the clock. The DAC
> doesn't need that clock since it's not pumping out bits, but I don't
> think it hurts to have the DPLL output that clock anyway.
> 
> This is fairly important on IVB since it has only two DPLLs with three
> pipes. So trying to drive three or more PCH ports with three pipes
> is only possible when at least one of the DPLLs gets shared between
> two of the pipes.
> 
> SNB doesn't really need to do this since it has only two pipes. It could
> be done to avoid enabling the second DPLL at all in certain cases, but
> I'm not sure that's such a huge win. So let's not do it for SNB, at
> least for now. On ILK it never makes sense as the DPLLs can't be shared.
> 
> v2: Just always enable the high speed clock to keep things simple (Daniel)
>     Beef up the commit message a bit (Daniel)
> 
> Cc: Nick Yamane <nick.diego@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> Tested-by: Nick Yamane <nick.diego@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index e5ad1010c8b1..45ff5007544c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> *intel_crtc,
>  	if (intel_crtc_has_dp_encoder(crtc_state))
>  		dpll |= DPLL_SDVO_HIGH_SPEED;
>  
> +	/*
> +	 * The high speed IO clock is only really required for
> +	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
> +	 * possible to share the DPLL between CRT and HDMI. Enabling
> +	 * the clock needlessly does no real harm, except use up a
> +	 * bit of power potentially.

I guess we could have a smarter way to check if two configurations are
compatible than the current memcmp(). I.e., a platform hook that takes two PLL
configs and either returns a merged configuration that satisfy both or signal
failure. That way we could only enable the high speed clock for CRT if really
necessary.

But meh, sounds like too much work for very little gain.

The documentation indeed doesn't say anything about not enabling this with CRT,
so

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> +	 *
> +	 * We'll limit this to IVB with 3 pipes, since it has only two
> +	 * DPLLs and so DPLL sharing is the only way to get three pipes
> +	 * driving PCH ports at the same time. On SNB we could do this,
> +	 * and potentially avoid enabling the second DPLL, but it's not
> +	 * clear if it''s a win or loss power wise. No point in doing
> +	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
> +	 */
> +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> +	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> +		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 */

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

[Ander Conselvan De Oliveira]

On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> CRT to make it possible to share the DPLL between HDMI and CRT.
> 
> What that bit apparently does is enable the x5 clock to the port,
> which then pumps out the bits on both edges of the clock. The DAC
> doesn't need that clock since it's not pumping out bits, but I don't
> think it hurts to have the DPLL output that clock anyway.
> 
> This is fairly important on IVB since it has only two DPLLs with three
> pipes. So trying to drive three or more PCH ports with three pipes
> is only possible when at least one of the DPLLs gets shared between
> two of the pipes.
> 
> SNB doesn't really need to do this since it has only two pipes. It could
> be done to avoid enabling the second DPLL at all in certain cases, but
> I'm not sure that's such a huge win. So let's not do it for SNB, at
> least for now. On ILK it never makes sense as the DPLLs can't be shared.
> 
> v2: Just always enable the high speed clock to keep things simple (Daniel)
>     Beef up the commit message a bit (Daniel)
> 
> Cc: Nick Yamane <nick.diego@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> Tested-by: Nick Yamane <nick.diego@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index e5ad1010c8b1..45ff5007544c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> *intel_crtc,
>  	if (intel_crtc_has_dp_encoder(crtc_state))
>  		dpll |= DPLL_SDVO_HIGH_SPEED;
>  
> +	/*
> +	 * The high speed IO clock is only really required for
> +	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
> +	 * possible to share the DPLL between CRT and HDMI. Enabling
> +	 * the clock needlessly does no real harm, except use up a
> +	 * bit of power potentially.

I guess we could have a smarter way to check if two configurations are
compatible than the current memcmp(). I.e., a platform hook that takes two PLL
configs and either returns a merged configuration that satisfy both or signal
failure. That way we could only enable the high speed clock for CRT if really
necessary.

But meh, sounds like too much work for very little gain.

The documentation indeed doesn't say anything about not enabling this with CRT,
so

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> +	 *
> +	 * We'll limit this to IVB with 3 pipes, since it has only two
> +	 * DPLLs and so DPLL sharing is the only way to get three pipes
> +	 * driving PCH ports at the same time. On SNB we could do this,
> +	 * and potentially avoid enabling the second DPLL, but it's not
> +	 * clear if it''s a win or loss power wise. No point in doing
> +	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
> +	 */
> +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> +	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> +		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 */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> > CRT to make it possible to share the DPLL between HDMI and CRT.
> > 
> > What that bit apparently does is enable the x5 clock to the port,
> > which then pumps out the bits on both edges of the clock. The DAC
> > doesn't need that clock since it's not pumping out bits, but I don't
> > think it hurts to have the DPLL output that clock anyway.
> > 
> > This is fairly important on IVB since it has only two DPLLs with three
> > pipes. So trying to drive three or more PCH ports with three pipes
> > is only possible when at least one of the DPLLs gets shared between
> > two of the pipes.
> > 
> > SNB doesn't really need to do this since it has only two pipes. It could
> > be done to avoid enabling the second DPLL at all in certain cases, but
> > I'm not sure that's such a huge win. So let's not do it for SNB, at
> > least for now. On ILK it never makes sense as the DPLLs can't be shared.
> > 
> > v2: Just always enable the high speed clock to keep things simple (Daniel)
> > ����Beef up the commit message a bit (Daniel)
> > 
> > Cc: Nick Yamane <nick.diego@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > Tested-by: Nick Yamane <nick.diego@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> > �drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> > �1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e5ad1010c8b1..45ff5007544c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> > *intel_crtc,
> > �	if (intel_crtc_has_dp_encoder(crtc_state))
> > �		dpll |= DPLL_SDVO_HIGH_SPEED;
> > �
> > +	/*
> > +	�* The high speed IO clock is only really required for
> > +	�* SDVO/HDMI/DP, but we also enable it for CRT to make it
> > +	�* possible to share the DPLL between CRT and HDMI. Enabling
> > +	�* the clock needlessly does no real harm, except use up a
> > +	�* bit of power potentially.
> 
> I guess we could have a smarter way to check if two configurations are
> compatible than the current memcmp(). I.e., a platform hook that takes two PLL
> configs and either returns a merged configuration that satisfy both or signal
> failure. That way we could only enable the high speed clock for CRT if really
> necessary.
> 
> But meh, sounds like too much work for very little gain.

Yeah. I did implement something more fancy for this (tracking it with a
refcount outside the state we memcmp()), but it was quite a lot of code
for just this little thing. So I decided that I'd rather not have to
debug it if/when it breaks.

> 
> The documentation indeed doesn't say anything about not enabling this with CRT,
> so
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> > +	�*
> > +	�* We'll limit this to IVB with 3 pipes, since it has only two
> > +	�* DPLLs and so DPLL sharing is the only way to get three pipes
> > +	�* driving PCH ports at the same time. On SNB we could do this,
> > +	�* and potentially avoid enabling the second DPLL, but it's not
> > +	�* clear if it''s a win or loss power wise. No point in doing
> > +	�* this on ILK at all since it has a fixed DPLL<->pipe mapping.
> > +	�*/
> > +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> > +	����intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> > +		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 */

-- 
Ville Syrj�l�
Intel OTC

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

[Ville Syrjälä]

On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> > CRT to make it possible to share the DPLL between HDMI and CRT.
> > 
> > What that bit apparently does is enable the x5 clock to the port,
> > which then pumps out the bits on both edges of the clock. The DAC
> > doesn't need that clock since it's not pumping out bits, but I don't
> > think it hurts to have the DPLL output that clock anyway.
> > 
> > This is fairly important on IVB since it has only two DPLLs with three
> > pipes. So trying to drive three or more PCH ports with three pipes
> > is only possible when at least one of the DPLLs gets shared between
> > two of the pipes.
> > 
> > SNB doesn't really need to do this since it has only two pipes. It could
> > be done to avoid enabling the second DPLL at all in certain cases, but
> > I'm not sure that's such a huge win. So let's not do it for SNB, at
> > least for now. On ILK it never makes sense as the DPLLs can't be shared.
> > 
> > v2: Just always enable the high speed clock to keep things simple (Daniel)
> >     Beef up the commit message a bit (Daniel)
> > 
> > Cc: Nick Yamane <nick.diego@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > Tested-by: Nick Yamane <nick.diego@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e5ad1010c8b1..45ff5007544c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> > *intel_crtc,
> >  	if (intel_crtc_has_dp_encoder(crtc_state))
> >  		dpll |= DPLL_SDVO_HIGH_SPEED;
> >  
> > +	/*
> > +	 * The high speed IO clock is only really required for
> > +	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
> > +	 * possible to share the DPLL between CRT and HDMI. Enabling
> > +	 * the clock needlessly does no real harm, except use up a
> > +	 * bit of power potentially.
> 
> I guess we could have a smarter way to check if two configurations are
> compatible than the current memcmp(). I.e., a platform hook that takes two PLL
> configs and either returns a merged configuration that satisfy both or signal
> failure. That way we could only enable the high speed clock for CRT if really
> necessary.
> 
> But meh, sounds like too much work for very little gain.

Yeah. I did implement something more fancy for this (tracking it with a
refcount outside the state we memcmp()), but it was quite a lot of code
for just this little thing. So I decided that I'd rather not have to
debug it if/when it breaks.

> 
> The documentation indeed doesn't say anything about not enabling this with CRT,
> so
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> > +	 *
> > +	 * We'll limit this to IVB with 3 pipes, since it has only two
> > +	 * DPLLs and so DPLL sharing is the only way to get three pipes
> > +	 * driving PCH ports at the same time. On SNB we could do this,
> > +	 * and potentially avoid enabling the second DPLL, but it's not
> > +	 * clear if it''s a win or loss power wise. No point in doing
> > +	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
> > +	 */
> > +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> > +	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> > +		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 */

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

Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> > CRT to make it possible to share the DPLL between HDMI and CRT.
> > 
> > What that bit apparently does is enable the x5 clock to the port,
> > which then pumps out the bits on both edges of the clock. The DAC
> > doesn't need that clock since it's not pumping out bits, but I don't
> > think it hurts to have the DPLL output that clock anyway.
> > 
> > This is fairly important on IVB since it has only two DPLLs with three
> > pipes. So trying to drive three or more PCH ports with three pipes
> > is only possible when at least one of the DPLLs gets shared between
> > two of the pipes.
> > 
> > SNB doesn't really need to do this since it has only two pipes. It could
> > be done to avoid enabling the second DPLL at all in certain cases, but
> > I'm not sure that's such a huge win. So let's not do it for SNB, at
> > least for now. On ILK it never makes sense as the DPLLs can't be shared.
> > 
> > v2: Just always enable the high speed clock to keep things simple (Daniel)
> > ����Beef up the commit message a bit (Daniel)
> > 
> > Cc: Nick Yamane <nick.diego@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > Tested-by: Nick Yamane <nick.diego@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> > �drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> > �1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e5ad1010c8b1..45ff5007544c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> > *intel_crtc,
> > �	if (intel_crtc_has_dp_encoder(crtc_state))
> > �		dpll |= DPLL_SDVO_HIGH_SPEED;
> > �
> > +	/*
> > +	�* The high speed IO clock is only really required for
> > +	�* SDVO/HDMI/DP, but we also enable it for CRT to make it
> > +	�* possible to share the DPLL between CRT and HDMI. Enabling
> > +	�* the clock needlessly does no real harm, except use up a
> > +	�* bit of power potentially.
> 
> I guess we could have a smarter way to check if two configurations are
> compatible than the current memcmp(). I.e., a platform hook that takes two PLL
> configs and either returns a merged configuration that satisfy both or signal
> failure. That way we could only enable the high speed clock for CRT if really
> necessary.
> 
> But meh, sounds like too much work for very little gain.
> 
> The documentation indeed doesn't say anything about not enabling this with CRT,
> so
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks. Pushed to dinq.

> 
> > +	�*
> > +	�* We'll limit this to IVB with 3 pipes, since it has only two
> > +	�* DPLLs and so DPLL sharing is the only way to get three pipes
> > +	�* driving PCH ports at the same time. On SNB we could do this,
> > +	�* and potentially avoid enabling the second DPLL, but it's not
> > +	�* clear if it''s a win or loss power wise. No point in doing
> > +	�* this on ILK at all since it has a fixed DPLL<->pipe mapping.
> > +	�*/
> > +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> > +	����intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> > +		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 */

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-09-26 at 11:30 +0300, 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 also set it on
> > CRT to make it possible to share the DPLL between HDMI and CRT.
> > 
> > What that bit apparently does is enable the x5 clock to the port,
> > which then pumps out the bits on both edges of the clock. The DAC
> > doesn't need that clock since it's not pumping out bits, but I don't
> > think it hurts to have the DPLL output that clock anyway.
> > 
> > This is fairly important on IVB since it has only two DPLLs with three
> > pipes. So trying to drive three or more PCH ports with three pipes
> > is only possible when at least one of the DPLLs gets shared between
> > two of the pipes.
> > 
> > SNB doesn't really need to do this since it has only two pipes. It could
> > be done to avoid enabling the second DPLL at all in certain cases, but
> > I'm not sure that's such a huge win. So let's not do it for SNB, at
> > least for now. On ILK it never makes sense as the DPLLs can't be shared.
> > 
> > v2: Just always enable the high speed clock to keep things simple (Daniel)
> >     Beef up the commit message a bit (Daniel)
> > 
> > Cc: Nick Yamane <nick.diego@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > Tested-by: Nick Yamane <nick.diego@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e5ad1010c8b1..45ff5007544c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc
> > *intel_crtc,
> >  	if (intel_crtc_has_dp_encoder(crtc_state))
> >  		dpll |= DPLL_SDVO_HIGH_SPEED;
> >  
> > +	/*
> > +	 * The high speed IO clock is only really required for
> > +	 * SDVO/HDMI/DP, but we also enable it for CRT to make it
> > +	 * possible to share the DPLL between CRT and HDMI. Enabling
> > +	 * the clock needlessly does no real harm, except use up a
> > +	 * bit of power potentially.
> 
> I guess we could have a smarter way to check if two configurations are
> compatible than the current memcmp(). I.e., a platform hook that takes two PLL
> configs and either returns a merged configuration that satisfy both or signal
> failure. That way we could only enable the high speed clock for CRT if really
> necessary.
> 
> But meh, sounds like too much work for very little gain.
> 
> The documentation indeed doesn't say anything about not enabling this with CRT,
> so
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks. Pushed to dinq.

> 
> > +	 *
> > +	 * We'll limit this to IVB with 3 pipes, since it has only two
> > +	 * DPLLs and so DPLL sharing is the only way to get three pipes
> > +	 * driving PCH ports at the same time. On SNB we could do this,
> > +	 * and potentially avoid enabling the second DPLL, but it's not
> > +	 * clear if it''s a win or loss power wise. No point in doing
> > +	 * this on ILK at all since it has a fixed DPLL<->pipe mapping.
> > +	 */
> > +	if (INTEL_INFO(dev_priv)->num_pipes == 3 &&
> > +	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG))
> > +		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 */

-- 
Ville Syrjälä
Intel OTC