[PATCH v3 0/7] Tiger Lake batch 3.5

[PATCH v3 0/7] Tiger Lake batch 3.5

[Lucas De Marchi]

Mostly the same patches as revision 5 of
https://patchwork.freedesktop.org/series/65290/ with some dropped and
some trivial ones added.

Implementation for the first patches changed though, in order to address
the review comments. Intention here is to get these first 4 patches as
soon as possible so we can have CI enabled with TGL.

Also last patch is not to be merged, it's only here to help with CI as
comments from previous versions need to be handled. Daniele, do you plan
to submit it yourself?

I will be out for the next 2.5w, so don't expect me to handle the
comments here. However José and/or other people can handle them.

thanks
Lucas De Marchi

José Roberto de Souza (2):
  drm/i915/psr: Only handle interruptions of the transcoder in use
  drm/i915/tgl: Access the right register when handling PSR
    interruptions

Lucas De Marchi (4):
  drm/i915: protect access to DP_TP_* on non-dp
  drm/i915/tgl: move DP_TP_* to transcoder
  drm/i915/tgl: disable SAGV temporarily
  drm/i915/tgl: add gen12 to stolen initialization

Michel Thierry (1):
  FIXME: drm/i915/tgl: Register state context definition for Gen12

 drivers/gpu/drm/i915/display/intel_ddi.c      |  49 ++++--
 .../drm/i915/display/intel_display_types.h    |   9 +
 drivers/gpu/drm/i915/display/intel_dp.c       |  13 +-
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |   8 +-
 drivers/gpu/drm/i915/display/intel_psr.c      | 163 +++++++++---------
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |   5 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c           | 156 ++++++++++++-----
 drivers/gpu/drm/i915/gt/intel_lrc.h           |   2 +
 drivers/gpu/drm/i915/gt/intel_lrc_reg.h       |  30 +++-
 drivers/gpu/drm/i915/i915_irq.c               |  51 +++++-
 drivers/gpu/drm/i915/i915_reg.h               |  27 ++-
 drivers/gpu/drm/i915/intel_pm.c               |   4 +
 12 files changed, 345 insertions(+), 172 deletions(-)

-- 
2.23.0

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

[PATCH v3 1/7] drm/i915/psr: Only handle interruptions of the transcoder in use

[Lucas De Marchi]

From: José Roberto de Souza <jose.souza@intel.com>

It was enabling and checking PSR interruptions in every transcoder
while it should keep the interruptions on the non-used transcoders
masked.

While doing this it gives us trouble on Tiger Lake if we are
reading/writing to registers of disabled transcoders since from gen12
onwards the registers are relative to the transcoder. Instead of forcing
them ON to access those registers, just avoid the accesses as they are
not needed.

v2 (Lucas):
  - Explain why we can't keep accessing all transcoders
  - Remove TODO about extending the irq handling to multiple instances:
    when/if implementing multiple instances it's pretty clear by the
    singleton psr that it needs to be extended
  - Fix intel_psr_debug_set() calling psr_irq_control() with
    psr.transcoder not set yet (from Imre). Now we only set the debug
    register right away if psr is already enabled. Otherwise we just
    record the value to be set when enabling the source.
  - Do not depend on the value of TRANSCODER_A. Just be relative to it
    (from Imre)
  - handle psr error last so we don't schedule the work before handling
    the other flags

Cc: Imre Deak <imre.deak@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 137 +++++++++--------------
 drivers/gpu/drm/i915/i915_reg.h          |  13 +--
 2 files changed, 57 insertions(+), 93 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 629b8b98a97f..6f2bf50b6d80 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -88,48 +88,19 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 	}
 }
 
-static int edp_psr_shift(enum transcoder cpu_transcoder)
+static void psr_irq_control(struct drm_i915_private *dev_priv)
 {
-	switch (cpu_transcoder) {
-	case TRANSCODER_A:
-		return EDP_PSR_TRANSCODER_A_SHIFT;
-	case TRANSCODER_B:
-		return EDP_PSR_TRANSCODER_B_SHIFT;
-	case TRANSCODER_C:
-		return EDP_PSR_TRANSCODER_C_SHIFT;
-	default:
-		MISSING_CASE(cpu_transcoder);
-		/* fallthrough */
-	case TRANSCODER_EDP:
-		return EDP_PSR_TRANSCODER_EDP_SHIFT;
-	}
-}
-
-static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
-{
-	u32 debug_mask, mask;
-	enum transcoder cpu_transcoder;
-	u32 transcoders = BIT(TRANSCODER_EDP);
-
-	if (INTEL_GEN(dev_priv) >= 8)
-		transcoders |= BIT(TRANSCODER_A) |
-			       BIT(TRANSCODER_B) |
-			       BIT(TRANSCODER_C);
-
-	debug_mask = 0;
-	mask = 0;
-	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
-		int shift = edp_psr_shift(cpu_transcoder);
-
-		mask |= EDP_PSR_ERROR(shift);
-		debug_mask |= EDP_PSR_POST_EXIT(shift) |
-			      EDP_PSR_PRE_ENTRY(shift);
-	}
+	enum transcoder trans = dev_priv->psr.transcoder;
+	u32 val, mask;
 
-	if (debug & I915_PSR_DEBUG_IRQ)
-		mask |= debug_mask;
+	mask = EDP_PSR_ERROR(trans);
+	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
+		mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
 
-	I915_WRITE(EDP_PSR_IMR, ~mask);
+	val = I915_READ(EDP_PSR_IMR);
+	val &= ~EDP_PSR_TRANS_MASK(trans);
+	val |= ~mask;
+	I915_WRITE(EDP_PSR_IMR, val);
 }
 
 static void psr_event_print(u32 val, bool psr2_enabled)
@@ -171,60 +142,48 @@ static void psr_event_print(u32 val, bool psr2_enabled)
 
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 {
-	u32 transcoders = BIT(TRANSCODER_EDP);
-	enum transcoder cpu_transcoder;
+	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
 	ktime_t time_ns =  ktime_get();
-	u32 mask = 0;
-
-	if (INTEL_GEN(dev_priv) >= 8)
-		transcoders |= BIT(TRANSCODER_A) |
-			       BIT(TRANSCODER_B) |
-			       BIT(TRANSCODER_C);
 
-	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
-		int shift = edp_psr_shift(cpu_transcoder);
+	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+		dev_priv->psr.last_entry_attempt = time_ns;
+		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
+			      transcoder_name(cpu_transcoder));
+	}
 
-		if (psr_iir & EDP_PSR_ERROR(shift)) {
-			DRM_WARN("[transcoder %s] PSR aux error\n",
-				 transcoder_name(cpu_transcoder));
+	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+		dev_priv->psr.last_exit = time_ns;
+		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
+			      transcoder_name(cpu_transcoder));
 
-			dev_priv->psr.irq_aux_error = true;
+		if (INTEL_GEN(dev_priv) >= 9) {
+			u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
+			bool psr2_enabled = dev_priv->psr.psr2_enabled;
 
-			/*
-			 * If this interruption is not masked it will keep
-			 * interrupting so fast that it prevents the scheduled
-			 * work to run.
-			 * Also after a PSR error, we don't want to arm PSR
-			 * again so we don't care about unmask the interruption
-			 * or unset irq_aux_error.
-			 */
-			mask |= EDP_PSR_ERROR(shift);
-		}
-
-		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
-			dev_priv->psr.last_entry_attempt = time_ns;
-			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
-				      transcoder_name(cpu_transcoder));
+			I915_WRITE(PSR_EVENT(cpu_transcoder), val);
+			psr_event_print(val, psr2_enabled);
 		}
+	}
 
-		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
-			dev_priv->psr.last_exit = time_ns;
-			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
-				      transcoder_name(cpu_transcoder));
+	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+		u32 val;
 
-			if (INTEL_GEN(dev_priv) >= 9) {
-				u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
-				bool psr2_enabled = dev_priv->psr.psr2_enabled;
+		DRM_WARN("[transcoder %s] PSR aux error\n",
+			 transcoder_name(cpu_transcoder));
 
-				I915_WRITE(PSR_EVENT(cpu_transcoder), val);
-				psr_event_print(val, psr2_enabled);
-			}
-		}
-	}
+		dev_priv->psr.irq_aux_error = true;
 
-	if (mask) {
-		mask |= I915_READ(EDP_PSR_IMR);
-		I915_WRITE(EDP_PSR_IMR, mask);
+		/*
+		 * If this interruption is not masked it will keep
+		 * interrupting so fast that it prevents the scheduled
+		 * work to run.
+		 * Also after a PSR error, we don't want to arm PSR
+		 * again so we don't care about unmask the interruption
+		 * or unset irq_aux_error.
+		 */
+		val = I915_READ(EDP_PSR_IMR);
+		val |= EDP_PSR_ERROR(cpu_transcoder);
+		I915_WRITE(EDP_PSR_IMR, val);
 
 		schedule_work(&dev_priv->psr.work);
 	}
@@ -747,7 +706,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
 
 	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
 
-	psr_irq_control(dev_priv, dev_priv->psr.debug);
+	psr_irq_control(dev_priv);
 }
 
 static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
@@ -772,7 +731,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
 	 * to avoid any rendering problems.
 	 */
 	val = I915_READ(EDP_PSR_IIR);
-	val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
+	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
 	if (val) {
 		dev_priv->psr.sink_not_reliable = true;
 		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
@@ -1120,7 +1079,13 @@ int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 val)
 
 	old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
 	dev_priv->psr.debug = val;
-	psr_irq_control(dev_priv, dev_priv->psr.debug);
+
+	/*
+	 * Do it right away if it's already enabled, otherwise it will be done
+	 * when enabling the source.
+	 */
+	if (dev_priv->psr.enabled)
+		psr_irq_control(dev_priv);
 
 	mutex_unlock(&dev_priv->psr.lock);
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 02e1ef10c47e..6e7db9c65981 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4225,13 +4225,12 @@ enum {
 /* Bspec claims those aren't shifted but stay at 0x64800 */
 #define EDP_PSR_IMR				_MMIO(0x64834)
 #define EDP_PSR_IIR				_MMIO(0x64838)
-#define   EDP_PSR_ERROR(shift)			(1 << ((shift) + 2))
-#define   EDP_PSR_POST_EXIT(shift)		(1 << ((shift) + 1))
-#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))
-#define   EDP_PSR_TRANSCODER_C_SHIFT		24
-#define   EDP_PSR_TRANSCODER_B_SHIFT		16
-#define   EDP_PSR_TRANSCODER_A_SHIFT		8
-#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
+#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
+						 0 : ((trans) - TRANSCODER_A + 1) * 8)
+#define   EDP_PSR_TRANS_MASK(trans)		(0x7 << _EDP_PSR_TRANS_SHIFT(trans))
+#define   EDP_PSR_ERROR(trans)			(0x4 << _EDP_PSR_TRANS_SHIFT(trans))
+#define   EDP_PSR_POST_EXIT(trans)		(0x2 << _EDP_PSR_TRANS_SHIFT(trans))
+#define   EDP_PSR_PRE_ENTRY(trans)		(0x1 << _EDP_PSR_TRANS_SHIFT(trans))
 
 #define _SRD_AUX_CTL_A				0x60810
 #define _SRD_AUX_CTL_EDP			0x6f810
-- 
2.23.0

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

Re: [PATCH v3 1/7] drm/i915/psr: Only handle interruptions of the transcoder in use

[Matt Roper]

On Thu, Aug 29, 2019 at 02:25:48AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza@intel.com>
> 
> It was enabling and checking PSR interruptions in every transcoder
> while it should keep the interruptions on the non-used transcoders
> masked.
> 
> While doing this it gives us trouble on Tiger Lake if we are
> reading/writing to registers of disabled transcoders since from gen12
> onwards the registers are relative to the transcoder. Instead of forcing
> them ON to access those registers, just avoid the accesses as they are
> not needed.
> 
> v2 (Lucas):
>   - Explain why we can't keep accessing all transcoders
>   - Remove TODO about extending the irq handling to multiple instances:
>     when/if implementing multiple instances it's pretty clear by the
>     singleton psr that it needs to be extended
>   - Fix intel_psr_debug_set() calling psr_irq_control() with
>     psr.transcoder not set yet (from Imre). Now we only set the debug
>     register right away if psr is already enabled. Otherwise we just
>     record the value to be set when enabling the source.
>   - Do not depend on the value of TRANSCODER_A. Just be relative to it
>     (from Imre)
>   - handle psr error last so we don't schedule the work before handling
>     the other flags
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 137 +++++++++--------------
>  drivers/gpu/drm/i915/i915_reg.h          |  13 +--
>  2 files changed, 57 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 629b8b98a97f..6f2bf50b6d80 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -88,48 +88,19 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static int edp_psr_shift(enum transcoder cpu_transcoder)
> +static void psr_irq_control(struct drm_i915_private *dev_priv)
>  {
> -	switch (cpu_transcoder) {
> -	case TRANSCODER_A:
> -		return EDP_PSR_TRANSCODER_A_SHIFT;
> -	case TRANSCODER_B:
> -		return EDP_PSR_TRANSCODER_B_SHIFT;
> -	case TRANSCODER_C:
> -		return EDP_PSR_TRANSCODER_C_SHIFT;
> -	default:
> -		MISSING_CASE(cpu_transcoder);
> -		/* fallthrough */
> -	case TRANSCODER_EDP:
> -		return EDP_PSR_TRANSCODER_EDP_SHIFT;
> -	}
> -}
> -
> -static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> -{
> -	u32 debug_mask, mask;
> -	enum transcoder cpu_transcoder;
> -	u32 transcoders = BIT(TRANSCODER_EDP);
> -
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		transcoders |= BIT(TRANSCODER_A) |
> -			       BIT(TRANSCODER_B) |
> -			       BIT(TRANSCODER_C);
> -
> -	debug_mask = 0;
> -	mask = 0;
> -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> -		int shift = edp_psr_shift(cpu_transcoder);
> -
> -		mask |= EDP_PSR_ERROR(shift);
> -		debug_mask |= EDP_PSR_POST_EXIT(shift) |
> -			      EDP_PSR_PRE_ENTRY(shift);
> -	}
> +	enum transcoder trans = dev_priv->psr.transcoder;
> +	u32 val, mask;
>  
> -	if (debug & I915_PSR_DEBUG_IRQ)
> -		mask |= debug_mask;
> +	mask = EDP_PSR_ERROR(trans);
> +	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> +		mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
>  
> -	I915_WRITE(EDP_PSR_IMR, ~mask);
> +	val = I915_READ(EDP_PSR_IMR);
> +	val &= ~EDP_PSR_TRANS_MASK(trans);
> +	val |= ~mask;
> +	I915_WRITE(EDP_PSR_IMR, val);

I guess we've done this all along, but it jumped out at me during the
review here that we're setting a bunch of bits to 1 that I don't think
have a defined meaning.  I.e., the bspec explicitly indicates that
0x07070707 would be "all interrupts masked" whereas we're setting
something more along the lines of 0xFFFFBFF (for an example with PSR on
transcoder A).  

That's apparently fine for current platforms (since it's what we've been
doing all along), but it makes me slightly more nervous on TGL and
beyond given that the next patch switches to the per-transcoder PSR_IMR
registers and those explicitly say that bits 31:4 are reserved and
must-be-zero.  Maybe we should add a code comment about this just in
case it comes back to bite us on a future platform?


Matt

