[Intel-gfx] [PATCH 32/67] drm/i915/cnl: DDI - PLL mapping

[Vivi, Rodrigo]

On Thu, 2017-05-04 at 16:11 +0300, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:02:07PM +0200, Maarten Lankhorst wrote:
> > Op 04-05-17 om 14:44 schreef Ville Syrjälä:
> > > On Thu, May 04, 2017 at 03:35:51PM +0300, Ander Conselvan De Oliveira wrote:
> > >> On Fri, 2017-04-07 at 18:12 -0300, Paulo Zanoni wrote:
> > >>> Em Qui, 2017-04-06 às 12:15 -0700, Rodrigo Vivi escreveu:
> > >>>> One of the steps for PLL (un)initialization is to (un)map
> > >>>> the correspondent DDI that is actually using that PLL.
> > >>>>
> > >>>> So, let's do this step following the places already stablished
> > >>>> and used so far, although spec put this as part of PLL
> > >>>> initialization sequences.
> > >>>>
> > >>>> v2: Use proper prefix on bits names as suggested by Ander.
> > >>>> v3: Add missed "~". Without that the logic was inverted
> > >>>>     so we were disabling interrupts.
> > >>>>     Credits-to: Clinton
> > >>>>     Credits-to: Art
> > >>>> v4: Spec is getting updated to do DDI -> PLL mapping
> > >>>>     and clock on in 2 separated reg writes. (Paulo)
> > >>>>     Also update bits definitions to use space
> > >>>>     (1 << 1) instead of (1<<1). (Paulo)
> > >>>>
> > >>>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > >>>> Cc: Art Runyan <arthur.j.runyan at intel.com>
> > >>>> Cc: Clint Taylor <clinton.a.taylor at intel.com>
> > >>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >>>> Cc: Kahola, Mika <mika.kahola at intel.com>
> > >>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira at intel.co
> > >>>> m>
> > >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >>>> Reviewed-by: Kahola, Mika <mika.kahola at intel.com>
> > >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >>>> ---
> > >>>>  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> > >>>>  drivers/gpu/drm/i915/intel_ddi.c | 23 ++++++++++++++++++++---
> > >>>>  2 files changed, 29 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > >>>> b/drivers/gpu/drm/i915/i915_reg.h
> > >>>> index 3cfc65f..dcb8e21 100644
> > >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> > >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> > >>>> @@ -8150,6 +8150,15 @@ enum {
> > >>>>  #define DPLL_CFGCR1(id)	_MMIO_PIPE((id) - SKL_DPLL1,
> > >>>> _DPLL1_CFGCR1, _DPLL2_CFGCR1)
> > >>>>  #define DPLL_CFGCR2(id)	_MMIO_PIPE((id) - SKL_DPLL1,
> > >>>> _DPLL1_CFGCR2, _DPLL2_CFGCR2)
> > >>>>  
> > >>>> +/*
> > >>>> + * CNL Clocks
> > >>>> + */
> > >>>> +#define DPCLKA_CFGCR0				_MMIO(0x6C200)
> > >>>> +#define  DPCLKA_CFGCR0_DDI_CLK_OFF(port)	(1 << ((port)+10))
> > >>>> +#define  DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port)	(3 <<
> > >>>> ((port)*2))
> > >>>> +#define  DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port)	((port)*2)
> > >>>> +#define  DPCLKA_CFGCR0_DDI_CLK_SEL(pll, port)	((pll) <<
> > >>>> ((port)*2))
> > >>>> +
> > >>>>  /* BXT display engine PLL */
> > >>>>  #define BXT_DE_PLL_CTL			_MMIO(0x6d000)
> > >>>>  #define   BXT_DE_PLL_RATIO(x)		(x)	/*
> > >>>> {60,65,100} * 19.2MHz */
> > >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > >>>> b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>> index 0914ad9..2a901bf 100644
> > >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> > >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > >>>> @@ -1621,13 +1621,27 @@ static void intel_ddi_clk_select(struct
> > >>>> intel_encoder *encoder,
> > >>>>  {
> > >>>>  	struct drm_i915_private *dev_priv = to_i915(encoder-
> > >>>>> base.dev);
> > >>>>  	enum port port = intel_ddi_get_encoder_port(encoder);
> > >>>> +	uint32_t val;
> > >>>>  
> > >>>>  	if (WARN_ON(!pll))
> > >>>>  		return;
> > >>>>  
> > >>>> -	if (IS_GEN9_BC(dev_priv)) {
> > >>>> -		uint32_t val;
> > >>>> +	if (IS_CANNONLAKE(dev_priv)) {
> > >>>> +		/* Configure DPCLKA_CFGCR0 to map the DPLL to the
> > >>>> DDI. */
> > >>>> +		val = I915_READ(DPCLKA_CFGCR0);
> > >>>> +		val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->id, port);
> > >>>> +		I915_WRITE(DPCLKA_CFGCR0, val);
> > >>> A question to the Atomic Lords: don't we need some sort of locking
> > >>> around this register since it's used by all ports/clocks? I suppose
> > >>> dev_priv->dpll_lock would do...
> > >>>
> > >>> Maybe the same would apply for gen9_bc.
> > >> If there are modesets happening in parallel for different crtcs, then some
> > >> locking is needed. dpll_lock seems like the right call, that's what's used to
> > >> avoid the same problem with the enable/disable hooks.
> > > If something is allowing modesets to commit in parallel then probably
> > > the whole world is on fire. Historically connection_mutex has been there
> > > to protect us, but not sure how that goes with nonblocking commits. I
> > > do hope there's still something there to prevents this...
> > 
> > During nonblocking modesets we don't hold any locks. It's still possible
> > that we force serialization through some other means, for example grabbing
> > all crtc_states might force serialization previously. But I'm not sure this
> > is guaranteed to happen even for SKL. It might happen for when DDB
> > allocation or cdclk changes but there's no guarantee during modeset.
> > 
> > So quite likely you'll need locking here. :)
> 
> Someone just need to fix things so that modesets are always serialized.
> I don't think anyone has actually reviewd the entire driver sufficiently
> to allow parallel modesets.

Besides, the structure itself just follows previous platforms. If there
is something wrong it would affect all current platforms I'm afraid. 


>