diff mbox

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

Message ID 1491506163-14587-32-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi April 6, 2017, 7:15 p.m. UTC
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@intel.com>
Cc: Art Runyan <arthur.j.runyan@intel.com>
Cc: Clint Taylor <clinton.a.taylor@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Kahola, Mika <mika.kahola@intel.com>
Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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(-)

Comments

Zanoni, Paulo R April 7, 2017, 9:12 p.m. UTC | #1
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@intel.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Kahola, Mika <mika.kahola@intel.com>
> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co
> m>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

>  
> +		/*
> +		 * Configure DPCLKA_CFGCR0 to turn on the clock for
> the DDI.
> +		 * This step and the step before must be done with
> separate
> +		 * register writes.
> +		 */
> +		val = I915_READ(DPCLKA_CFGCR0);
> +		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> +			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
> +		I915_WRITE(DPCLKA_CFGCR0, val);
> +	} else if (IS_GEN9_BC(dev_priv)) {
>  		/* DDI -> PLL mapping  */
>  		val = I915_READ(DPLL_CTRL2);
>  
> @@ -1763,7 +1777,10 @@ static void intel_ddi_post_disable(struct
> intel_encoder *intel_encoder,
>  	if (dig_port)
>  		intel_display_power_put(dev_priv, dig_port-
> >ddi_io_power_domain);
>  
> -	if (IS_GEN9_BC(dev_priv))
> +	if (IS_CANNONLAKE(dev_priv))
> +		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
> +			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> +	else if (IS_GEN9_BC(dev_priv))
>  		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
>  					DPLL_CTRL2_DDI_CLK_OFF(port)
> ));
>  	else if (INTEL_GEN(dev_priv) < 9)
Ander Conselvan de Oliveira May 4, 2017, 12:35 p.m. UTC | #2
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@intel.com>
> > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Kahola, Mika <mika.kahola@intel.com>
> > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co
> > m>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

Btw, I think this patch shows why something like [1] might be a good idea.

[1] https://patchwork.freedesktop.org/patch/113598/
> 
> >  
> > +		/*
> > +		 * Configure DPCLKA_CFGCR0 to turn on the clock for
> > the DDI.
> > +		 * This step and the step before must be done with
> > separate
> > +		 * register writes.
> > +		 */
> > +		val = I915_READ(DPCLKA_CFGCR0);
> > +		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> > +			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
> > +		I915_WRITE(DPCLKA_CFGCR0, val);
> > +	} else if (IS_GEN9_BC(dev_priv)) {
> >  		/* DDI -> PLL mapping  */
> >  		val = I915_READ(DPLL_CTRL2);
> >  
> > @@ -1763,7 +1777,10 @@ static void intel_ddi_post_disable(struct
> > intel_encoder *intel_encoder,
> >  	if (dig_port)
> >  		intel_display_power_put(dev_priv, dig_port-
> > > ddi_io_power_domain);
> > 
> >  
> > -	if (IS_GEN9_BC(dev_priv))
> > +	if (IS_CANNONLAKE(dev_priv))
> > +		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
> > +			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> > +	else if (IS_GEN9_BC(dev_priv))
> >  		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
> >  					DPLL_CTRL2_DDI_CLK_OFF(port)
> > ));
> >  	else if (INTEL_GEN(dev_priv) < 9)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä May 4, 2017, 12:44 p.m. UTC | #3
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@intel.com>
> > > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Kahola, Mika <mika.kahola@intel.com>
> > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co
> > > m>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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...