>  }
>  
>  static void psr_event_print(u32 val, bool psr2_enabled)
> @@ -171,60 +142,48 @@ static void psr_event_print(u32 val, bool psr2_enabled)
>  
>  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  {
> -	u32 transcoders = BIT(TRANSCODER_EDP);
> -	enum transcoder cpu_transcoder;
> +	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
>  	ktime_t time_ns =  ktime_get();
> -	u32 mask = 0;
> -
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		transcoders |= BIT(TRANSCODER_A) |
> -			       BIT(TRANSCODER_B) |
> -			       BIT(TRANSCODER_C);
>  
> -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> -		int shift = edp_psr_shift(cpu_transcoder);
> +	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> +		dev_priv->psr.last_entry_attempt = time_ns;
> +		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> +			      transcoder_name(cpu_transcoder));
> +	}
>  
> -		if (psr_iir & EDP_PSR_ERROR(shift)) {
> -			DRM_WARN("[transcoder %s] PSR aux error\n",
> -				 transcoder_name(cpu_transcoder));
> +	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> +		dev_priv->psr.last_exit = time_ns;
> +		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> +			      transcoder_name(cpu_transcoder));
>  
> -			dev_priv->psr.irq_aux_error = true;
> +		if (INTEL_GEN(dev_priv) >= 9) {
> +			u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> +			bool psr2_enabled = dev_priv->psr.psr2_enabled;
>  
> -			/*
> -			 * If this interruption is not masked it will keep
> -			 * interrupting so fast that it prevents the scheduled
> -			 * work to run.
> -			 * Also after a PSR error, we don't want to arm PSR
> -			 * again so we don't care about unmask the interruption
> -			 * or unset irq_aux_error.
> -			 */
> -			mask |= EDP_PSR_ERROR(shift);
> -		}
> -
> -		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> -			dev_priv->psr.last_entry_attempt = time_ns;
> -			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> -				      transcoder_name(cpu_transcoder));
> +			I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> +			psr_event_print(val, psr2_enabled);
>  		}
> +	}
>  
> -		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> -			dev_priv->psr.last_exit = time_ns;
> -			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> -				      transcoder_name(cpu_transcoder));
> +	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> +		u32 val;
>  
> -			if (INTEL_GEN(dev_priv) >= 9) {
> -				u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> -				bool psr2_enabled = dev_priv->psr.psr2_enabled;
> +		DRM_WARN("[transcoder %s] PSR aux error\n",
> +			 transcoder_name(cpu_transcoder));
>  
> -				I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> -				psr_event_print(val, psr2_enabled);
> -			}
> -		}
> -	}
> +		dev_priv->psr.irq_aux_error = true;
>  
> -	if (mask) {
> -		mask |= I915_READ(EDP_PSR_IMR);
> -		I915_WRITE(EDP_PSR_IMR, mask);
> +		/*
> +		 * If this interruption is not masked it will keep
> +		 * interrupting so fast that it prevents the scheduled
> +		 * work to run.
> +		 * Also after a PSR error, we don't want to arm PSR
> +		 * again so we don't care about unmask the interruption
> +		 * or unset irq_aux_error.
> +		 */
> +		val = I915_READ(EDP_PSR_IMR);
> +		val |= EDP_PSR_ERROR(cpu_transcoder);
> +		I915_WRITE(EDP_PSR_IMR, val);
>  
>  		schedule_work(&dev_priv->psr.work);
>  	}
> @@ -747,7 +706,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  
>  	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
>  
> -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> +	psr_irq_control(dev_priv);
>  }
>  
>  static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> @@ -772,7 +731,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>  	 * to avoid any rendering problems.
>  	 */
>  	val = I915_READ(EDP_PSR_IIR);
> -	val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
> +	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
>  	if (val) {
>  		dev_priv->psr.sink_not_reliable = true;
>  		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
> @@ -1120,7 +1079,13 @@ int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 val)
>  
>  	old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
>  	dev_priv->psr.debug = val;
> -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> +
> +	/*
> +	 * Do it right away if it's already enabled, otherwise it will be done
> +	 * when enabling the source.
> +	 */
> +	if (dev_priv->psr.enabled)
> +		psr_irq_control(dev_priv);
>  
>  	mutex_unlock(&dev_priv->psr.lock);
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 02e1ef10c47e..6e7db9c65981 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4225,13 +4225,12 @@ enum {
>  /* Bspec claims those aren't shifted but stay at 0x64800 */
>  #define EDP_PSR_IMR				_MMIO(0x64834)
>  #define EDP_PSR_IIR				_MMIO(0x64838)
> -#define   EDP_PSR_ERROR(shift)			(1 << ((shift) + 2))
> -#define   EDP_PSR_POST_EXIT(shift)		(1 << ((shift) + 1))
> -#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))
> -#define   EDP_PSR_TRANSCODER_C_SHIFT		24
> -#define   EDP_PSR_TRANSCODER_B_SHIFT		16
> -#define   EDP_PSR_TRANSCODER_A_SHIFT		8
> -#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> +#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
> +						 0 : ((trans) - TRANSCODER_A + 1) * 8)
> +#define   EDP_PSR_TRANS_MASK(trans)		(0x7 << _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_ERROR(trans)			(0x4 << _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_POST_EXIT(trans)		(0x2 << _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_PRE_ENTRY(trans)		(0x1 << _EDP_PSR_TRANS_SHIFT(trans))
>  
>  #define _SRD_AUX_CTL_A				0x60810
>  #define _SRD_AUX_CTL_EDP			0x6f810
> -- 
> 2.23.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915/psr: Only handle interruptions of the transcoder in use

[Souza, Jose]

On Tue, 2019-09-03 at 14:42 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 02:25:48AM -0700, Lucas De Marchi wrote:
> > From: José Roberto de Souza <jose.souza@intel.com>
> > 
> > It was enabling and checking PSR interruptions in every transcoder
> > while it should keep the interruptions on the non-used transcoders
> > masked.
> > 
> > While doing this it gives us trouble on Tiger Lake if we are
> > reading/writing to registers of disabled transcoders since from
> > gen12
> > onwards the registers are relative to the transcoder. Instead of
> > forcing
> > them ON to access those registers, just avoid the accesses as they
> > are
> > not needed.
> > 
> > v2 (Lucas):
> >   - Explain why we can't keep accessing all transcoders
> >   - Remove TODO about extending the irq handling to multiple
> > instances:
> >     when/if implementing multiple instances it's pretty clear by
> > the
> >     singleton psr that it needs to be extended
> >   - Fix intel_psr_debug_set() calling psr_irq_control() with
> >     psr.transcoder not set yet (from Imre). Now we only set the
> > debug
> >     register right away if psr is already enabled. Otherwise we
> > just
> >     record the value to be set when enabling the source.
> >   - Do not depend on the value of TRANSCODER_A. Just be relative to
> > it
> >     (from Imre)
> >   - handle psr error last so we don't schedule the work before
> > handling
> >     the other flags
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 137 +++++++++--------
> > ------
> >  drivers/gpu/drm/i915/i915_reg.h          |  13 +--
> >  2 files changed, 57 insertions(+), 93 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 629b8b98a97f..6f2bf50b6d80 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -88,48 +88,19 @@ static bool intel_psr2_enabled(struct
> > drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > +static void psr_irq_control(struct drm_i915_private *dev_priv)
> >  {
> > -	switch (cpu_transcoder) {
> > -	case TRANSCODER_A:
> > -		return EDP_PSR_TRANSCODER_A_SHIFT;
> > -	case TRANSCODER_B:
> > -		return EDP_PSR_TRANSCODER_B_SHIFT;
> > -	case TRANSCODER_C:
> > -		return EDP_PSR_TRANSCODER_C_SHIFT;
> > -	default:
> > -		MISSING_CASE(cpu_transcoder);
> > -		/* fallthrough */
> > -	case TRANSCODER_EDP:
> > -		return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > -	}
> > -}
> > -
> > -static void psr_irq_control(struct drm_i915_private *dev_priv, u32
> > debug)
> > -{
> > -	u32 debug_mask, mask;
> > -	enum transcoder cpu_transcoder;
> > -	u32 transcoders = BIT(TRANSCODER_EDP);
> > -
> > -	if (INTEL_GEN(dev_priv) >= 8)
> > -		transcoders |= BIT(TRANSCODER_A) |
> > -			       BIT(TRANSCODER_B) |
> > -			       BIT(TRANSCODER_C);
> > -
> > -	debug_mask = 0;
> > -	mask = 0;
> > -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > -		int shift = edp_psr_shift(cpu_transcoder);
> > -
> > -		mask |= EDP_PSR_ERROR(shift);
> > -		debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > -			      EDP_PSR_PRE_ENTRY(shift);
> > -	}
> > +	enum transcoder trans = dev_priv->psr.transcoder;
> > +	u32 val, mask;
> >  
> > -	if (debug & I915_PSR_DEBUG_IRQ)
> > -		mask |= debug_mask;
> > +	mask = EDP_PSR_ERROR(trans);
> > +	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> > +		mask |= EDP_PSR_POST_EXIT(trans) |
> > EDP_PSR_PRE_ENTRY(trans);
> >  
> > -	I915_WRITE(EDP_PSR_IMR, ~mask);
> > +	val = I915_READ(EDP_PSR_IMR);
> > +	val &= ~EDP_PSR_TRANS_MASK(trans);
> > +	val |= ~mask;
> > +	I915_WRITE(EDP_PSR_IMR, val);
> 
> I guess we've done this all along, but it jumped out at me during the
> review here that we're setting a bunch of bits to 1 that I don't
> think
> have a defined meaning.  I.e., the bspec explicitly indicates that
> 0x07070707 would be "all interrupts masked" whereas we're setting
> something more along the lines of 0xFFFFBFF (for an example with PSR
> on
> transcoder A).  
> 
> That's apparently fine for current platforms (since it's what we've
> been
> doing all along), but it makes me slightly more nervous on TGL and
> beyond given that the next patch switches to the per-transcoder
> PSR_IMR
> registers and those explicitly say that bits 31:4 are reserved and
> must-be-zero.  Maybe we should add a code comment about this just in
> case it comes back to bite us on a future platform?

Like you said we do it for all other platforms and for all other
interruption registers but anyways I can add a comment on top:

/* Masking/setting reserved bits too */

It is enough? do you have any other suggestion?

