[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 at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at gmail.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: stable at vger.kernel.org
> > Tested-by: Nick Yamane <nick.diego at gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204
> > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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