> 
> Btw, I think this patch shows why something like [1] might be a good idea.
> 
> [1] https://patchwork.freedesktop.org/patch/113598/
> > 
> > >  
> > > +		/*
> > > +		 * Configure DPCLKA_CFGCR0 to turn on the clock for
> > > the DDI.
> > > +		 * This step and the step before must be done with
> > > separate
> > > +		 * register writes.
> > > +		 */
> > > +		val = I915_READ(DPCLKA_CFGCR0);
> > > +		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> > > +			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
> > > +		I915_WRITE(DPCLKA_CFGCR0, val);
> > > +	} else if (IS_GEN9_BC(dev_priv)) {
> > >  		/* DDI -> PLL mapping  */
> > >  		val = I915_READ(DPLL_CTRL2);
> > >  
> > > @@ -1763,7 +1777,10 @@ static void intel_ddi_post_disable(struct
> > > intel_encoder *intel_encoder,
> > >  	if (dig_port)
> > >  		intel_display_power_put(dev_priv, dig_port-
> > > > ddi_io_power_domain);
> > > 
> > >  
> > > -	if (IS_GEN9_BC(dev_priv))
> > > +	if (IS_CANNONLAKE(dev_priv))
> > > +		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
> > > +			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> > > +	else if (IS_GEN9_BC(dev_priv))
> > >  		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
> > >  					DPLL_CTRL2_DDI_CLK_OFF(port)
> > > ));
> > >  	else if (INTEL_GEN(dev_priv) < 9)
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ander Conselvan de Oliveira May 4, 2017, 12:55 p.m. UTC | #4
On Thu, 2017-04-06 at 12:15 -0700, Rodrigo Vivi wrote:
> 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@intel.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Kahola, Mika <mika.kahola@intel.com>
> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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);
>  
> +		/*
> +		 * Configure DPCLKA_CFGCR0 to turn on the clock for the DDI.
> +		 * This step and the step before must be done with separate
> +		 * register writes.
> +		 */
> +		val = I915_READ(DPCLKA_CFGCR0);
> +		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> +			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));

		val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->id, port); ?

Or clearing the clock select to zero has no effect here?

Ander

> +		I915_WRITE(DPCLKA_CFGCR0, val);
> +	} else if (IS_GEN9_BC(dev_priv)) {
>  		/* DDI -> PLL mapping  */
>  		val = I915_READ(DPLL_CTRL2);
>  
> @@ -1763,7 +1777,10 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
>  	if (dig_port)
>  		intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
>  
> -	if (IS_GEN9_BC(dev_priv))
> +	if (IS_CANNONLAKE(dev_priv))
> +		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
> +			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> +	else if (IS_GEN9_BC(dev_priv))
>  		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
>  					DPLL_CTRL2_DDI_CLK_OFF(port)));
>  	else if (INTEL_GEN(dev_priv) < 9)
Maarten Lankhorst May 4, 2017, 1:02 p.m. UTC | #5
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@intel.com>
>>>> Cc: Art Runyan <arthur.j.runyan@intel.com>
>>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Kahola, Mika <mika.kahola@intel.com>
>>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co
>>>> m>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. :)

~Maarten
Ville Syrjälä May 4, 2017, 1:11 p.m. UTC | #6
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@intel.com>
> >>>> Cc: Art Runyan <arthur.j.runyan@intel.com>
> >>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Cc: Kahola, Mika <mika.kahola@intel.com>
> >>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co
> >>>> m>
> >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> Reviewed-by: Kahola, Mika <mika.kahola@intel.com>
> >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.
Rodrigo Vivi May 23, 2017, 7:42 p.m. UTC | #7
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@intel.com>

> > >>>> Cc: Art Runyan <arthur.j.runyan@intel.com>

> > >>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>

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

> > >>>> Cc: Kahola, Mika <mika.kahola@intel.com>

> > >>>> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.co

> > >>>> m>

> > >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > >>>> Reviewed-by: Kahola, Mika <mika.kahola@intel.com>

> > >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. 


>
diff mbox

Patch

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);
 
+		/*
+		 * Configure DPCLKA_CFGCR0 to turn on the clock for the DDI.
+		 * This step and the step before must be done with separate
+		 * register writes.
+		 */
+		val = I915_READ(DPCLKA_CFGCR0);
+		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
+			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
+		I915_WRITE(DPCLKA_CFGCR0, val);
+	} else if (IS_GEN9_BC(dev_priv)) {
 		/* DDI -> PLL mapping  */
 		val = I915_READ(DPLL_CTRL2);
 
@@ -1763,7 +1777,10 @@  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
 	if (dig_port)
 		intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
 
-	if (IS_GEN9_BC(dev_priv))
+	if (IS_CANNONLAKE(dev_priv))
+		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
+			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
+	else if (IS_GEN9_BC(dev_priv))
 		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
 					DPLL_CTRL2_DDI_CLK_OFF(port)));
 	else if (INTEL_GEN(dev_priv) < 9)