> 
> 
> Matt
> 
> >  }
> >  
> >  static void psr_event_print(u32 val, bool psr2_enabled)
> > @@ -171,60 +142,48 @@ static void psr_event_print(u32 val, bool
> > psr2_enabled)
> >  
> >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > psr_iir)
> >  {
> > -	u32 transcoders = BIT(TRANSCODER_EDP);
> > -	enum transcoder cpu_transcoder;
> > +	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> >  	ktime_t time_ns =  ktime_get();
> > -	u32 mask = 0;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 8)
> > -		transcoders |= BIT(TRANSCODER_A) |
> > -			       BIT(TRANSCODER_B) |
> > -			       BIT(TRANSCODER_C);
> >  
> > -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > -		int shift = edp_psr_shift(cpu_transcoder);
> > +	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > +		dev_priv->psr.last_entry_attempt = time_ns;
> > +		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2
> > vblanks\n",
> > +			      transcoder_name(cpu_transcoder));
> > +	}
> >  
> > -		if (psr_iir & EDP_PSR_ERROR(shift)) {
> > -			DRM_WARN("[transcoder %s] PSR aux error\n",
> > -				 transcoder_name(cpu_transcoder));
> > +	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > +		dev_priv->psr.last_exit = time_ns;
> > +		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > +			      transcoder_name(cpu_transcoder));
> >  
> > -			dev_priv->psr.irq_aux_error = true;
> > +		if (INTEL_GEN(dev_priv) >= 9) {
> > +			u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> > +			bool psr2_enabled = dev_priv->psr.psr2_enabled;
> >  
> > -			/*
> > -			 * If this interruption is not masked it will
> > keep
> > -			 * interrupting so fast that it prevents the
> > scheduled
> > -			 * work to run.
> > -			 * Also after a PSR error, we don't want to arm
> > PSR
> > -			 * again so we don't care about unmask the
> > interruption
> > -			 * or unset irq_aux_error.
> > -			 */
> > -			mask |= EDP_PSR_ERROR(shift);
> > -		}
> > -
> > -		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> > -			dev_priv->psr.last_entry_attempt = time_ns;
> > -			DRM_DEBUG_KMS("[transcoder %s] PSR entry
> > attempt in 2 vblanks\n",
> > -				      transcoder_name(cpu_transcoder));
> > +			I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> > +			psr_event_print(val, psr2_enabled);
> >  		}
> > +	}
> >  
> > -		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> > -			dev_priv->psr.last_exit = time_ns;
> > -			DRM_DEBUG_KMS("[transcoder %s] PSR exit
> > completed\n",
> > -				      transcoder_name(cpu_transcoder));
> > +	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > +		u32 val;
> >  
> > -			if (INTEL_GEN(dev_priv) >= 9) {
> > -				u32 val =
> > I915_READ(PSR_EVENT(cpu_transcoder));
> > -				bool psr2_enabled = dev_priv-
> > >psr.psr2_enabled;
> > +		DRM_WARN("[transcoder %s] PSR aux error\n",
> > +			 transcoder_name(cpu_transcoder));
> >  
> > -				I915_WRITE(PSR_EVENT(cpu_transcoder),
> > val);
> > -				psr_event_print(val, psr2_enabled);
> > -			}
> > -		}
> > -	}
> > +		dev_priv->psr.irq_aux_error = true;
> >  
> > -	if (mask) {
> > -		mask |= I915_READ(EDP_PSR_IMR);
> > -		I915_WRITE(EDP_PSR_IMR, mask);
> > +		/*
> > +		 * If this interruption is not masked it will keep
> > +		 * interrupting so fast that it prevents the scheduled
> > +		 * work to run.
> > +		 * Also after a PSR error, we don't want to arm PSR
> > +		 * again so we don't care about unmask the interruption
> > +		 * or unset irq_aux_error.
> > +		 */
> > +		val = I915_READ(EDP_PSR_IMR);
> > +		val |= EDP_PSR_ERROR(cpu_transcoder);
> > +		I915_WRITE(EDP_PSR_IMR, val);
> >  
> >  		schedule_work(&dev_priv->psr.work);
> >  	}
> > @@ -747,7 +706,7 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  
> >  	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> >  
> > -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> > +	psr_irq_control(dev_priv);
> >  }
> >  
> >  static void intel_psr_enable_locked(struct drm_i915_private
> > *dev_priv,
> > @@ -772,7 +731,7 @@ static void intel_psr_enable_locked(struct
> > drm_i915_private *dev_priv,
> >  	 * to avoid any rendering problems.
> >  	 */
> >  	val = I915_READ(EDP_PSR_IIR);
> > -	val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
> > +	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> >  	if (val) {
> >  		dev_priv->psr.sink_not_reliable = true;
> >  		DRM_DEBUG_KMS("PSR interruption error set, not enabling
> > PSR\n");
> > @@ -1120,7 +1079,13 @@ int intel_psr_debug_set(struct
> > drm_i915_private *dev_priv, u64 val)
> >  
> >  	old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> >  	dev_priv->psr.debug = val;
> > -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> > +
> > +	/*
> > +	 * Do it right away if it's already enabled, otherwise it will
> > be done
> > +	 * when enabling the source.
> > +	 */
> > +	if (dev_priv->psr.enabled)
> > +		psr_irq_control(dev_priv);
> >  
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 02e1ef10c47e..6e7db9c65981 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4225,13 +4225,12 @@ enum {
> >  /* Bspec claims those aren't shifted but stay at 0x64800 */
> >  #define EDP_PSR_IMR				_MMIO(0x64834)
> >  #define EDP_PSR_IIR				_MMIO(0x64838)
> > -#define   EDP_PSR_ERROR(shift)			(1 << ((shift)
> > + 2))
> > -#define   EDP_PSR_POST_EXIT(shift)		(1 << ((shift) + 1))
> > -#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))
> > -#define   EDP_PSR_TRANSCODER_C_SHIFT		24
> > -#define   EDP_PSR_TRANSCODER_B_SHIFT		16
> > -#define   EDP_PSR_TRANSCODER_A_SHIFT		8
> > -#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> > +#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) ==
> > TRANSCODER_EDP ? \
> > +						 0 : ((trans) -
> > TRANSCODER_A + 1) * 8)
> > +#define   EDP_PSR_TRANS_MASK(trans)		(0x7 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define   EDP_PSR_ERROR(trans)			(0x4 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define   EDP_PSR_POST_EXIT(trans)		(0x2 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define   EDP_PSR_PRE_ENTRY(trans)		(0x1 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> >  
> >  #define _SRD_AUX_CTL_A				0x60810
> >  #define _SRD_AUX_CTL_EDP			0x6f810
> > -- 
> > 2.23.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915/psr: Only handle interruptions of the transcoder in use

[Matt Roper]

On Tue, Sep 03, 2019 at 02:53:04PM -0700, Souza, Jose wrote:
> On Tue, 2019-09-03 at 14:42 -0700, Matt Roper wrote:
> > On Thu, Aug 29, 2019 at 02:25:48AM -0700, Lucas De Marchi wrote:
> > > From: José Roberto de Souza <jose.souza@intel.com>
> > > 
> > > It was enabling and checking PSR interruptions in every transcoder
> > > while it should keep the interruptions on the non-used transcoders
> > > masked.
> > > 
> > > While doing this it gives us trouble on Tiger Lake if we are
> > > reading/writing to registers of disabled transcoders since from
> > > gen12
> > > onwards the registers are relative to the transcoder. Instead of
> > > forcing
> > > them ON to access those registers, just avoid the accesses as they
> > > are
> > > not needed.
> > > 
> > > v2 (Lucas):
> > >   - Explain why we can't keep accessing all transcoders
> > >   - Remove TODO about extending the irq handling to multiple
> > > instances:
> > >     when/if implementing multiple instances it's pretty clear by
> > > the
> > >     singleton psr that it needs to be extended
> > >   - Fix intel_psr_debug_set() calling psr_irq_control() with
> > >     psr.transcoder not set yet (from Imre). Now we only set the
> > > debug
> > >     register right away if psr is already enabled. Otherwise we
> > > just
> > >     record the value to be set when enabling the source.
> > >   - Do not depend on the value of TRANSCODER_A. Just be relative to
> > > it
> > >     (from Imre)
> > >   - handle psr error last so we don't schedule the work before
> > > handling
> > >     the other flags
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 137 +++++++++--------
> > > ------
> > >  drivers/gpu/drm/i915/i915_reg.h          |  13 +--
> > >  2 files changed, 57 insertions(+), 93 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 629b8b98a97f..6f2bf50b6d80 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -88,48 +88,19 @@ static bool intel_psr2_enabled(struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > +static void psr_irq_control(struct drm_i915_private *dev_priv)
> > >  {
> > > -	switch (cpu_transcoder) {
> > > -	case TRANSCODER_A:
> > > -		return EDP_PSR_TRANSCODER_A_SHIFT;
> > > -	case TRANSCODER_B:
> > > -		return EDP_PSR_TRANSCODER_B_SHIFT;
> > > -	case TRANSCODER_C:
> > > -		return EDP_PSR_TRANSCODER_C_SHIFT;
> > > -	default:
> > > -		MISSING_CASE(cpu_transcoder);
> > > -		/* fallthrough */
> > > -	case TRANSCODER_EDP:
> > > -		return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > > -	}
> > > -}
> > > -
> > > -static void psr_irq_control(struct drm_i915_private *dev_priv, u32
> > > debug)
> > > -{
> > > -	u32 debug_mask, mask;
> > > -	enum transcoder cpu_transcoder;
> > > -	u32 transcoders = BIT(TRANSCODER_EDP);
> > > -
> > > -	if (INTEL_GEN(dev_priv) >= 8)
> > > -		transcoders |= BIT(TRANSCODER_A) |
> > > -			       BIT(TRANSCODER_B) |
> > > -			       BIT(TRANSCODER_C);
> > > -
> > > -	debug_mask = 0;
> > > -	mask = 0;
> > > -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > > transcoders) {
> > > -		int shift = edp_psr_shift(cpu_transcoder);
> > > -
> > > -		mask |= EDP_PSR_ERROR(shift);
> > > -		debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > > -			      EDP_PSR_PRE_ENTRY(shift);
> > > -	}
> > > +	enum transcoder trans = dev_priv->psr.transcoder;
> > > +	u32 val, mask;
> > >  
> > > -	if (debug & I915_PSR_DEBUG_IRQ)
> > > -		mask |= debug_mask;
> > > +	mask = EDP_PSR_ERROR(trans);
> > > +	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> > > +		mask |= EDP_PSR_POST_EXIT(trans) |
> > > EDP_PSR_PRE_ENTRY(trans);
> > >  
> > > -	I915_WRITE(EDP_PSR_IMR, ~mask);
> > > +	val = I915_READ(EDP_PSR_IMR);
> > > +	val &= ~EDP_PSR_TRANS_MASK(trans);
> > > +	val |= ~mask;
> > > +	I915_WRITE(EDP_PSR_IMR, val);
> > 
> > I guess we've done this all along, but it jumped out at me during the
> > review here that we're setting a bunch of bits to 1 that I don't
> > think
> > have a defined meaning.  I.e., the bspec explicitly indicates that
> > 0x07070707 would be "all interrupts masked" whereas we're setting
> > something more along the lines of 0xFFFFBFF (for an example with PSR
> > on
> > transcoder A).  
> > 
> > That's apparently fine for current platforms (since it's what we've
> > been
> > doing all along), but it makes me slightly more nervous on TGL and
> > beyond given that the next patch switches to the per-transcoder
> > PSR_IMR
> > registers and those explicitly say that bits 31:4 are reserved and
> > must-be-zero.  Maybe we should add a code comment about this just in
> > case it comes back to bite us on a future platform?
> 
> Like you said we do it for all other platforms and for all other
> interruption registers but anyways I can add a comment on top:
> 
> /* Masking/setting reserved bits too */
> 
> It is enough? do you have any other suggestion?

Yeah, I think a comment like that is probably good enough for now.

Aside from that you can consider the patch

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> 
> > 
> > 
> > Matt
> > 
> > >  }
> > >  
> > >  static void psr_event_print(u32 val, bool psr2_enabled)
> > > @@ -171,60 +142,48 @@ static void psr_event_print(u32 val, bool
> > > psr2_enabled)
> > >  
> > >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > > psr_iir)
> > >  {
> > > -	u32 transcoders = BIT(TRANSCODER_EDP);
> > > -	enum transcoder cpu_transcoder;
> > > +	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> > >  	ktime_t time_ns =  ktime_get();
> > > -	u32 mask = 0;
> > > -
> > > -	if (INTEL_GEN(dev_priv) >= 8)
> > > -		transcoders |= BIT(TRANSCODER_A) |
> > > -			       BIT(TRANSCODER_B) |
> > > -			       BIT(TRANSCODER_C);
> > >  
> > > -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > > transcoders) {
> > > -		int shift = edp_psr_shift(cpu_transcoder);
> > > +	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > +		dev_priv->psr.last_entry_attempt = time_ns;
> > > +		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2
> > > vblanks\n",
> > > +			      transcoder_name(cpu_transcoder));
> > > +	}
> > >  
> > > -		if (psr_iir & EDP_PSR_ERROR(shift)) {
> > > -			DRM_WARN("[transcoder %s] PSR aux error\n",
> > > -				 transcoder_name(cpu_transcoder));
> > > +	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > > +		dev_priv->psr.last_exit = time_ns;
> > > +		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > +			      transcoder_name(cpu_transcoder));
> > >  
> > > -			dev_priv->psr.irq_aux_error = true;
> > > +		if (INTEL_GEN(dev_priv) >= 9) {
> > > +			u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> > > +			bool psr2_enabled = dev_priv->psr.psr2_enabled;
> > >  
> > > -			/*
> > > -			 * If this interruption is not masked it will
> > > keep
> > > -			 * interrupting so fast that it prevents the
> > > scheduled
> > > -			 * work to run.
> > > -			 * Also after a PSR error, we don't want to arm
> > > PSR
> > > -			 * again so we don't care about unmask the
> > > interruption
> > > -			 * or unset irq_aux_error.
> > > -			 */
> > > -			mask |= EDP_PSR_ERROR(shift);
> > > -		}
> > > -
> > > -		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> > > -			dev_priv->psr.last_entry_attempt = time_ns;
> > > -			DRM_DEBUG_KMS("[transcoder %s] PSR entry
> > > attempt in 2 vblanks\n",
> > > -				      transcoder_name(cpu_transcoder));
> > > +			I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> > > +			psr_event_print(val, psr2_enabled);
> > >  		}
> > > +	}
> > >  
> > > -		if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> > > -			dev_priv->psr.last_exit = time_ns;
> > > -			DRM_DEBUG_KMS("[transcoder %s] PSR exit
> > > completed\n",
> > > -				      transcoder_name(cpu_transcoder));
> > > +	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > +		u32 val;
> > >  
> > > -			if (INTEL_GEN(dev_priv) >= 9) {
> > > -				u32 val =
> > > I915_READ(PSR_EVENT(cpu_transcoder));
> > > -				bool psr2_enabled = dev_priv-
> > > >psr.psr2_enabled;
> > > +		DRM_WARN("[transcoder %s] PSR aux error\n",
> > > +			 transcoder_name(cpu_transcoder));
> > >  
> > > -				I915_WRITE(PSR_EVENT(cpu_transcoder),
> > > val);
> > > -				psr_event_print(val, psr2_enabled);
> > > -			}
> > > -		}
> > > -	}
> > > +		dev_priv->psr.irq_aux_error = true;
> > >  
> > > -	if (mask) {
> > > -		mask |= I915_READ(EDP_PSR_IMR);
> > > -		I915_WRITE(EDP_PSR_IMR, mask);
> > > +		/*
> > > +		 * If this interruption is not masked it will keep
> > > +		 * interrupting so fast that it prevents the scheduled
> > > +		 * work to run.
> > > +		 * Also after a PSR error, we don't want to arm PSR
> > > +		 * again so we don't care about unmask the interruption
> > > +		 * or unset irq_aux_error.
> > > +		 */
> > > +		val = I915_READ(EDP_PSR_IMR);
> > > +		val |= EDP_PSR_ERROR(cpu_transcoder);
> > > +		I915_WRITE(EDP_PSR_IMR, val);
> > >  
> > >  		schedule_work(&dev_priv->psr.work);
> > >  	}
> > > @@ -747,7 +706,7 @@ static void intel_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  
> > >  	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> > >  
> > > -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > +	psr_irq_control(dev_priv);
> > >  }
> > >  
> > >  static void intel_psr_enable_locked(struct drm_i915_private
> > > *dev_priv,
> > > @@ -772,7 +731,7 @@ static void intel_psr_enable_locked(struct
> > > drm_i915_private *dev_priv,
> > >  	 * to avoid any rendering problems.
> > >  	 */
> > >  	val = I915_READ(EDP_PSR_IIR);
> > > -	val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
> > > +	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> > >  	if (val) {
> > >  		dev_priv->psr.sink_not_reliable = true;
> > >  		DRM_DEBUG_KMS("PSR interruption error set, not enabling
> > > PSR\n");
> > > @@ -1120,7 +1079,13 @@ int intel_psr_debug_set(struct
> > > drm_i915_private *dev_priv, u64 val)
> > >  
> > >  	old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> > >  	dev_priv->psr.debug = val;
> > > -	psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > +
> > > +	/*
> > > +	 * Do it right away if it's already enabled, otherwise it will
> > > be done
> > > +	 * when enabling the source.
> > > +	 */
> > > +	if (dev_priv->psr.enabled)
> > > +		psr_irq_control(dev_priv);
> > >  
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 02e1ef10c47e..6e7db9c65981 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4225,13 +4225,12 @@ enum {
> > >  /* Bspec claims those aren't shifted but stay at 0x64800 */
> > >  #define EDP_PSR_IMR				_MMIO(0x64834)
> > >  #define EDP_PSR_IIR				_MMIO(0x64838)
> > > -#define   EDP_PSR_ERROR(shift)			(1 << ((shift)
> > > + 2))
> > > -#define   EDP_PSR_POST_EXIT(shift)		(1 << ((shift) + 1))
> > > -#define   EDP_PSR_PRE_ENTRY(shift)		(1 << (shift))
> > > -#define   EDP_PSR_TRANSCODER_C_SHIFT		24
> > > -#define   EDP_PSR_TRANSCODER_B_SHIFT		16
> > > -#define   EDP_PSR_TRANSCODER_A_SHIFT		8
> > > -#define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> > > +#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) ==
> > > TRANSCODER_EDP ? \
> > > +						 0 : ((trans) -
> > > TRANSCODER_A + 1) * 8)
> > > +#define   EDP_PSR_TRANS_MASK(trans)		(0x7 <<
> > > _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define   EDP_PSR_ERROR(trans)			(0x4 <<
> > > _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define   EDP_PSR_POST_EXIT(trans)		(0x2 <<
> > > _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define   EDP_PSR_PRE_ENTRY(trans)		(0x1 <<
> > > _EDP_PSR_TRANS_SHIFT(trans))
> > >  
> > >  #define _SRD_AUX_CTL_A				0x60810
> > >  #define _SRD_AUX_CTL_EDP			0x6f810
> > > -- 
> > > 2.23.0
> > > 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/7] drm/i915/tgl: Access the right register when handling PSR interruptions

[Lucas De Marchi]

From: José Roberto de Souza <jose.souza@intel.com>

For older gens PSR IIR and IMR have fixed addresses. From TGL onwards those
registers moved to each transcoder offset. The bits for the registers
are defined without an offset per transcoder as right now we have one
register per transcoder. So add a fake "trans_shift" when calculating
the bits offsets: it will be 0 for gen12+ and psr.transcoder otherwise.

v2 (Lucas): change the implementation to use trans_shift instead of
getting each bit value with a different macro

Cc: Imre Deak <imre.deak@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 60 ++++++++++++++++++------
 drivers/gpu/drm/i915/i915_irq.c          | 51 +++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h          | 10 +++-
 3 files changed, 99 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 6f2bf50b6d80..e01c897ec9f9 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -90,17 +90,32 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 
 static void psr_irq_control(struct drm_i915_private *dev_priv)
 {
-	enum transcoder trans = dev_priv->psr.transcoder;
-	u32 val, mask;
+	enum transcoder trans_shift;
+	u32 mask, val;
+	i915_reg_t imr_reg;
 
-	mask = EDP_PSR_ERROR(trans);
+	/*
+	 * gen12+ has registers relative to transcoder and one per transcoder
+	 * using the same bit definition: handle it as TRANSCODER_EDP to force
+	 * 0 shift in bit definition
+	 */
+	if (INTEL_GEN(dev_priv) >= 12) {
+		trans_shift = 0;
+		imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
+	} else {
+		trans_shift = dev_priv->psr.transcoder;
+		imr_reg = EDP_PSR_IMR;
+	}
+
+	mask = EDP_PSR_ERROR(trans_shift);
 	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
-		mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
+		mask |= EDP_PSR_POST_EXIT(trans_shift) |
+			EDP_PSR_PRE_ENTRY(trans_shift);
 
-	val = I915_READ(EDP_PSR_IMR);
-	val &= ~EDP_PSR_TRANS_MASK(trans);
+	val = I915_READ(imr_reg);
+	val &= ~EDP_PSR_TRANS_MASK(trans_shift);
 	val |= ~mask;
-	I915_WRITE(EDP_PSR_IMR, val);
+	I915_WRITE(imr_reg, val);
 }
 
 static void psr_event_print(u32 val, bool psr2_enabled)
@@ -143,15 +158,25 @@ static void psr_event_print(u32 val, bool psr2_enabled)
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 {
 	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
+	enum transcoder trans_shift;
+	i915_reg_t imr_reg;
 	ktime_t time_ns =  ktime_get();
 
-	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+	if (INTEL_GEN(dev_priv) >= 12) {
+		trans_shift = 0;
+		imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
+	} else {
+		trans_shift = dev_priv->psr.transcoder;
+		imr_reg = EDP_PSR_IMR;
+	}
+
+	if (psr_iir & EDP_PSR_PRE_ENTRY(trans_shift)) {
 		dev_priv->psr.last_entry_attempt = time_ns;
 		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
 			      transcoder_name(cpu_transcoder));
 	}
 
-	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+	if (psr_iir & EDP_PSR_POST_EXIT(trans_shift)) {
 		dev_priv->psr.last_exit = time_ns;
 		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
 			      transcoder_name(cpu_transcoder));
@@ -165,7 +190,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 		}
 	}
 
-	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+	if (psr_iir & EDP_PSR_ERROR(trans_shift)) {
 		u32 val;
 
 		DRM_WARN("[transcoder %s] PSR aux error\n",
@@ -181,9 +206,9 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 		 * again so we don't care about unmask the interruption
 		 * or unset irq_aux_error.
 		 */
-		val = I915_READ(EDP_PSR_IMR);
-		val |= EDP_PSR_ERROR(cpu_transcoder);
-		I915_WRITE(EDP_PSR_IMR, val);
+		val = I915_READ(imr_reg);
+		val |= EDP_PSR_ERROR(trans_shift);
+		I915_WRITE(imr_reg, val);
 
 		schedule_work(&dev_priv->psr.work);
 	}
@@ -730,8 +755,13 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
 	 * first time that PSR HW tries to activate so lets keep PSR disabled
 	 * to avoid any rendering problems.
 	 */
-	val = I915_READ(EDP_PSR_IIR);
-	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
+	if (INTEL_GEN(dev_priv) >= 12) {
+		val = I915_READ(TRANS_PSR_IIR(dev_priv->psr.transcoder));
+		val &= EDP_PSR_ERROR(0);
+	} else {
+		val = I915_READ(EDP_PSR_IIR);
+		val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
+	}
 	if (val) {
 		dev_priv->psr.sink_not_reliable = true;
 		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3f1b6ee157ba..73f1bd60bc68 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2656,11 +2656,21 @@ gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
 	}
 
 	if (iir & GEN8_DE_EDP_PSR) {
-		u32 psr_iir = I915_READ(EDP_PSR_IIR);
+		u32 psr_iir;
+		i915_reg_t iir_reg;
+
+		if (INTEL_GEN(dev_priv) >= 12)
+			iir_reg = TRANS_PSR_IIR(dev_priv->psr.transcoder);
+		else
+			iir_reg = EDP_PSR_IIR;
+
+		psr_iir = I915_READ(iir_reg);
+		I915_WRITE(iir_reg, psr_iir);
+
+		if (psr_iir)
+			found = true;
 
 		intel_psr_irq_handler(dev_priv, psr_iir);
-		I915_WRITE(EDP_PSR_IIR, psr_iir);
-		found = true;
 	}
 
 	if (!found)
@@ -3280,8 +3290,23 @@ static void gen11_irq_reset(struct drm_i915_private *dev_priv)
 
 	intel_uncore_write(uncore, GEN11_DISPLAY_INT_CTL, 0);
 
-	intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
-	intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
+	if (INTEL_GEN(dev_priv) >= 12) {
+		enum transcoder trans;
+
+		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
+			enum intel_display_power_domain domain;
+
+			domain = POWER_DOMAIN_TRANSCODER(trans);
+			if (!intel_display_power_is_enabled(dev_priv, domain))
+				continue;
+
+			intel_uncore_write(uncore, TRANS_PSR_IMR(trans), 0xffffffff);
+			intel_uncore_write(uncore, TRANS_PSR_IIR(trans), 0xffffffff);
+		}
+	} else {
+		intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
+		intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
+	}
 
 	for_each_pipe(dev_priv, pipe)
 		if (intel_display_power_is_enabled(dev_priv,
@@ -3794,7 +3819,21 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	else if (IS_BROADWELL(dev_priv))
 		de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
 
-	gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
+	if (INTEL_GEN(dev_priv) >= 12) {
+		enum transcoder trans;
+
+		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
+			enum intel_display_power_domain domain;
+
+			domain = POWER_DOMAIN_TRANSCODER(trans);
+			if (!intel_display_power_is_enabled(dev_priv, domain))
+				continue;
+
+			gen3_assert_iir_is_zero(uncore, TRANS_PSR_IIR(trans));
+		}
+	} else {
+		gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
+	}
 
 	for_each_pipe(dev_priv, pipe) {
 		dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6e7db9c65981..33d9d141ee8f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4222,9 +4222,17 @@ enum {
 #define   EDP_PSR_TP1_TIME_0us			(3 << 4)
 #define   EDP_PSR_IDLE_FRAME_SHIFT		0
 
-/* Bspec claims those aren't shifted but stay at 0x64800 */
+/*
+ * Until TGL, Bspec says IMR/IIR are fixed at 0x648xx. On TGL+ those registers
+ * are relative to transcoder and bits defined for each one as
+ * if using no shift (i.e. as if it was for TRANSCODER_EDP)
+ */
 #define EDP_PSR_IMR				_MMIO(0x64834)
 #define EDP_PSR_IIR				_MMIO(0x64838)
+#define _PSR_IMR_A				0x60814
+#define _PSR_IIR_A				0x60818
+#define TRANS_PSR_IMR(tran)			_MMIO_TRANS2(tran, _PSR_IMR_A)
+#define TRANS_PSR_IIR(tran)			_MMIO_TRANS2(tran, _PSR_IIR_A)
 #define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
 						 0 : ((trans) - TRANSCODER_A + 1) * 8)
 #define   EDP_PSR_TRANS_MASK(trans)		(0x7 << _EDP_PSR_TRANS_SHIFT(trans))
-- 
2.23.0

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

Re: [PATCH v3 2/7] drm/i915/tgl: Access the right register when handling PSR interruptions

[Matt Roper]

On Thu, Aug 29, 2019 at 02:25:49AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza@intel.com>
> 
> For older gens PSR IIR and IMR have fixed addresses. From TGL onwards those
> registers moved to each transcoder offset. The bits for the registers
> are defined without an offset per transcoder as right now we have one
> register per transcoder. So add a fake "trans_shift" when calculating
> the bits offsets: it will be 0 for gen12+ and psr.transcoder otherwise.
> 
> v2 (Lucas): change the implementation to use trans_shift instead of
> getting each bit value with a different macro
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 60 ++++++++++++++++++------
>  drivers/gpu/drm/i915/i915_irq.c          | 51 +++++++++++++++++---
>  drivers/gpu/drm/i915/i915_reg.h          | 10 +++-
>  3 files changed, 99 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 6f2bf50b6d80..e01c897ec9f9 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -90,17 +90,32 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  
>  static void psr_irq_control(struct drm_i915_private *dev_priv)
>  {
> -	enum transcoder trans = dev_priv->psr.transcoder;
> -	u32 val, mask;
> +	enum transcoder trans_shift;
> +	u32 mask, val;
> +	i915_reg_t imr_reg;
>  
> -	mask = EDP_PSR_ERROR(trans);
> +	/*
> +	 * gen12+ has registers relative to transcoder and one per transcoder
> +	 * using the same bit definition: handle it as TRANSCODER_EDP to force
> +	 * 0 shift in bit definition
> +	 */
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		trans_shift = 0;
> +		imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
> +	} else {
> +		trans_shift = dev_priv->psr.transcoder;
> +		imr_reg = EDP_PSR_IMR;
> +	}
> +
> +	mask = EDP_PSR_ERROR(trans_shift);
>  	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> -		mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
> +		mask |= EDP_PSR_POST_EXIT(trans_shift) |
> +			EDP_PSR_PRE_ENTRY(trans_shift);
>  
> -	val = I915_READ(EDP_PSR_IMR);
> -	val &= ~EDP_PSR_TRANS_MASK(trans);
> +	val = I915_READ(imr_reg);
> +	val &= ~EDP_PSR_TRANS_MASK(trans_shift);
>  	val |= ~mask;
> -	I915_WRITE(EDP_PSR_IMR, val);
> +	I915_WRITE(imr_reg, val);
>  }
>  
>  static void psr_event_print(u32 val, bool psr2_enabled)
> @@ -143,15 +158,25 @@ static void psr_event_print(u32 val, bool psr2_enabled)
>  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  {
>  	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> +	enum transcoder trans_shift;
> +	i915_reg_t imr_reg;
>  	ktime_t time_ns =  ktime_get();
>  
> -	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		trans_shift = 0;
> +		imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
> +	} else {
> +		trans_shift = dev_priv->psr.transcoder;
> +		imr_reg = EDP_PSR_IMR;
> +	}
> +
> +	if (psr_iir & EDP_PSR_PRE_ENTRY(trans_shift)) {
>  		dev_priv->psr.last_entry_attempt = time_ns;
>  		DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
>  			      transcoder_name(cpu_transcoder));
>  	}
>  
> -	if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> +	if (psr_iir & EDP_PSR_POST_EXIT(trans_shift)) {
>  		dev_priv->psr.last_exit = time_ns;
>  		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>  			      transcoder_name(cpu_transcoder));
> @@ -165,7 +190,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  		}
>  	}
>  
> -	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> +	if (psr_iir & EDP_PSR_ERROR(trans_shift)) {
>  		u32 val;
>  
>  		DRM_WARN("[transcoder %s] PSR aux error\n",
> @@ -181,9 +206,9 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  		 * again so we don't care about unmask the interruption
>  		 * or unset irq_aux_error.
>  		 */
> -		val = I915_READ(EDP_PSR_IMR);
> -		val |= EDP_PSR_ERROR(cpu_transcoder);
> -		I915_WRITE(EDP_PSR_IMR, val);
> +		val = I915_READ(imr_reg);
> +		val |= EDP_PSR_ERROR(trans_shift);
> +		I915_WRITE(imr_reg, val);
>  
>  		schedule_work(&dev_priv->psr.work);
>  	}
> @@ -730,8 +755,13 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>  	 * first time that PSR HW tries to activate so lets keep PSR disabled
>  	 * to avoid any rendering problems.
>  	 */
> -	val = I915_READ(EDP_PSR_IIR);
> -	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		val = I915_READ(TRANS_PSR_IIR(dev_priv->psr.transcoder));
> +		val &= EDP_PSR_ERROR(0);
> +	} else {
> +		val = I915_READ(EDP_PSR_IIR);
> +		val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> +	}
>  	if (val) {
>  		dev_priv->psr.sink_not_reliable = true;
>  		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3f1b6ee157ba..73f1bd60bc68 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2656,11 +2656,21 @@ gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
>  	}
>  
>  	if (iir & GEN8_DE_EDP_PSR) {
> -		u32 psr_iir = I915_READ(EDP_PSR_IIR);
> +		u32 psr_iir;
> +		i915_reg_t iir_reg;
> +
> +		if (INTEL_GEN(dev_priv) >= 12)
> +			iir_reg = TRANS_PSR_IIR(dev_priv->psr.transcoder);
> +		else
> +			iir_reg = EDP_PSR_IIR;
> +
> +		psr_iir = I915_READ(iir_reg);
> +		I915_WRITE(iir_reg, psr_iir);
> +
> +		if (psr_iir)
> +			found = true;
>  
>  		intel_psr_irq_handler(dev_priv, psr_iir);
> -		I915_WRITE(EDP_PSR_IIR, psr_iir);
> -		found = true;
>  	}
>  
>  	if (!found)
> @@ -3280,8 +3290,23 @@ static void gen11_irq_reset(struct drm_i915_private *dev_priv)
>  
>  	intel_uncore_write(uncore, GEN11_DISPLAY_INT_CTL, 0);
>  
> -	intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> -	intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		enum transcoder trans;
> +
> +		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> +			enum intel_display_power_domain domain;
> +
> +			domain = POWER_DOMAIN_TRANSCODER(trans);
> +			if (!intel_display_power_is_enabled(dev_priv, domain))
> +				continue;
> +
> +			intel_uncore_write(uncore, TRANS_PSR_IMR(trans), 0xffffffff);
> +			intel_uncore_write(uncore, TRANS_PSR_IIR(trans), 0xffffffff);
> +		}
> +	} else {
> +		intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> +		intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> +	}
>  
>  	for_each_pipe(dev_priv, pipe)
>  		if (intel_display_power_is_enabled(dev_priv,
> @@ -3794,7 +3819,21 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	else if (IS_BROADWELL(dev_priv))
>  		de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
>  
> -	gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		enum transcoder trans;
> +
> +		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> +			enum intel_display_power_domain domain;
> +
> +			domain = POWER_DOMAIN_TRANSCODER(trans);
> +			if (!intel_display_power_is_enabled(dev_priv, domain))
> +				continue;
> +
> +			gen3_assert_iir_is_zero(uncore, TRANS_PSR_IIR(trans));
> +		}
> +	} else {
> +		gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> +	}
>  
>  	for_each_pipe(dev_priv, pipe) {
>  		dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6e7db9c65981..33d9d141ee8f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4222,9 +4222,17 @@ enum {
>  #define   EDP_PSR_TP1_TIME_0us			(3 << 4)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
> -/* Bspec claims those aren't shifted but stay at 0x64800 */
> +/*
> + * Until TGL, Bspec says IMR/IIR are fixed at 0x648xx. On TGL+ those registers

The way these comments are written (both old and new) makes it sound
like we don't completely trust the bspec for some reason.  I'd just drop
the mention of the bspec and say "Until TGL, IMR/IIR are fixed..."

The patch itself looks correct, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> + * are relative to transcoder and bits defined for each one as
> + * if using no shift (i.e. as if it was for TRANSCODER_EDP)
> + */
>  #define EDP_PSR_IMR				_MMIO(0x64834)
>  #define EDP_PSR_IIR				_MMIO(0x64838)
> +#define _PSR_IMR_A				0x60814
> +#define _PSR_IIR_A				0x60818
> +#define TRANS_PSR_IMR(tran)			_MMIO_TRANS2(tran, _PSR_IMR_A)
> +#define TRANS_PSR_IIR(tran)			_MMIO_TRANS2(tran, _PSR_IIR_A)
>  #define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
>  						 0 : ((trans) - TRANSCODER_A + 1) * 8)
>  #define   EDP_PSR_TRANS_MASK(trans)		(0x7 << _EDP_PSR_TRANS_SHIFT(trans))
> -- 
> 2.23.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 3/7] drm/i915: protect access to DP_TP_* on non-dp

[Lucas De Marchi]

DP_TP_{CTL,STATUS} should only be programmed when the encoder is intel_dp.
Checking its current usages intel_disable_ddi_buf() is the only
offender, with other places being protected by checks like
pipe_config->fec_enable that is only set by intel_dp.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 3180dacb5be4..df3e4fe7e3e9 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3462,10 +3462,12 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
 		wait = true;
 	}
 
-	val = I915_READ(DP_TP_CTL(port));
-	val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
-	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
-	I915_WRITE(DP_TP_CTL(port), val);
+	if (intel_encoder_is_dp(encoder)) {
+		val = I915_READ(DP_TP_CTL(port));
+		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
+		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
+		I915_WRITE(DP_TP_CTL(port), val);
+	}
 
 	/* Disable FEC in DP Sink */
 	intel_ddi_disable_fec_state(encoder, crtc_state);
-- 
2.23.0

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

Re: [PATCH v3 3/7] drm/i915: protect access to DP_TP_* on non-dp

[Ville Syrjälä]

On Thu, Aug 29, 2019 at 02:25:50AM -0700, Lucas De Marchi wrote:
> DP_TP_{CTL,STATUS} should only be programmed when the encoder is intel_dp.
> Checking its current usages intel_disable_ddi_buf() is the only
> offender, with other places being protected by checks like
> pipe_config->fec_enable that is only set by intel_dp.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 3180dacb5be4..df3e4fe7e3e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3462,10 +3462,12 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
>  		wait = true;
>  	}
>  
> -	val = I915_READ(DP_TP_CTL(port));
> -	val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> -	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> -	I915_WRITE(DP_TP_CTL(port), val);
> +	if (intel_encoder_is_dp(encoder)) {

Doesn't really make sense to me. Either we just do it (because a DDI is
just a DDI so DP_TP_CTL does exist always), or we only do it when driving
DP and not when driving HDMI.

For the latter I would perhaps suggest moving all this extra junk out
from intel_disable_ddi_buf() into the DP specific code paths, leaving
just the actual DDI_BUF_CTL disable here.

> +		val = I915_READ(DP_TP_CTL(port));
> +		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> +		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> +		I915_WRITE(DP_TP_CTL(port), val);
> +	}
>  
>  	/* Disable FEC in DP Sink */
>  	intel_ddi_disable_fec_state(encoder, crtc_state);
> -- 
> 2.23.0

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

Re: [PATCH v3 3/7] drm/i915: protect access to DP_TP_* on non-dp

[Matt Roper]

On Thu, Aug 29, 2019 at 01:37:55PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 29, 2019 at 02:25:50AM -0700, Lucas De Marchi wrote:
> > DP_TP_{CTL,STATUS} should only be programmed when the encoder is intel_dp.
> > Checking its current usages intel_disable_ddi_buf() is the only
> > offender, with other places being protected by checks like
> > pipe_config->fec_enable that is only set by intel_dp.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 3180dacb5be4..df3e4fe7e3e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3462,10 +3462,12 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
> >  		wait = true;
> >  	}
> >  
> > -	val = I915_READ(DP_TP_CTL(port));
> > -	val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > -	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > -	I915_WRITE(DP_TP_CTL(port), val);
> > +	if (intel_encoder_is_dp(encoder)) {
> 
> Doesn't really make sense to me. Either we just do it (because a DDI is
> just a DDI so DP_TP_CTL does exist always), or we only do it when driving
> DP and not when driving HDMI.

I agree; I don't think there's a need to avoid program programming the
register just because we weren't previously in DP mode.

But I do question whether a RMW is necessary; it seems like just writing
a constant 0 to this register would be sufficient for the disable
sequence.


Matt

> 
> For the latter I would perhaps suggest moving all this extra junk out
> from intel_disable_ddi_buf() into the DP specific code paths, leaving
> just the actual DDI_BUF_CTL disable here.
> 
> > +		val = I915_READ(DP_TP_CTL(port));
> > +		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > +		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > +		I915_WRITE(DP_TP_CTL(port), val);
> > +	}
> >  
> >  	/* Disable FEC in DP Sink */
> >  	intel_ddi_disable_fec_state(encoder, crtc_state);
> > -- 
> > 2.23.0
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/7] drm/i915: protect access to DP_TP_* on non-dp

[Souza, Jose]

On Tue, 2019-09-03 at 10:16 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 01:37:55PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 29, 2019 at 02:25:50AM -0700, Lucas De Marchi wrote:
> > > DP_TP_{CTL,STATUS} should only be programmed when the encoder is
> > > intel_dp.
> > > Checking its current usages intel_disable_ddi_buf() is the only
> > > offender, with other places being protected by checks like
> > > pipe_config->fec_enable that is only set by intel_dp.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 3180dacb5be4..df3e4fe7e3e9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3462,10 +3462,12 @@ static void intel_disable_ddi_buf(struct
> > > intel_encoder *encoder,
> > >  		wait = true;
> > >  	}
> > >  
> > > -	val = I915_READ(DP_TP_CTL(port));
> > > -	val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > > -	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > +	if (intel_encoder_is_dp(encoder)) {
> > 
> > Doesn't really make sense to me. Either we just do it (because a
> > DDI is
> > just a DDI so DP_TP_CTL does exist always), or we only do it when
> > driving
> > DP and not when driving HDMI.
> 
> I agree; I don't think there's a need to avoid program programming
> the
> register just because we weren't previously in DP mode.

The problem of always programing DP_TP_CTL comes with TGL, when
DP_TP_CTL() moves to transcoder, see next patch: drm/i915/tgl: move
DP_TP_* to transcoder.

We are adding intel_dp->regs.dp_tp_ctl and initializing(this is
necessary for MST for SST we could keep the current approach) it in DP
paths, we could move it to intel_encoder or intel_digital_port and
initialized it for HDMI too but it would not make any sense for someone
reading HDMI sequences.

And to move this to a DP specific function would force us to create
another function to execute the last "wait DDI_BUF_CTL to idle".

BSpec: 53339 and 22243

Personally I prefer this patch solution but let me know your thoughts
after this explanation.

> 
> But I do question whether a RMW is necessary; it seems like just
> writing
> a constant 0 to this register would be sufficient for the disable
> sequence.
> 
> 
> Matt
> 
> > For the latter I would perhaps suggest moving all this extra junk
> > out
> > from intel_disable_ddi_buf() into the DP specific code paths,
> > leaving
> > just the actual DDI_BUF_CTL disable here.
> > 
> > > +		val = I915_READ(DP_TP_CTL(port));
> > > +		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > > +		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > +		I915_WRITE(DP_TP_CTL(port), val);
> > > +	}
> > >  
> > >  	/* Disable FEC in DP Sink */
> > >  	intel_ddi_disable_fec_state(encoder, crtc_state);
> > > -- 
> > > 2.23.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/7] drm/i915: protect access to DP_TP_* on non-dp

[Ville Syrjälä]

On Wed, Sep 04, 2019 at 12:45:35AM +0000, Souza, Jose wrote:
> On Tue, 2019-09-03 at 10:16 -0700, Matt Roper wrote:
> > On Thu, Aug 29, 2019 at 01:37:55PM +0300, Ville Syrjälä wrote:
> > > On Thu, Aug 29, 2019 at 02:25:50AM -0700, Lucas De Marchi wrote:
> > > > DP_TP_{CTL,STATUS} should only be programmed when the encoder is
> > > > intel_dp.
> > > > Checking its current usages intel_disable_ddi_buf() is the only
> > > > offender, with other places being protected by checks like
> > > > pipe_config->fec_enable that is only set by intel_dp.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 3180dacb5be4..df3e4fe7e3e9 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -3462,10 +3462,12 @@ static void intel_disable_ddi_buf(struct
> > > > intel_encoder *encoder,
> > > >  		wait = true;
> > > >  	}
> > > >  
> > > > -	val = I915_READ(DP_TP_CTL(port));
> > > > -	val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > > > -	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > > +	if (intel_encoder_is_dp(encoder)) {
> > > 
> > > Doesn't really make sense to me. Either we just do it (because a
> > > DDI is
> > > just a DDI so DP_TP_CTL does exist always), or we only do it when
> > > driving
> > > DP and not when driving HDMI.
> > 
> > I agree; I don't think there's a need to avoid program programming
> > the
> > register just because we weren't previously in DP mode.
> 
> The problem of always programing DP_TP_CTL comes with TGL, when
> DP_TP_CTL() moves to transcoder, see next patch: drm/i915/tgl: move
> DP_TP_* to transcoder.
> 
> We are adding intel_dp->regs.dp_tp_ctl and initializing(this is
> necessary for MST for SST we could keep the current approach) it in DP
> paths, we could move it to intel_encoder or intel_digital_port and
> initialized it for HDMI too but it would not make any sense for someone
> reading HDMI sequences.

I'm not 100% sold on intel_dp->regs thing. Eventually I'd like to
remove all that crud from intel_dp and instead just plumb the crtc
state everywhere. But let's go with it for now.

Hmm. I just glanced at the modeset sequence in the spec, and it doesn't
touch DP_TP_CTL for HDMI, so I guess we might as well follow that same
sequence here and just go with intel_crtc_has_dp_encoder() instead
of intel_encoder_is_dp().

> 
> And to move this to a DP specific function would force us to create
> another function to execute the last "wait DDI_BUF_CTL to idle".
> 
> BSpec: 53339 and 22243
> 
> Personally I prefer this patch solution but let me know your thoughts
> after this explanation.
> 
> > 
> > But I do question whether a RMW is necessary; it seems like just
> > writing
> > a constant 0 to this register would be sufficient for the disable
> > sequence.
> > 
> > 
> > Matt
> > 
> > > For the latter I would perhaps suggest moving all this extra junk
> > > out
> > > from intel_disable_ddi_buf() into the DP specific code paths,
> > > leaving
> > > just the actual DDI_BUF_CTL disable here.
> > > 
> > > > +		val = I915_READ(DP_TP_CTL(port));
> > > > +		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > > > +		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > > +		I915_WRITE(DP_TP_CTL(port), val);
> > > > +	}
> > > >  
> > > >  	/* Disable FEC in DP Sink */
> > > >  	intel_ddi_disable_fec_state(encoder, crtc_state);
> > > > -- 
> > > > 2.23.0
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel

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

[PATCH v3 4/7] drm/i915/tgl: move DP_TP_* to transcoder

[Lucas De Marchi]

Gen 12 onwards moves the DP_TP_* registers to be transcoder-based rather
than port-based. This adds the new register addresses and changes all
the callers to use the register saved in intel_dp->regs.*. This is
filled out when preparing to enable the port so we take into account if
we should use the transcoder or the port.

v2: reimplement by stashing the registers we want to access under
intel_dp->reg. Now they are initialized when enabling the port.
Ville suggested to store the transcoder to be used exclusively
by TGL+. After implementing I thought just storing the register directly
made it cleaner.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c      | 43 ++++++++++++-------
 .../drm/i915/display/intel_display_types.h    |  9 ++++
 drivers/gpu/drm/i915/display/intel_dp.c       | 13 +++---
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  8 ++--
 drivers/gpu/drm/i915/i915_reg.h               |  4 ++
 5 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index df3e4fe7e3e9..73f7a4b0f6c2 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3164,17 +3164,18 @@ static void intel_ddi_enable_fec(struct intel_encoder *encoder,
 				 const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = encoder->port;
+	struct intel_dp *intel_dp;
 	u32 val;
 
 	if (!crtc_state->fec_enable)
 		return;
 
-	val = I915_READ(DP_TP_CTL(port));
+	intel_dp = enc_to_intel_dp(&encoder->base);
+	val = I915_READ(intel_dp->regs.dp_tp_ctl);
 	val |= DP_TP_CTL_FEC_ENABLE;
-	I915_WRITE(DP_TP_CTL(port), val);
+	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
 
-	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
+	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
 				  DP_TP_STATUS_FEC_ENABLE_LIVE, 1))
 		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
 }
@@ -3183,16 +3184,17 @@ static void intel_ddi_disable_fec_state(struct intel_encoder *encoder,
 					const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = encoder->port;
+	struct intel_dp *intel_dp;
 	u32 val;
 
 	if (!crtc_state->fec_enable)
 		return;
 
-	val = I915_READ(DP_TP_CTL(port));
+	intel_dp = enc_to_intel_dp(&encoder->base);
+	val = I915_READ(intel_dp->regs.dp_tp_ctl);
 	val &= ~DP_TP_CTL_FEC_ENABLE;
-	I915_WRITE(DP_TP_CTL(port), val);
-	POSTING_READ(DP_TP_CTL(port));
+	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
+	POSTING_READ(intel_dp->regs.dp_tp_ctl);
 }
 
 static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
@@ -3205,10 +3207,14 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
 	int level = intel_ddi_dp_level(intel_dp);
+	enum transcoder transcoder = crtc_state->cpu_transcoder;
 
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
 				 crtc_state->lane_count, is_mst);
 
+	intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(transcoder);
+	intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(transcoder);
+
 	/* 1.a got on intel_atomic_commit_tail() */
 
 	/* 2. */
@@ -3297,6 +3303,9 @@ static void hsw_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
 				 crtc_state->lane_count, is_mst);
 
+	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
+	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
+
 	intel_edp_panel_on(intel_dp);
 
 	intel_ddi_clk_select(encoder, crtc_state);
@@ -3463,10 +3472,12 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
 	}
 
 	if (intel_encoder_is_dp(encoder)) {
-		val = I915_READ(DP_TP_CTL(port));
+		struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+
+		val = I915_READ(intel_dp->regs.dp_tp_ctl);
 		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
 		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
-		I915_WRITE(DP_TP_CTL(port), val);
+		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
 	}
 
 	/* Disable FEC in DP Sink */
@@ -3895,7 +3906,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 	u32 val;
 	bool wait = false;
 
-	if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
+	if (I915_READ(intel_dp->regs.dp_tp_ctl) & DP_TP_CTL_ENABLE) {
 		val = I915_READ(DDI_BUF_CTL(port));
 		if (val & DDI_BUF_CTL_ENABLE) {
 			val &= ~DDI_BUF_CTL_ENABLE;
@@ -3903,11 +3914,11 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 			wait = true;
 		}
 
-		val = I915_READ(DP_TP_CTL(port));
+		val = I915_READ(intel_dp->regs.dp_tp_ctl);
 		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
 		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
-		I915_WRITE(DP_TP_CTL(port), val);
-		POSTING_READ(DP_TP_CTL(port));
+		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
+		POSTING_READ(intel_dp->regs.dp_tp_ctl);
 
 		if (wait)
 			intel_wait_ddi_buf_idle(dev_priv, port);
@@ -3922,8 +3933,8 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
 			val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 	}
-	I915_WRITE(DP_TP_CTL(port), val);
-	POSTING_READ(DP_TP_CTL(port));
+	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
+	POSTING_READ(intel_dp->regs.dp_tp_ctl);
 
 	intel_dp->DP |= DDI_BUF_CTL_ENABLE;
 	I915_WRITE(DDI_BUF_CTL(port), intel_dp->DP);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 96514dcc7812..3745553ac3ec 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1211,6 +1211,15 @@ struct intel_dp {
 	bool can_mst; /* this port supports mst */
 	bool is_mst;
 	int active_mst_links;
+
+	/*
+	 * DP_TP_* registers may be either on port or transcoder register space.
+	 */
+	struct {
+		i915_reg_t dp_tp_ctl;
+		i915_reg_t dp_tp_status;
+	} regs;
+
 	/* connector directly attached - won't be use for modeset in mst world */
 	struct intel_connector *attached_connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 00c3752fa197..938e6e7cccf1 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2288,6 +2288,9 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 				 intel_crtc_has_type(pipe_config,
 						     INTEL_OUTPUT_DP_MST));
 
+	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
+	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
+
 	/*
 	 * There are four kinds of DP registers:
 	 *
@@ -3237,7 +3240,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 			      dp_train_pat & train_pat_mask);
 
 	if (HAS_DDI(dev_priv)) {
-		u32 temp = I915_READ(DP_TP_CTL(port));
+		u32 temp = I915_READ(intel_dp->regs.dp_tp_ctl);
 
 		if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
 			temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
@@ -3263,7 +3266,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 			temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
 			break;
 		}
-		I915_WRITE(DP_TP_CTL(port), temp);
+		I915_WRITE(intel_dp->regs.dp_tp_ctl, temp);
 
 	} else if ((IS_IVYBRIDGE(dev_priv) && port == PORT_A) ||
 		   (HAS_PCH_CPT(dev_priv) && port != PORT_A)) {
@@ -3961,10 +3964,10 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 	if (!HAS_DDI(dev_priv))
 		return;
 
-	val = I915_READ(DP_TP_CTL(port));
+	val = I915_READ(intel_dp->regs.dp_tp_ctl);
 	val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
 	val |= DP_TP_CTL_LINK_TRAIN_IDLE;
-	I915_WRITE(DP_TP_CTL(port), val);
+	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
 
 	/*
 	 * Until TGL on PORT_A we can have only eDP in SST mode. There the only
@@ -3976,7 +3979,7 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 	if (port == PORT_A && INTEL_GEN(dev_priv) < 12)
 		return;
 
-	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
+	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
 				  DP_TP_STATUS_IDLE_DONE, 1))
 		DRM_ERROR("Timed out waiting for DP idle patterns\n");
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2c5ac3dd647f..2774126ca9ac 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -287,7 +287,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = intel_dig_port->base.port;
 	struct intel_connector *connector =
 		to_intel_connector(conn_state->connector);
 	int ret;
@@ -318,8 +317,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 		DRM_ERROR("failed to allocate vcpi\n");
 
 	intel_dp->active_mst_links++;
-	temp = I915_READ(DP_TP_STATUS(port));
-	I915_WRITE(DP_TP_STATUS(port), temp);
+	temp = I915_READ(intel_dp->regs.dp_tp_status);
+	I915_WRITE(intel_dp->regs.dp_tp_status, temp);
 
 	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
 
@@ -334,11 +333,10 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = intel_dig_port->base.port;
 
 	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
-	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
+	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
 				  DP_TP_STATUS_ACT_SENT, 1))
 		DRM_ERROR("Timed out waiting for ACT sent\n");
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 33d9d141ee8f..1c8fab11ed9d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9451,7 +9451,9 @@ enum skl_power_gate {
 /* DisplayPort Transport Control */
 #define _DP_TP_CTL_A			0x64040
 #define _DP_TP_CTL_B			0x64140
+#define _TGL_DP_TP_CTL_A		0x60540
 #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
+#define TGL_DP_TP_CTL(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_CTL_A)
 #define  DP_TP_CTL_ENABLE			(1 << 31)
 #define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
 #define  DP_TP_CTL_MODE_SST			(0 << 27)
@@ -9471,7 +9473,9 @@ enum skl_power_gate {
 /* DisplayPort Transport Status */
 #define _DP_TP_STATUS_A			0x64044
 #define _DP_TP_STATUS_B			0x64144
+#define _TGL_DP_TP_STATUS_A		0x60544
 #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
+#define TGL_DP_TP_STATUS(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_STATUS_A)
 #define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
 #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
 #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
-- 
2.23.0

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

Re: [PATCH v3 4/7] drm/i915/tgl: move DP_TP_* to transcoder

[Matt Roper]

On Thu, Aug 29, 2019 at 02:25:51AM -0700, Lucas De Marchi wrote:
> Gen 12 onwards moves the DP_TP_* registers to be transcoder-based rather
> than port-based. This adds the new register addresses and changes all
> the callers to use the register saved in intel_dp->regs.*. This is
> filled out when preparing to enable the port so we take into account if
> we should use the transcoder or the port.
> 
> v2: reimplement by stashing the registers we want to access under
> intel_dp->reg. Now they are initialized when enabling the port.
> Ville suggested to store the transcoder to be used exclusively
> by TGL+. After implementing I thought just storing the register directly
> made it cleaner.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Should we replace the direct usages of DP_TP_CTL DP_TP_STATUS in
hsw_fdi_link_train with as well for consistency?  That code is specific
to HSW/BDW so it doesn't cause a problem, but there's always the risk
that it might get copy/pasted somewhere else where the direct register
usage is wrong.

Otherwise,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c      | 43 ++++++++++++-------
>  .../drm/i915/display/intel_display_types.h    |  9 ++++
>  drivers/gpu/drm/i915/display/intel_dp.c       | 13 +++---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  8 ++--
>  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
>  5 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index df3e4fe7e3e9..73f7a4b0f6c2 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3164,17 +3164,18 @@ static void intel_ddi_enable_fec(struct intel_encoder *encoder,
>  				 const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = encoder->port;
> +	struct intel_dp *intel_dp;
>  	u32 val;
>  
>  	if (!crtc_state->fec_enable)
>  		return;
>  
> -	val = I915_READ(DP_TP_CTL(port));
> +	intel_dp = enc_to_intel_dp(&encoder->base);
> +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
>  	val |= DP_TP_CTL_FEC_ENABLE;
> -	I915_WRITE(DP_TP_CTL(port), val);
> +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
>  
> -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> +	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
>  				  DP_TP_STATUS_FEC_ENABLE_LIVE, 1))
>  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
>  }
> @@ -3183,16 +3184,17 @@ static void intel_ddi_disable_fec_state(struct intel_encoder *encoder,
>  					const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = encoder->port;
> +	struct intel_dp *intel_dp;
>  	u32 val;
>  
>  	if (!crtc_state->fec_enable)
>  		return;
>  
> -	val = I915_READ(DP_TP_CTL(port));
> +	intel_dp = enc_to_intel_dp(&encoder->base);
> +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
>  	val &= ~DP_TP_CTL_FEC_ENABLE;
> -	I915_WRITE(DP_TP_CTL(port), val);
> -	POSTING_READ(DP_TP_CTL(port));
> +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
>  }
>  
>  static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
> @@ -3205,10 +3207,14 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>  	int level = intel_ddi_dp_level(intel_dp);
> +	enum transcoder transcoder = crtc_state->cpu_transcoder;
>  
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> +	intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(transcoder);
> +	intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(transcoder);
> +
>  	/* 1.a got on intel_atomic_commit_tail() */
>  
>  	/* 2. */
> @@ -3297,6 +3303,9 @@ static void hsw_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> +
>  	intel_edp_panel_on(intel_dp);
>  
>  	intel_ddi_clk_select(encoder, crtc_state);
> @@ -3463,10 +3472,12 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
>  	}
>  
>  	if (intel_encoder_is_dp(encoder)) {
> -		val = I915_READ(DP_TP_CTL(port));
> +		struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
>  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
>  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> -		I915_WRITE(DP_TP_CTL(port), val);
> +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
>  	}
>  
>  	/* Disable FEC in DP Sink */
> @@ -3895,7 +3906,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  	u32 val;
>  	bool wait = false;
>  
> -	if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
> +	if (I915_READ(intel_dp->regs.dp_tp_ctl) & DP_TP_CTL_ENABLE) {
>  		val = I915_READ(DDI_BUF_CTL(port));
>  		if (val & DDI_BUF_CTL_ENABLE) {
>  			val &= ~DDI_BUF_CTL_ENABLE;
> @@ -3903,11 +3914,11 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  			wait = true;
>  		}
>  
> -		val = I915_READ(DP_TP_CTL(port));
> +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
>  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
>  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> -		I915_WRITE(DP_TP_CTL(port), val);
> -		POSTING_READ(DP_TP_CTL(port));
> +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> +		POSTING_READ(intel_dp->regs.dp_tp_ctl);
>  
>  		if (wait)
>  			intel_wait_ddi_buf_idle(dev_priv, port);
> @@ -3922,8 +3933,8 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>  			val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
>  	}
> -	I915_WRITE(DP_TP_CTL(port), val);
> -	POSTING_READ(DP_TP_CTL(port));
> +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
>  
>  	intel_dp->DP |= DDI_BUF_CTL_ENABLE;
>  	I915_WRITE(DDI_BUF_CTL(port), intel_dp->DP);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 96514dcc7812..3745553ac3ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1211,6 +1211,15 @@ struct intel_dp {
>  	bool can_mst; /* this port supports mst */
>  	bool is_mst;
>  	int active_mst_links;
> +
> +	/*
> +	 * DP_TP_* registers may be either on port or transcoder register space.
> +	 */
> +	struct {
> +		i915_reg_t dp_tp_ctl;
> +		i915_reg_t dp_tp_status;
> +	} regs;
> +
>  	/* connector directly attached - won't be use for modeset in mst world */
>  	struct intel_connector *attached_connector;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 00c3752fa197..938e6e7cccf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2288,6 +2288,9 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  				 intel_crtc_has_type(pipe_config,
>  						     INTEL_OUTPUT_DP_MST));
>  
> +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> +
>  	/*
>  	 * There are four kinds of DP registers:
>  	 *
> @@ -3237,7 +3240,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
>  			      dp_train_pat & train_pat_mask);
>  
>  	if (HAS_DDI(dev_priv)) {
> -		u32 temp = I915_READ(DP_TP_CTL(port));
> +		u32 temp = I915_READ(intel_dp->regs.dp_tp_ctl);
>  
>  		if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
>  			temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> @@ -3263,7 +3266,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
>  			temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
>  			break;
>  		}
> -		I915_WRITE(DP_TP_CTL(port), temp);
> +		I915_WRITE(intel_dp->regs.dp_tp_ctl, temp);
>  
>  	} else if ((IS_IVYBRIDGE(dev_priv) && port == PORT_A) ||
>  		   (HAS_PCH_CPT(dev_priv) && port != PORT_A)) {
> @@ -3961,10 +3964,10 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>  	if (!HAS_DDI(dev_priv))
>  		return;
>  
> -	val = I915_READ(DP_TP_CTL(port));
> +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
>  	val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
>  	val |= DP_TP_CTL_LINK_TRAIN_IDLE;
> -	I915_WRITE(DP_TP_CTL(port), val);
> +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
>  
>  	/*
>  	 * Until TGL on PORT_A we can have only eDP in SST mode. There the only
> @@ -3976,7 +3979,7 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>  	if (port == PORT_A && INTEL_GEN(dev_priv) < 12)
>  		return;
>  
> -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> +	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
>  				  DP_TP_STATUS_IDLE_DONE, 1))
>  		DRM_ERROR("Timed out waiting for DP idle patterns\n");
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2c5ac3dd647f..2774126ca9ac 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -287,7 +287,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_dig_port->base.port;
>  	struct intel_connector *connector =
>  		to_intel_connector(conn_state->connector);
>  	int ret;
> @@ -318,8 +317,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  		DRM_ERROR("failed to allocate vcpi\n");
>  
>  	intel_dp->active_mst_links++;
> -	temp = I915_READ(DP_TP_STATUS(port));
> -	I915_WRITE(DP_TP_STATUS(port), temp);
> +	temp = I915_READ(intel_dp->regs.dp_tp_status);
> +	I915_WRITE(intel_dp->regs.dp_tp_status, temp);
>  
>  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
>  
> @@ -334,11 +333,10 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
>  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_dig_port->base.port;
>  
>  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  
> -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> +	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
>  				  DP_TP_STATUS_ACT_SENT, 1))
>  		DRM_ERROR("Timed out waiting for ACT sent\n");
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 33d9d141ee8f..1c8fab11ed9d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9451,7 +9451,9 @@ enum skl_power_gate {
>  /* DisplayPort Transport Control */
>  #define _DP_TP_CTL_A			0x64040
>  #define _DP_TP_CTL_B			0x64140
> +#define _TGL_DP_TP_CTL_A		0x60540
>  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> +#define TGL_DP_TP_CTL(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_CTL_A)
>  #define  DP_TP_CTL_ENABLE			(1 << 31)
>  #define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> @@ -9471,7 +9473,9 @@ enum skl_power_gate {
>  /* DisplayPort Transport Status */
>  #define _DP_TP_STATUS_A			0x64044
>  #define _DP_TP_STATUS_B			0x64144
> +#define _TGL_DP_TP_STATUS_A		0x60544
>  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> +#define TGL_DP_TP_STATUS(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_STATUS_A)
>  #define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
>  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> -- 
> 2.23.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/7] drm/i915/tgl: move DP_TP_* to transcoder

[Souza, Jose]

On Tue, 2019-09-03 at 10:55 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 02:25:51AM -0700, Lucas De Marchi wrote:
> > Gen 12 onwards moves the DP_TP_* registers to be transcoder-based
> > rather
> > than port-based. This adds the new register addresses and changes
> > all
> > the callers to use the register saved in intel_dp->regs.*. This is
> > filled out when preparing to enable the port so we take into
> > account if
> > we should use the transcoder or the port.
> > 
> > v2: reimplement by stashing the registers we want to access under
> > intel_dp->reg. Now they are initialized when enabling the port.
> > Ville suggested to store the transcoder to be used exclusively
> > by TGL+. After implementing I thought just storing the register
> > directly
> > made it cleaner.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Should we replace the direct usages of DP_TP_CTL DP_TP_STATUS in
> hsw_fdi_link_train with as well for consistency?  That code is
> specific
> to HSW/BDW so it doesn't cause a problem, but there's always the risk
> that it might get copy/pasted somewhere else where the direct
> register
> usage is wrong.

Lucas was probably afraid of break it as it is a special case that only
works over PORT_E but we are setting regs.dp_tp_ctl to PORT_E in that
code path, so replacing it.

Thanks

> 
> Otherwise,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c      | 43 ++++++++++++---
> > ----
> >  .../drm/i915/display/intel_display_types.h    |  9 ++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       | 13 +++---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  8 ++--
> >  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
> >  5 files changed, 51 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index df3e4fe7e3e9..73f7a4b0f6c2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3164,17 +3164,18 @@ static void intel_ddi_enable_fec(struct
> > intel_encoder *encoder,
> >  				 const struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > -	enum port port = encoder->port;
> > +	struct intel_dp *intel_dp;
> >  	u32 val;
> >  
> >  	if (!crtc_state->fec_enable)
> >  		return;
> >  
> > -	val = I915_READ(DP_TP_CTL(port));
> > +	intel_dp = enc_to_intel_dp(&encoder->base);
> > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  	val |= DP_TP_CTL_FEC_ENABLE;
> > -	I915_WRITE(DP_TP_CTL(port), val);
> > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> >  
> > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > >regs.dp_tp_status,
> >  				  DP_TP_STATUS_FEC_ENABLE_LIVE, 1))
> >  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> >  }
> > @@ -3183,16 +3184,17 @@ static void
> > intel_ddi_disable_fec_state(struct intel_encoder *encoder,
> >  					const struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > -	enum port port = encoder->port;
> > +	struct intel_dp *intel_dp;
> >  	u32 val;
> >  
> >  	if (!crtc_state->fec_enable)
> >  		return;
> >  
> > -	val = I915_READ(DP_TP_CTL(port));
> > +	intel_dp = enc_to_intel_dp(&encoder->base);
> > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  	val &= ~DP_TP_CTL_FEC_ENABLE;
> > -	I915_WRITE(DP_TP_CTL(port), val);
> > -	POSTING_READ(DP_TP_CTL(port));
> > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
> >  }
> >  
> >  static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > @@ -3205,10 +3207,14 @@ static void tgl_ddi_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder-
> > >base);
> >  	bool is_mst = intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_DP_MST);
> >  	int level = intel_ddi_dp_level(intel_dp);
> > +	enum transcoder transcoder = crtc_state->cpu_transcoder;
> >  
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >  				 crtc_state->lane_count, is_mst);
> >  
> > +	intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(transcoder);
> > +	intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(transcoder);
> > +
> >  	/* 1.a got on intel_atomic_commit_tail() */
> >  
> >  	/* 2. */
> > @@ -3297,6 +3303,9 @@ static void hsw_ddi_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >  				 crtc_state->lane_count, is_mst);
> >  
> > +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > +
> >  	intel_edp_panel_on(intel_dp);
> >  
> >  	intel_ddi_clk_select(encoder, crtc_state);
> > @@ -3463,10 +3472,12 @@ static void intel_disable_ddi_buf(struct
> > intel_encoder *encoder,
> >  	}
> >  
> >  	if (intel_encoder_is_dp(encoder)) {
> > -		val = I915_READ(DP_TP_CTL(port));
> > +		struct intel_dp *intel_dp = enc_to_intel_dp(&encoder-
> > >base);
> > +
> > +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> >  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > -		I915_WRITE(DP_TP_CTL(port), val);
> > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> >  	}
> >  
> >  	/* Disable FEC in DP Sink */
> > @@ -3895,7 +3906,7 @@ static void
> > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> >  	u32 val;
> >  	bool wait = false;
> >  
> > -	if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
> > +	if (I915_READ(intel_dp->regs.dp_tp_ctl) & DP_TP_CTL_ENABLE) {
> >  		val = I915_READ(DDI_BUF_CTL(port));
> >  		if (val & DDI_BUF_CTL_ENABLE) {
> >  			val &= ~DDI_BUF_CTL_ENABLE;
> > @@ -3903,11 +3914,11 @@ static void
> > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> >  			wait = true;
> >  		}
> >  
> > -		val = I915_READ(DP_TP_CTL(port));
> > +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> >  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > -		I915_WRITE(DP_TP_CTL(port), val);
> > -		POSTING_READ(DP_TP_CTL(port));
> > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > +		POSTING_READ(intel_dp->regs.dp_tp_ctl);
> >  
> >  		if (wait)
> >  			intel_wait_ddi_buf_idle(dev_priv, port);
> > @@ -3922,8 +3933,8 @@ static void
> > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> >  		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> >  			val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
> >  	}
> > -	I915_WRITE(DP_TP_CTL(port), val);
> > -	POSTING_READ(DP_TP_CTL(port));
> > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
> >  
> >  	intel_dp->DP |= DDI_BUF_CTL_ENABLE;
> >  	I915_WRITE(DDI_BUF_CTL(port), intel_dp->DP);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 96514dcc7812..3745553ac3ec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1211,6 +1211,15 @@ struct intel_dp {
> >  	bool can_mst; /* this port supports mst */
> >  	bool is_mst;
> >  	int active_mst_links;
> > +
> > +	/*
> > +	 * DP_TP_* registers may be either on port or transcoder
> > register space.
> > +	 */
> > +	struct {
> > +		i915_reg_t dp_tp_ctl;
> > +		i915_reg_t dp_tp_status;
> > +	} regs;
> > +
> >  	/* connector directly attached - won't be use for modeset in
> > mst world */
> >  	struct intel_connector *attached_connector;
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 00c3752fa197..938e6e7cccf1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2288,6 +2288,9 @@ static void intel_dp_prepare(struct
> > intel_encoder *encoder,
> >  				 intel_crtc_has_type(pipe_config,
> >  						     INTEL_OUTPUT_DP_MS
> > T));
> >  
> > +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > +
> >  	/*
> >  	 * There are four kinds of DP registers:
> >  	 *
> > @@ -3237,7 +3240,7 @@ _intel_dp_set_link_train(struct intel_dp
> > *intel_dp,
> >  			      dp_train_pat & train_pat_mask);
> >  
> >  	if (HAS_DDI(dev_priv)) {
> > -		u32 temp = I915_READ(DP_TP_CTL(port));
> > +		u32 temp = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  
> >  		if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
> >  			temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> > @@ -3263,7 +3266,7 @@ _intel_dp_set_link_train(struct intel_dp
> > *intel_dp,
> >  			temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
> >  			break;
> >  		}
> > -		I915_WRITE(DP_TP_CTL(port), temp);
> > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, temp);
> >  
> >  	} else if ((IS_IVYBRIDGE(dev_priv) && port == PORT_A) ||
> >  		   (HAS_PCH_CPT(dev_priv) && port != PORT_A)) {
> > @@ -3961,10 +3964,10 @@ void intel_dp_set_idle_link_train(struct
> > intel_dp *intel_dp)
> >  	if (!HAS_DDI(dev_priv))
> >  		return;
> >  
> > -	val = I915_READ(DP_TP_CTL(port));
> > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> >  	val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> >  	val |= DP_TP_CTL_LINK_TRAIN_IDLE;
> > -	I915_WRITE(DP_TP_CTL(port), val);
> > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> >  
> >  	/*
> >  	 * Until TGL on PORT_A we can have only eDP in SST mode. There
> > the only
> > @@ -3976,7 +3979,7 @@ void intel_dp_set_idle_link_train(struct
> > intel_dp *intel_dp)
> >  	if (port == PORT_A && INTEL_GEN(dev_priv) < 12)
> >  		return;
> >  
> > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > >regs.dp_tp_status,
> >  				  DP_TP_STATUS_IDLE_DONE, 1))
> >  		DRM_ERROR("Timed out waiting for DP idle patterns\n");
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 2c5ac3dd647f..2774126ca9ac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -287,7 +287,6 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > -	enum port port = intel_dig_port->base.port;
> >  	struct intel_connector *connector =
> >  		to_intel_connector(conn_state->connector);
> >  	int ret;
> > @@ -318,8 +317,8 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  		DRM_ERROR("failed to allocate vcpi\n");
> >  
> >  	intel_dp->active_mst_links++;
> > -	temp = I915_READ(DP_TP_STATUS(port));
> > -	I915_WRITE(DP_TP_STATUS(port), temp);
> > +	temp = I915_READ(intel_dp->regs.dp_tp_status);
> > +	I915_WRITE(intel_dp->regs.dp_tp_status, temp);
> >  
> >  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> >  
> > @@ -334,11 +333,10 @@ static void intel_mst_enable_dp(struct
> > intel_encoder *encoder,
> >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > -	enum port port = intel_dig_port->base.port;
> >  
> >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >  
> > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > >regs.dp_tp_status,
> >  				  DP_TP_STATUS_ACT_SENT, 1))
> >  		DRM_ERROR("Timed out waiting for ACT sent\n");
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 33d9d141ee8f..1c8fab11ed9d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9451,7 +9451,9 @@ enum skl_power_gate {
> >  /* DisplayPort Transport Control */
> >  #define _DP_TP_CTL_A			0x64040
> >  #define _DP_TP_CTL_B			0x64140
> > +#define _TGL_DP_TP_CTL_A		0x60540
> >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> > _DP_TP_CTL_B)
> > +#define TGL_DP_TP_CTL(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_CTL_A)
> >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> >  #define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > @@ -9471,7 +9473,9 @@ enum skl_power_gate {
> >  /* DisplayPort Transport Status */
> >  #define _DP_TP_STATUS_A			0x64044
> >  #define _DP_TP_STATUS_B			0x64144
> > +#define _TGL_DP_TP_STATUS_A		0x60544
> >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> > _DP_TP_STATUS_B)
> > +#define TGL_DP_TP_STATUS(tran) _MMIO_TRANS2((tran),
> > _TGL_DP_TP_STATUS_A)
> >  #define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > -- 
> > 2.23.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/7] drm/i915/tgl: move DP_TP_* to transcoder

[Souza, Jose]

On Wed, 2019-09-04 at 20:44 +0000, Souza, Jose wrote:
> On Tue, 2019-09-03 at 10:55 -0700, Matt Roper wrote:
> > On Thu, Aug 29, 2019 at 02:25:51AM -0700, Lucas De Marchi wrote:
> > > Gen 12 onwards moves the DP_TP_* registers to be transcoder-based
> > > rather
> > > than port-based. This adds the new register addresses and changes
> > > all
> > > the callers to use the register saved in intel_dp->regs.*. This
> > > is
> > > filled out when preparing to enable the port so we take into
> > > account if
> > > we should use the transcoder or the port.
> > > 
> > > v2: reimplement by stashing the registers we want to access under
> > > intel_dp->reg. Now they are initialized when enabling the port.
> > > Ville suggested to store the transcoder to be used exclusively
> > > by TGL+. After implementing I thought just storing the register
> > > directly
> > > made it cleaner.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> > Should we replace the direct usages of DP_TP_CTL DP_TP_STATUS in
> > hsw_fdi_link_train with as well for consistency?  That code is
> > specific
> > to HSW/BDW so it doesn't cause a problem, but there's always the
> > risk
> > that it might get copy/pasted somewhere else where the direct
> > register
> > usage is wrong.
> 
> Lucas was probably afraid of break it as it is a special case that
> only
> works over PORT_E but we are setting regs.dp_tp_ctl to PORT_E in that
> code path, so replacing it.

Actually no, it is not a DP port in i915 but the hardware output is.
See intel_crt_init(), the type is even set to INTEL_OUTPUT_ANALOG.

So not changing it, also it is unlikely that future GENs will go back
to this FDI monster.

> 
> Thanks
> 
> > Otherwise,
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > 
> > Matt
> > 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c      | 43 ++++++++++++-
> > > --
> > > ----
> > >  .../drm/i915/display/intel_display_types.h    |  9 ++++
> > >  drivers/gpu/drm/i915/display/intel_dp.c       | 13 +++---
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  8 ++--
> > >  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
> > >  5 files changed, 51 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index df3e4fe7e3e9..73f7a4b0f6c2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3164,17 +3164,18 @@ static void intel_ddi_enable_fec(struct
> > > intel_encoder *encoder,
> > >  				 const struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = encoder->port;
> > > +	struct intel_dp *intel_dp;
> > >  	u32 val;
> > >  
> > >  	if (!crtc_state->fec_enable)
> > >  		return;
> > >  
> > > -	val = I915_READ(DP_TP_CTL(port));
> > > +	intel_dp = enc_to_intel_dp(&encoder->base);
> > > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  	val |= DP_TP_CTL_FEC_ENABLE;
> > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > >  
> > > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > > > regs.dp_tp_status,
> > >  				  DP_TP_STATUS_FEC_ENABLE_LIVE, 1))
> > >  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > >  }
> > > @@ -3183,16 +3184,17 @@ static void
> > > intel_ddi_disable_fec_state(struct intel_encoder *encoder,
> > >  					const struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = encoder->port;
> > > +	struct intel_dp *intel_dp;
> > >  	u32 val;
> > >  
> > >  	if (!crtc_state->fec_enable)
> > >  		return;
> > >  
> > > -	val = I915_READ(DP_TP_CTL(port));
> > > +	intel_dp = enc_to_intel_dp(&encoder->base);
> > > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  	val &= ~DP_TP_CTL_FEC_ENABLE;
> > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > -	POSTING_READ(DP_TP_CTL(port));
> > > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > > +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
> > >  }
> > >  
> > >  static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > @@ -3205,10 +3207,14 @@ static void tgl_ddi_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder-
> > > > base);
> > >  	bool is_mst = intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_DP_MST);
> > >  	int level = intel_ddi_dp_level(intel_dp);
> > > +	enum transcoder transcoder = crtc_state->cpu_transcoder;
> > >  
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >  				 crtc_state->lane_count, is_mst);
> > >  
> > > +	intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(transcoder);
> > > +	intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(transcoder);
> > > +
> > >  	/* 1.a got on intel_atomic_commit_tail() */
> > >  
> > >  	/* 2. */
> > > @@ -3297,6 +3303,9 @@ static void hsw_ddi_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >  				 crtc_state->lane_count, is_mst);
> > >  
> > > +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > +
> > >  	intel_edp_panel_on(intel_dp);
> > >  
> > >  	intel_ddi_clk_select(encoder, crtc_state);
> > > @@ -3463,10 +3472,12 @@ static void intel_disable_ddi_buf(struct
> > > intel_encoder *encoder,
> > >  	}
> > >  
> > >  	if (intel_encoder_is_dp(encoder)) {
> > > -		val = I915_READ(DP_TP_CTL(port));
> > > +		struct intel_dp *intel_dp = enc_to_intel_dp(&encoder-
> > > > base);
> > > +
> > > +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > >  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > -		I915_WRITE(DP_TP_CTL(port), val);
> > > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > >  	}
> > >  
> > >  	/* Disable FEC in DP Sink */
> > > @@ -3895,7 +3906,7 @@ static void
> > > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > >  	u32 val;
> > >  	bool wait = false;
> > >  
> > > -	if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
> > > +	if (I915_READ(intel_dp->regs.dp_tp_ctl) & DP_TP_CTL_ENABLE) {
> > >  		val = I915_READ(DDI_BUF_CTL(port));
> > >  		if (val & DDI_BUF_CTL_ENABLE) {
> > >  			val &= ~DDI_BUF_CTL_ENABLE;
> > > @@ -3903,11 +3914,11 @@ static void
> > > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > >  			wait = true;
> > >  		}
> > >  
> > > -		val = I915_READ(DP_TP_CTL(port));
> > > +		val = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  		val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > >  		val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > > -		I915_WRITE(DP_TP_CTL(port), val);
> > > -		POSTING_READ(DP_TP_CTL(port));
> > > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > > +		POSTING_READ(intel_dp->regs.dp_tp_ctl);
> > >  
> > >  		if (wait)
> > >  			intel_wait_ddi_buf_idle(dev_priv, port);
> > > @@ -3922,8 +3933,8 @@ static void
> > > intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > >  		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> > >  			val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
> > >  	}
> > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > -	POSTING_READ(DP_TP_CTL(port));
> > > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > > +	POSTING_READ(intel_dp->regs.dp_tp_ctl);
> > >  
> > >  	intel_dp->DP |= DDI_BUF_CTL_ENABLE;
> > >  	I915_WRITE(DDI_BUF_CTL(port), intel_dp->DP);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 96514dcc7812..3745553ac3ec 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1211,6 +1211,15 @@ struct intel_dp {
> > >  	bool can_mst; /* this port supports mst */
> > >  	bool is_mst;
> > >  	int active_mst_links;
> > > +
> > > +	/*
> > > +	 * DP_TP_* registers may be either on port or transcoder
> > > register space.
> > > +	 */
> > > +	struct {
> > > +		i915_reg_t dp_tp_ctl;
> > > +		i915_reg_t dp_tp_status;
> > > +	} regs;
> > > +
> > >  	/* connector directly attached - won't be use for modeset in
> > > mst world */
> > >  	struct intel_connector *attached_connector;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 00c3752fa197..938e6e7cccf1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2288,6 +2288,9 @@ static void intel_dp_prepare(struct
> > > intel_encoder *encoder,
> > >  				 intel_crtc_has_type(pipe_config,
> > >  						     INTEL_OUTPUT_DP_MS
> > > T));
> > >  
> > > +	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > +	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > +
> > >  	/*
> > >  	 * There are four kinds of DP registers:
> > >  	 *
> > > @@ -3237,7 +3240,7 @@ _intel_dp_set_link_train(struct intel_dp
> > > *intel_dp,
> > >  			      dp_train_pat & train_pat_mask);
> > >  
> > >  	if (HAS_DDI(dev_priv)) {
> > > -		u32 temp = I915_READ(DP_TP_CTL(port));
> > > +		u32 temp = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  
> > >  		if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
> > >  			temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> > > @@ -3263,7 +3266,7 @@ _intel_dp_set_link_train(struct intel_dp
> > > *intel_dp,
> > >  			temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
> > >  			break;
> > >  		}
> > > -		I915_WRITE(DP_TP_CTL(port), temp);
> > > +		I915_WRITE(intel_dp->regs.dp_tp_ctl, temp);
> > >  
> > >  	} else if ((IS_IVYBRIDGE(dev_priv) && port == PORT_A) ||
> > >  		   (HAS_PCH_CPT(dev_priv) && port != PORT_A)) {
> > > @@ -3961,10 +3964,10 @@ void intel_dp_set_idle_link_train(struct
> > > intel_dp *intel_dp)
> > >  	if (!HAS_DDI(dev_priv))
> > >  		return;
> > >  
> > > -	val = I915_READ(DP_TP_CTL(port));
> > > +	val = I915_READ(intel_dp->regs.dp_tp_ctl);
> > >  	val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> > >  	val |= DP_TP_CTL_LINK_TRAIN_IDLE;
> > > -	I915_WRITE(DP_TP_CTL(port), val);
> > > +	I915_WRITE(intel_dp->regs.dp_tp_ctl, val);
> > >  
> > >  	/*
> > >  	 * Until TGL on PORT_A we can have only eDP in SST mode. There
> > > the only
> > > @@ -3976,7 +3979,7 @@ void intel_dp_set_idle_link_train(struct
> > > intel_dp *intel_dp)
> > >  	if (port == PORT_A && INTEL_GEN(dev_priv) < 12)
> > >  		return;
> > >  
> > > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > > > regs.dp_tp_status,
> > >  				  DP_TP_STATUS_IDLE_DONE, 1))
> > >  		DRM_ERROR("Timed out waiting for DP idle patterns\n");
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 2c5ac3dd647f..2774126ca9ac 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -287,7 +287,6 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = intel_dig_port->base.port;
> > >  	struct intel_connector *connector =
> > >  		to_intel_connector(conn_state->connector);
> > >  	int ret;
> > > @@ -318,8 +317,8 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  		DRM_ERROR("failed to allocate vcpi\n");
> > >  
> > >  	intel_dp->active_mst_links++;
> > > -	temp = I915_READ(DP_TP_STATUS(port));
> > > -	I915_WRITE(DP_TP_STATUS(port), temp);
> > > +	temp = I915_READ(intel_dp->regs.dp_tp_status);
> > > +	I915_WRITE(intel_dp->regs.dp_tp_status, temp);
> > >  
> > >  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> > >  
> > > @@ -334,11 +333,10 @@ static void intel_mst_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	enum port port = intel_dig_port->base.port;
> > >  
> > >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> > >  
> > > -	if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > > > regs.dp_tp_status,
> > >  				  DP_TP_STATUS_ACT_SENT, 1))
> > >  		DRM_ERROR("Timed out waiting for ACT sent\n");
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 33d9d141ee8f..1c8fab11ed9d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9451,7 +9451,9 @@ enum skl_power_gate {
> > >  /* DisplayPort Transport Control */
> > >  #define _DP_TP_CTL_A			0x64040
> > >  #define _DP_TP_CTL_B			0x64140
> > > +#define _TGL_DP_TP_CTL_A		0x60540
> > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> > > _DP_TP_CTL_B)
> > > +#define TGL_DP_TP_CTL(tran) _MMIO_TRANS2((tran),
> > > _TGL_DP_TP_CTL_A)
> > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > >  #define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > > @@ -9471,7 +9473,9 @@ enum skl_power_gate {
> > >  /* DisplayPort Transport Status */
> > >  #define _DP_TP_STATUS_A			0x64044
> > >  #define _DP_TP_STATUS_B			0x64144
> > > +#define _TGL_DP_TP_STATUS_A		0x60544
> > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> > > _DP_TP_STATUS_B)
> > > +#define TGL_DP_TP_STATUS(tran) _MMIO_TRANS2((tran),
> > > _TGL_DP_TP_STATUS_A)
> > >  #define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > > -- 
> > > 2.23.0
> > > 
> _______________________________________________
> 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

[PATCH v3 5/7] drm/i915/tgl: disable SAGV temporarily

[Lucas De Marchi]

SAGV is not currently working for Tiger Lake. We better disable it until
the implementation is stabilized and we can enable it.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4fa9bc83c8b4..7294fcf05323 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3654,6 +3654,10 @@ static bool skl_needs_memory_bw_wa(struct drm_i915_private *dev_priv)
 static bool
 intel_has_sagv(struct drm_i915_private *dev_priv)
 {
+	/* HACK! */
+	if (IS_GEN(dev_priv, 12))
+		return false;
+
 	return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
 		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
 }
-- 
2.23.0

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

Re: [PATCH v3 5/7] drm/i915/tgl: disable SAGV temporarily

[Souza, Jose]

On Thu, 2019-08-29 at 02:25 -0700, Lucas De Marchi wrote:
> SAGV is not currently working for Tiger Lake. We better disable it
> until
> the implementation is stabilized and we can enable it.
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 4fa9bc83c8b4..7294fcf05323 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3654,6 +3654,10 @@ static bool skl_needs_memory_bw_wa(struct
> drm_i915_private *dev_priv)
>  static bool
>  intel_has_sagv(struct drm_i915_private *dev_priv)
>  {
> +	/* HACK! */
> +	if (IS_GEN(dev_priv, 12))
> +		return false;
> +
>  	return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
>  		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/7] drm/i915/tgl: disable SAGV temporarily

[Matt Roper]

On Thu, Aug 29, 2019 at 02:25:52AM -0700, Lucas De Marchi wrote:
> SAGV is not currently working for Tiger Lake. We better disable it until
> the implementation is stabilized and we can enable it.

Does "not currently working" refer to the hardware not working in the
current stepping, or is it just a matter of us not having proper
sequences documented yet in the bspec (and gen11's sequences not being
sufficient)?

Something more descriptive than "HACK!" in the comment below might be a
good idea since we're trying to land this upstream.


Matt

> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4fa9bc83c8b4..7294fcf05323 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3654,6 +3654,10 @@ static bool skl_needs_memory_bw_wa(struct drm_i915_private *dev_priv)
>  static bool
>  intel_has_sagv(struct drm_i915_private *dev_priv)
>  {
> +	/* HACK! */
> +	if (IS_GEN(dev_priv, 12))
> +		return false;
> +
>  	return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
>  		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
>  }
> -- 
> 2.23.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/7] drm/i915/tgl: disable SAGV temporarily

[Souza, Jose]

On Tue, 2019-09-03 at 15:05 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 02:25:52AM -0700, Lucas De Marchi wrote:
> > SAGV is not currently working for Tiger Lake. We better disable it
> > until
> > the implementation is stabilized and we can enable it.
> 
> Does "not currently working" refer to the hardware not working in the
> current stepping, or is it just a matter of us not having proper
> sequences documented yet in the bspec (and gen11's sequences not
> being
> sufficient)?
> 
> Something more descriptive than "HACK!" in the comment below might be
> a
> good idea since we're trying to land this upstream.

The information that I had was that in the current stepping it would
not work but doing some searching I found this HSD: 2208191909 So looks
like it was fixed 15 days ago and a new BIOS should fix the issue.

I guess for now we could go with this patch and the revert it when we
confirm that a reliable released BIOS has the fix and adding the HSD to
the commit message.

> 
> 
> Matt
> 
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 4fa9bc83c8b4..7294fcf05323 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3654,6 +3654,10 @@ static bool skl_needs_memory_bw_wa(struct
> > drm_i915_private *dev_priv)
> >  static bool
> >  intel_has_sagv(struct drm_i915_private *dev_priv)
> >  {
> > +	/* HACK! */
> > +	if (IS_GEN(dev_priv, 12))
> > +		return false;
> > +
> >  	return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
> >  		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
> >  }
> > -- 
> > 2.23.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 6/7] drm/i915/tgl: add gen12 to stolen initialization

[Lucas De Marchi]

Add case for gen == 12 and add MISSING_CASE() for future gens. We were
already handling gen12 as the default, so this doesn't change the
current behavior.

Cc: CQ Tang <cq.tang@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index aa533b4ab5f5..7ce5259d73d6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -425,8 +425,11 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 			bdw_get_stolen_reserved(dev_priv,
 						&reserved_base, &reserved_size);
 		break;
-	case 11:
 	default:
+		MISSING_CASE(INTEL_GEN(dev_priv));
+		/* fall-through */
+	case 11:
+	case 12:
 		icl_get_stolen_reserved(dev_priv, &reserved_base,
 					&reserved_size);
 		break;
-- 
2.23.0

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

Re: [PATCH v3 6/7] drm/i915/tgl: add gen12 to stolen initialization

[Souza, Jose]

On Thu, 2019-08-29 at 02:25 -0700, Lucas De Marchi wrote:
> Add case for gen == 12 and add MISSING_CASE() for future gens. We
> were
> already handling gen12 as the default, so this doesn't change the
> current behavior.

With: BSpec: 19481 and 44980


Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Cc: CQ Tang <cq.tang@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index aa533b4ab5f5..7ce5259d73d6 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -425,8 +425,11 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  			bdw_get_stolen_reserved(dev_priv,
>  						&reserved_base,
> &reserved_size);
>  		break;
> -	case 11:
>  	default:
> +		MISSING_CASE(INTEL_GEN(dev_priv));
> +		/* fall-through */
> +	case 11:
> +	case 12:
>  		icl_get_stolen_reserved(dev_priv, &reserved_base,
>  					&reserved_size);
>  		break;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/7] drm/i915/tgl: add gen12 to stolen initialization

[Matt Roper]

On Thu, Aug 29, 2019 at 02:25:53AM -0700, Lucas De Marchi wrote:
> Add case for gen == 12 and add MISSING_CASE() for future gens. We were
> already handling gen12 as the default, so this doesn't change the
> current behavior.
> 
> Cc: CQ Tang <cq.tang@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Another approach would be to just convert the switch to a more
traditional if/else ladder as we use pretty much everywhere else in the
driver (which would also allow us to handle stuff like vlv and chv
without an extra level of nesting).  But this works too, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index aa533b4ab5f5..7ce5259d73d6 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -425,8 +425,11 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  			bdw_get_stolen_reserved(dev_priv,
>  						&reserved_base, &reserved_size);
>  		break;
> -	case 11:
>  	default:
> +		MISSING_CASE(INTEL_GEN(dev_priv));
> +		/* fall-through */
> +	case 11:
> +	case 12:
>  		icl_get_stolen_reserved(dev_priv, &reserved_base,
>  					&reserved_size);
>  		break;
> -- 
> 2.23.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 7/7] FIXME: drm/i915/tgl: Register state context definition for Gen12

[Lucas De Marchi]

From: Michel Thierry <michel.thierry@intel.com>

Gen12 has subtle changes in the reg state context offsets (some fields
are gone, some are in a different location), compared to previous Gens.

The simplest approach seems to be keeping Gen12 (and future platform)
changes apart from the previous gens, while keeping the registers that
are contiguous in functions we can reuse.

FIXME: https://patchwork.freedesktop.org/patch/324479/?series=65290&rev=4

Bspec: 46255
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c     | 156 +++++++++++++++++-------
 drivers/gpu/drm/i915/gt/intel_lrc.h     |   2 +
 drivers/gpu/drm/i915/gt/intel_lrc_reg.h |  30 ++++-
 3 files changed, 143 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 171d5205962c..b00083aa3ead 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -3169,28 +3169,12 @@ static u32 intel_lr_indirect_ctx_offset(struct intel_engine_cs *engine)
 	return indirect_ctx_offset;
 }
 
-static void execlists_init_reg_state(u32 *regs,
-				     struct intel_context *ce,
-				     struct intel_engine_cs *engine,
-				     struct intel_ring *ring)
+static void init_common_reg_state(u32 *regs,
+				  struct intel_engine_cs *engine,
+				  struct intel_ring *ring)
 {
-	struct i915_ppgtt *ppgtt = i915_vm_to_ppgtt(ce->vm);
-	bool rcs = engine->class == RENDER_CLASS;
 	u32 base = engine->mmio_base;
 
-	/*
-	 * A context is actually a big batch buffer with several
-	 * MI_LOAD_REGISTER_IMM commands followed by (reg, value) pairs. The
-	 * values we are setting here are only for the first context restore:
-	 * on a subsequent save, the GPU will recreate this batchbuffer with new
-	 * values (including all the missing MI_LOAD_REGISTER_IMM commands that
-	 * we are not initializing here).
-	 *
-	 * Must keep consistent with virtual_update_register_offsets().
-	 */
-	regs[CTX_LRI_HEADER_0] = MI_LOAD_REGISTER_IMM(rcs ? 14 : 11) |
-				 MI_LRI_FORCE_POSTED;
-
 	CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(base),
 		_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT) |
 		_MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH));
@@ -3207,38 +3191,44 @@ static void execlists_init_reg_state(u32 *regs,
 	CTX_REG(regs, CTX_BB_HEAD_U, RING_BBADDR_UDW(base), 0);
 	CTX_REG(regs, CTX_BB_HEAD_L, RING_BBADDR(base), 0);
 	CTX_REG(regs, CTX_BB_STATE, RING_BBSTATE(base), RING_BB_PPGTT);
-	CTX_REG(regs, CTX_SECOND_BB_HEAD_U, RING_SBBADDR_UDW(base), 0);
-	CTX_REG(regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
-	CTX_REG(regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
-	if (rcs) {
-		struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
-
-		CTX_REG(regs, CTX_RCS_INDIRECT_CTX, RING_INDIRECT_CTX(base), 0);
-		CTX_REG(regs, CTX_RCS_INDIRECT_CTX_OFFSET,
-			RING_INDIRECT_CTX_OFFSET(base), 0);
-		if (wa_ctx->indirect_ctx.size) {
-			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
+}
 
-			regs[CTX_RCS_INDIRECT_CTX + 1] =
-				(ggtt_offset + wa_ctx->indirect_ctx.offset) |
-				(wa_ctx->indirect_ctx.size / CACHELINE_BYTES);
+static void init_wa_bb_reg_state(u32 *regs,
+				 struct intel_engine_cs *engine,
+				 u32 pos_bb_per_ctx)
+{
+	struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
+	u32 base = engine->mmio_base;
+	u32 pos_indirect_ctx = pos_bb_per_ctx + 2;
+	u32 pos_indirect_ctx_offset = pos_indirect_ctx + 2;
 
-			regs[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
-				intel_lr_indirect_ctx_offset(engine) << 6;
-		}
+	GEM_BUG_ON(engine->id != RCS0);
+	CTX_REG(regs, pos_indirect_ctx, RING_INDIRECT_CTX(base), 0);
+	CTX_REG(regs, pos_indirect_ctx_offset,
+		RING_INDIRECT_CTX_OFFSET(base), 0);
+	if (wa_ctx->indirect_ctx.size) {
+		u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
 
-		CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
-		if (wa_ctx->per_ctx.size) {
-			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
+		regs[pos_indirect_ctx + 1] =
+			(ggtt_offset + wa_ctx->indirect_ctx.offset) |
+			(wa_ctx->indirect_ctx.size / CACHELINE_BYTES);
 
-			regs[CTX_BB_PER_CTX_PTR + 1] =
-				(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
-		}
+		regs[pos_indirect_ctx_offset + 1] =
+			intel_lr_indirect_ctx_offset(engine) << 6;
 	}
 
-	regs[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9) | MI_LRI_FORCE_POSTED;
+	CTX_REG(regs, pos_bb_per_ctx, RING_BB_PER_CTX_PTR(base), 0);
+	if (wa_ctx->per_ctx.size) {
+		u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
 
-	CTX_REG(regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
+		regs[pos_bb_per_ctx + 1] =
+			(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
+	}
+}
+
+static void init_ppgtt_reg_state(u32 *regs, u32 base,
+				 struct i915_ppgtt *ppgtt)
+{
 	/* PDP values well be assigned later if needed */
 	CTX_REG(regs, CTX_PDP3_UDW, GEN8_RING_PDP_UDW(base, 3), 0);
 	CTX_REG(regs, CTX_PDP3_LDW, GEN8_RING_PDP_LDW(base, 3), 0);
@@ -3261,6 +3251,32 @@ static void execlists_init_reg_state(u32 *regs,
 		ASSIGN_CTX_PDP(ppgtt, regs, 1);
 		ASSIGN_CTX_PDP(ppgtt, regs, 0);
 	}
+}
+
+static void gen8_init_reg_state(u32 *regs,
+				struct intel_context *ce,
+				struct intel_engine_cs *engine,
+				struct intel_ring *ring)
+{
+	struct i915_ppgtt *ppgtt = i915_vm_to_ppgtt(ce->vm);
+	bool rcs = engine->class == RENDER_CLASS;
+	u32 base = engine->mmio_base;
+
+	regs[CTX_LRI_HEADER_0] = MI_LOAD_REGISTER_IMM(rcs ? 14 : 11) |
+				 MI_LRI_FORCE_POSTED;
+
+	init_common_reg_state(regs, engine, ring);
+	CTX_REG(regs, CTX_SECOND_BB_HEAD_U, RING_SBBADDR_UDW(base), 0);
+	CTX_REG(regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
+	CTX_REG(regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
+	if (rcs)
+		init_wa_bb_reg_state(regs, engine, CTX_BB_PER_CTX_PTR);
+
+	regs[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9) | MI_LRI_FORCE_POSTED;
+
+	CTX_REG(regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
+
+	init_ppgtt_reg_state(regs, base, ppgtt);
 
 	if (rcs) {
 		regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
@@ -3272,6 +3288,58 @@ static void execlists_init_reg_state(u32 *regs,
 		regs[CTX_END] |= BIT(0);
 }
 
+static void gen12_init_reg_state(u32 *regs,
+				 struct intel_context *ce,
+				 struct intel_engine_cs *engine,
+				 struct intel_ring *ring)
+{
+	struct i915_ppgtt *ppgtt = i915_vm_to_ppgtt(ce->vm);
+	bool rcs = engine->class == RENDER_CLASS;
+	u32 base = engine->mmio_base;
+
+	GEM_DEBUG_EXEC(DRM_INFO_ONCE("Using GEN12 Register State Context\n"));
+
+	regs[GEN12_CTX_LRI_HEADER_0] = MI_LOAD_REGISTER_IMM(13) |
+				       MI_LRI_FORCE_POSTED;
+
+	init_common_reg_state(regs, engine, ring);
+	if (rcs)
+		init_wa_bb_reg_state(regs, engine, GEN12_CTX_BB_PER_CTX_PTR);
+
+	regs[GEN12_CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9) |
+				       MI_LRI_FORCE_POSTED;
+
+	CTX_REG(regs, GEN12_CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
+
+	init_ppgtt_reg_state(regs, base, ppgtt);
+
+	if (rcs) {
+		regs[GEN12_CTX_LRI_HEADER_3] = MI_LOAD_REGISTER_IMM(1);
+		CTX_REG(regs, GEN12_CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+			0);
+
+		/* TODO: oa_init_reg_state ? */
+	}
+}
+
+static void execlists_init_reg_state(u32 *regs,
+				     struct intel_context *ce,
+				     struct intel_engine_cs *engine,
+				     struct intel_ring *ring)
+{
+	/* A context is actually a big batch buffer with several
+	 * MI_LOAD_REGISTER_IMM commands followed by (reg, value) pairs. The
+	 * values we are setting here are only for the first context restore:
+	 * on a subsequent save, the GPU will recreate this batchbuffer with new
+	 * values (including all the missing MI_LOAD_REGISTER_IMM commands that
+	 * we are not initializing here).
+	 */
+	if (INTEL_GEN(engine->i915) >= 12)
+		gen12_init_reg_state(regs, ce, engine, ring);
+	else
+		gen8_init_reg_state(regs, ce, engine, ring);
+}
+
 static int
 populate_lr_context(struct intel_context *ce,
 		    struct drm_i915_gem_object *ctx_obj,
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h b/drivers/gpu/drm/i915/gt/intel_lrc.h
index dc0252e0589e..a4bde38e35d8 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
@@ -49,6 +49,8 @@ struct intel_engine_cs;
 
 #define	  EL_CTRL_LOAD				(1 << 0)
 
+#define GEN12_ENGINE_SEMAPHORE_TOKEN(engine)	_MMIO((engine)->mmio_base + 0x2b4)
+
 /* The docs specify that the write pointer wraps around after 5h, "After status
  * is written out to the last available status QW at offset 5h, this pointer
  * wraps to 0."
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
index b8f20ad71169..b7695b96e484 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
@@ -9,7 +9,7 @@
 
 #include <linux/types.h>
 
-/* GEN8+ Reg State Context */
+/* GEN8 to GEN11 Reg State Context */
 #define CTX_LRI_HEADER_0		0x01
 #define CTX_CONTEXT_CONTROL		0x02
 #define CTX_RING_HEAD			0x04
@@ -39,6 +39,34 @@
 #define CTX_R_PWR_CLK_STATE		0x42
 #define CTX_END				0x44
 
+/* GEN12+ Reg State Context */
+#define GEN12_CTX_LRI_HEADER_0			CTX_LRI_HEADER_0
+#define GEN12_CTX_CONTEXT_CONTROL		CTX_CONTEXT_CONTROL
+#define GEN12_CTX_RING_HEAD			CTX_RING_HEAD
+#define GEN12_CTX_RING_TAIL			CTX_RING_TAIL
+#define GEN12_CTX_RING_BUFFER_START		CTX_RING_BUFFER_START
+#define GEN12_CTX_RING_BUFFER_CONTROL		CTX_RING_BUFFER_CONTROL
+#define GEN12_CTX_BB_HEAD_U			CTX_BB_HEAD_U
+#define GEN12_CTX_BB_HEAD_L			CTX_BB_HEAD_L
+#define GEN12_CTX_BB_STATE			CTX_BB_STATE
+#define GEN12_CTX_BB_PER_CTX_PTR		0x12
+#define GEN12_CTX_RCS_INDIRECT_CTX		0x14
+#define GEN12_CTX_RCS_INDIRECT_CTX_OFFSET	0x16
+#define GEN12_CTX_LRI_HEADER_1			CTX_LRI_HEADER_1
+#define GEN12_CTX_CTX_TIMESTAMP			CTX_CTX_TIMESTAMP
+#define GEN12_CTX_PDP3_UDW			CTX_PDP3_UDW
+#define GEN12_CTX_PDP3_LDW			CTX_PDP3_LDW
+#define GEN12_CTX_PDP2_UDW			CTX_PDP2_UDW
+#define GEN12_CTX_PDP2_LDW			CTX_PDP2_LDW
+#define GEN12_CTX_PDP1_UDW			CTX_PDP1_UDW
+#define GEN12_CTX_PDP1_LDW			CTX_PDP1_LDW
+#define GEN12_CTX_PDP0_UDW			CTX_PDP0_UDW
+#define GEN12_CTX_PDP0_LDW			CTX_PDP0_LDW
+#define GEN12_CTX_LRI_HEADER_2			0x34
+#define GEN12_CTX_LRI_HEADER_3			0x41
+#define GEN12_CTX_R_PWR_CLK_STATE		0x42
+#define GEN12_CTX_GPGPU_CSR_BASE_ADDRESS	0x44
+
 #define CTX_REG(reg_state, pos, reg, val) do { \
 	u32 *reg_state__ = (reg_state); \
 	const u32 pos__ = (pos); \
-- 
2.23.0

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

✗ Fi.CI.CHECKPATCH: warning for Tiger Lake batch 3.5

[Patchwork]

== Series Details ==

Series: Tiger Lake batch 3.5
URL   : https://patchwork.freedesktop.org/series/65982/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3c4304cf3c0a drm/i915/psr: Only handle interruptions of the transcoder in use
-:243: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'trans' - possible side-effects?
#243: FILE: drivers/gpu/drm/i915/i915_reg.h:4228:
+#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
+						 0 : ((trans) - TRANSCODER_A + 1) * 8)

total: 0 errors, 0 warnings, 1 checks, 199 lines checked
c123e297f88d drm/i915/tgl: Access the right register when handling PSR interruptions
684702c6f6b2 drm/i915: protect access to DP_TP_* on non-dp
2c574cbf5bb0 drm/i915/tgl: move DP_TP_* to transcoder
ba00f24fff4c drm/i915/tgl: disable SAGV temporarily
887da21a6c1b drm/i915/tgl: add gen12 to stolen initialization
ba1de03b3f49 FIXME: drm/i915/tgl: Register state context definition for Gen12

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

✓ Fi.CI.BAT: success for Tiger Lake batch 3.5

[Patchwork]

== Series Details ==

Series: Tiger Lake batch 3.5
URL   : https://patchwork.freedesktop.org/series/65982/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6801 -> Patchwork_14221
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/

Known issues
------------

  Here are the changes found in Patchwork_14221 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [PASS][1] -> [DMESG-WARN][2] ([fdo#106387]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-kbl-guc:         [INCOMPLETE][3] -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-kbl-guc/igt@i915_selftest@live_gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-kbl-guc/igt@i915_selftest@live_gem_contexts.html
    - fi-cfl-guc:         [INCOMPLETE][5] ([fdo#111514]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u4}:        [FAIL][7] ([fdo#103167]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-icl-u4/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-icl-u4/igt@kms_frontbuffer_tracking@basic.html
    - fi-hsw-peppy:       [DMESG-WARN][9] ([fdo#102614]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
    - fi-icl-u2:          [FAIL][11] ([fdo#103167]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#111514]: https://bugs.freedesktop.org/show_bug.cgi?id=111514


Participating hosts (52 -> 44)
------------------------------

  Missing    (8): fi-ilk-m540 fi-tgl-u fi-hsw-4200u fi-bsw-cyan fi-icl-y fi-icl-guc fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6801 -> Patchwork_14221

  CI-20190529: 20190529
  CI_DRM_6801: 244c5c8116c0042d61455697a9d85e899e2d9267 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5153: 32ffe5304957d585d41c2f14cc5190d4588ccc41 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14221: ba1de03b3f4952c7607693e5b203f8754ce220ec @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ba1de03b3f49 FIXME: drm/i915/tgl: Register state context definition for Gen12
887da21a6c1b drm/i915/tgl: add gen12 to stolen initialization
ba00f24fff4c drm/i915/tgl: disable SAGV temporarily
2c574cbf5bb0 drm/i915/tgl: move DP_TP_* to transcoder
684702c6f6b2 drm/i915: protect access to DP_TP_* on non-dp
c123e297f88d drm/i915/tgl: Access the right register when handling PSR interruptions
3c4304cf3c0a drm/i915/psr: Only handle interruptions of the transcoder in use

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/7] Tiger Lake batch 3.5

[Daniele Ceraolo Spurio]

On 8/29/19 2:25 AM, Lucas De Marchi wrote:
> Mostly the same patches as revision 5 of
> https://patchwork.freedesktop.org/series/65290/ with some dropped and
> some trivial ones added.
> 
> Implementation for the first patches changed though, in order to address
> the review comments. Intention here is to get these first 4 patches as
> soon as possible so we can have CI enabled with TGL.
> 
> Also last patch is not to be merged, it's only here to help with CI as
> comments from previous versions need to be handled. Daniele, do you plan
> to submit it yourself?

Yes, I'm waiting for https://patchwork.freedesktop.org/series/57117/ to 
go in first so we can avoid the hassle of splitting the virtual context 
update function.

Daniele

> 
> I will be out for the next 2.5w, so don't expect me to handle the
> comments here. However José and/or other people can handle them.
> 
> thanks
> Lucas De Marchi
> 
> José Roberto de Souza (2):
>    drm/i915/psr: Only handle interruptions of the transcoder in use
>    drm/i915/tgl: Access the right register when handling PSR
>      interruptions
> 
> Lucas De Marchi (4):
>    drm/i915: protect access to DP_TP_* on non-dp
>    drm/i915/tgl: move DP_TP_* to transcoder
>    drm/i915/tgl: disable SAGV temporarily
>    drm/i915/tgl: add gen12 to stolen initialization
> 
> Michel Thierry (1):
>    FIXME: drm/i915/tgl: Register state context definition for Gen12
> 
>   drivers/gpu/drm/i915/display/intel_ddi.c      |  49 ++++--
>   .../drm/i915/display/intel_display_types.h    |   9 +
>   drivers/gpu/drm/i915/display/intel_dp.c       |  13 +-
>   drivers/gpu/drm/i915/display/intel_dp_mst.c   |   8 +-
>   drivers/gpu/drm/i915/display/intel_psr.c      | 163 +++++++++---------
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |   5 +-
>   drivers/gpu/drm/i915/gt/intel_lrc.c           | 156 ++++++++++++-----
>   drivers/gpu/drm/i915/gt/intel_lrc.h           |   2 +
>   drivers/gpu/drm/i915/gt/intel_lrc_reg.h       |  30 +++-
>   drivers/gpu/drm/i915/i915_irq.c               |  51 +++++-
>   drivers/gpu/drm/i915/i915_reg.h               |  27 ++-
>   drivers/gpu/drm/i915/intel_pm.c               |   4 +
>   12 files changed, 345 insertions(+), 172 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Tiger Lake batch 3.5

[Patchwork]

== Series Details ==

Series: Tiger Lake batch 3.5
URL   : https://patchwork.freedesktop.org/series/65982/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6801_full -> Patchwork_14221_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_14221_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-apl8/igt@gem_ctx_isolation@rcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-apl1/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_exec_flush@basic-wb-set-default:
    - shard-hsw:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103540])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-hsw6/igt@gem_exec_flush@basic-wb-set-default.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-hsw2/igt@gem_exec_flush@basic-wb-set-default.html

  * igt@gem_exec_schedule@fifo-bsd2:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#109276]) +8 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb1/igt@gem_exec_schedule@fifo-bsd2.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb6/igt@gem_exec_schedule@fifo-bsd2.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108686])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-glk9/igt@gem_tiled_swapping@non-threaded.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-glk8/igt@gem_tiled_swapping@non-threaded.html

  * igt@kms_flip@flip-vs-panning-vs-hang-interruptible:
    - shard-apl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#103927]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-apl4/igt@kms_flip@flip-vs-panning-vs-hang-interruptible.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-apl7/igt@kms_flip@flip-vs-panning-vs-hang-interruptible.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103167]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [PASS][13] -> [DMESG-WARN][14] ([fdo#108566]) +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-kbl4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-kbl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][15] -> [FAIL][16] ([fdo#108341])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb4/igt@kms_psr@no_drrs.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#109441]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb4/igt@kms_psr@psr2_cursor_blt.html

  
#### Possible fixes ####

  * igt@gem_exec_schedule@fifo-bsd:
    - shard-iclb:         [SKIP][19] ([fdo#111325]) -> [PASS][20] +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb2/igt@gem_exec_schedule@fifo-bsd.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb7/igt@gem_exec_schedule@fifo-bsd.html

  * igt@gem_exec_schedule@promotion-bsd1:
    - shard-iclb:         [SKIP][21] ([fdo#109276]) -> [PASS][22] +7 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb5/igt@gem_exec_schedule@promotion-bsd1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb1/igt@gem_exec_schedule@promotion-bsd1.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-hsw:          [FAIL][23] ([fdo#102887]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-hsw2/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-hsw5/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          [FAIL][25] ([fdo#105363]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-glk5/igt@kms_flip@flip-vs-expired-vblank.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-glk4/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-skl2/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +6 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +2 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-apl7/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-apl7/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][33] ([fdo#103166]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][35] ([fdo#99912]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-kbl1/igt@kms_setmode@basic.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-kbl7/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-b-wait-idle-hang:
    - shard-apl:          [INCOMPLETE][37] ([fdo#103927]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-apl4/igt@kms_vblank@pipe-b-wait-idle-hang.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-apl4/igt@kms_vblank@pipe-b-wait-idle-hang.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [SKIP][39] ([fdo#109276]) -> [FAIL][40] ([fdo#111329])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb3/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb4/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_mocs_settings@mocs-reset-bsd2:
    - shard-iclb:         [FAIL][41] ([fdo#111330]) -> [SKIP][42] ([fdo#109276])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-iclb1/igt@gem_mocs_settings@mocs-reset-bsd2.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-iclb5/igt@gem_mocs_settings@mocs-reset-bsd2.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-mmap-gtt:
    - shard-skl:          [FAIL][43] ([fdo#108040]) -> [FAIL][44] ([fdo#103167])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6801/shard-skl5/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-mmap-gtt.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/shard-skl4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-mmap-gtt.html

  
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6801 -> Patchwork_14221

  CI-20190529: 20190529
  CI_DRM_6801: 244c5c8116c0042d61455697a9d85e899e2d9267 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5153: 32ffe5304957d585d41c2f14cc5190d4588ccc41 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14221: ba1de03b3f4952c7607693e5b203f8754ce220ec @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14221/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx