drm/i915: Make pipe/transcoder offsets not depend on enum values

Depending on the transcoder enum values to translate from transcoder to EDP PSR flags can easily break if we add a new transcoder. So remove the dependency by using an explicit mapping.

While at it also add a WARN for unexpected trancoders.

v2:
- Simplify things by defining flag shift values instead of indices.
- s/trans/cpu_transcoder/ (Ville)

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h  | 10 +++++---
 drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 21 deletions(-)

drivers/gpu/drm/i915/i915_reg.h +0 -0

index edb58af1e903..f80b16e3ff33 100644

drivers/gpu/drm/i915/i915_reg.h +4 -0

+#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_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
 #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)

drivers/gpu/drm/i915/intel_psr.c +37 -18

index 48df16a02fac..26292961d693 100644
@@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 	}
 }
 
+static int edp_psr_shift(enum transcoder cpu_transcoder)
+{
+	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;
+	}
+}
+
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
 {
 	u32 debug_mask, mask;
+	enum transcoder cpu_transcoder;
+	u32 transcoders = BIT(TRANSCODER_EDP);
 
-	mask = EDP_PSR_ERROR(TRANSCODER_EDP);
-	debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
-		     EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
-	if (INTEL_GEN(dev_priv) >= 8) {
-		mask |= EDP_PSR_ERROR(TRANSCODER_A) |
-			EDP_PSR_ERROR(TRANSCODER_B) |
-			EDP_PSR_ERROR(TRANSCODER_C);
-
-		debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
-			      EDP_PSR_POST_EXIT(TRANSCODER_B) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
-			      EDP_PSR_POST_EXIT(TRANSCODER_C) |
-			      EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+	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);
 	}
 
 	if (debug & I915_PSR_DEBUG_IRQ)

@@ -159,18 +176,20 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 			       BIT(TRANSCODER_C);
 
 	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+		int shift = edp_psr_shift(cpu_transcoder);
+
 		/* FIXME: Exit PSR and link train manually when this happens. */
-		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+		if (psr_iir & EDP_PSR_ERROR(shift))
 			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
 				      transcoder_name(cpu_transcoder));
 
-		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+		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));
 		}
 
-		if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+		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));

Signature · 1 line

2.13.2

Add a comment to the pipe and transcoder enum definitions about our assumption in the code that pipe==transcoder for PIPE_A-C / TRANSCODER_A-C. This means we have to keep the values for these pipe/transcoder enums fixed.

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/gpu/drm/i915/intel_display.h +0 -0

index 43eb4ebbcc35..cbb5d79d6a4c 100644

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum transcoder {
 	TRANSCODER_A = 0,
 	TRANSCODER_B,

Signature · 1 line

2.13.2

Quoted 21 lines · Imre Deak

Add a comment to the pipe and transcoder enum definitions about our assumption in the code that pipe==transcoder for PIPE_A-C / TRANSCODER_A-C. This means we have to keep the values for these pipe/transcoder enums fixed.

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/gpu/drm/i915/intel_display.h +0 -0

index 43eb4ebbcc35..cbb5d79d6a4c 100644
@@ -43,6 +43,10 @@ enum i915_gpio {
 	GPIOM,
 };
 

drivers/gpu/drm/i915/intel_display.h enum i915_gpio { · line 49

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

+/*
+ * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
+ * these pipes.
+ */

I suspect the A-C part is going to bitrot. Also I'm sure we make more assupmtions about these values (PIPE_A == 0, PIPE_N+1 == n+1, etc.). We should probably try to spell it all out here.

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum pipe {
 	INVALID_PIPE = -1,
 

@@ -56,6 +60,10 @@ enum pipe {
 

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 66

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

 #define pipe_name(p) ((p) + 'A')
 
+/*
+ * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
+ * these transcoders.
+ */

Same issue with the A-C part perhaps.

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 67

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum transcoder {

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 	TRANSCODER_A = 0,
 	TRANSCODER_B,

TRANSCODER_C,

Quoted 7 lines

TRANSCODER_EDP,
TRANSCODER_DSI_0,
TRANSCODER_DSI_1,

Signature · 1 line

2.13.2

Signature · 2 lines

Ville Syrjälä
Intel

Quoted 22 lines · Ville Syrjälä

Quoted 21 lines · Imre Deak

Add a comment to the pipe and transcoder enum definitions about our assumption in the code that pipe==transcoder for PIPE_A-C / TRANSCODER_A-C. This means we have to keep the values for these pipe/transcoder enums fixed.

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/gpu/drm/i915/intel_display.h +0 -0

index 43eb4ebbcc35..cbb5d79d6a4c 100644
@@ -43,6 +43,10 @@ enum i915_gpio {
 	GPIOM,
 };
 

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.h enum i915_gpio { · line 49

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

+/*
+ * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
+ * these pipes.
+ */

I suspect the A-C part is going to bitrot. Also I'm sure we make more assupmtions about these values (PIPE_A == 0, PIPE_N+1 == n+1, etc.). We should probably try to spell it all out here.

Ok, can add instead:

/*
* Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
* rest have consecutive values and match the enum values of transcoders
* with a 1:1 transcoder->pipe mapping.
*/

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum pipe {
 	INVALID_PIPE = -1,
 

@@ -56,6 +60,10 @@ enum pipe {
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 66

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

 #define pipe_name(p) ((p) + 'A')
 
+/*
+ * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
+ * these transcoders.
+ */

Same issue with the A-C part perhaps.

and here:

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 67

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum transcoder {

/*
* The following transcoders have a 1:1 transcoder->pipe mapping, keep
* their values fixed: the code assumes that TRANSCODER_A=0, the rest
* have consecutive values and match the enum values of the pipes they map
* to.
*/

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 	TRANSCODER_A = 0,
 	TRANSCODER_B,

TRANSCODER_C,

/*
* The following transcoders can map to any pipe, their enum value
* doesn't need to stay fixed.
*/

Quoted 12 lines

Quoted 8 lines

TRANSCODER_EDP,
TRANSCODER_DSI_0,
TRANSCODER_DSI_1,

Signature · 1 line

2.13.2

Signature · 2 lines

Ville Syrjälä
Intel

Quoted 22 lines · Ville Syrjälä

Quoted 21 lines · Imre Deak

Add a comment to the pipe and transcoder enum definitions about our assumption in the code that pipe==transcoder for PIPE_A-C / TRANSCODER_A-C. This means we have to keep the values for these pipe/transcoder enums fixed.

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/gpu/drm/i915/intel_display.h +0 -0

index 43eb4ebbcc35..cbb5d79d6a4c 100644
@@ -43,6 +43,10 @@ enum i915_gpio {
 	GPIOM,
 };
 

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.h enum i915_gpio { · line 49

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

+/*
+ * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
+ * these pipes.
+ */

I suspect the A-C part is going to bitrot. Also I'm sure we make more assupmtions about these values (PIPE_A == 0, PIPE_N+1 == n+1, etc.). We should probably try to spell it all out here.

Ok, can add instead:

/*
* Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
* rest have consecutive values and match the enum values of transcoders
* with a 1:1 transcoder->pipe mapping.

*/

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum pipe {
 	INVALID_PIPE = -1,
 

@@ -56,6 +60,10 @@ enum pipe {
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 66

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

 #define pipe_name(p) ((p) + 'A')
 
+/*
+ * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
+ * these transcoders.
+ */

Same issue with the A-C part perhaps.

and here:

Quoted 1 line

Quoted 1 line

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 67

Quoted 1 line

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum transcoder {

/*
* The following transcoders have a 1:1 transcoder->pipe mapping, keep
* their values fixed: the code assumes that TRANSCODER_A=0, the rest
* have consecutive values and match the enum values of the pipes they map
* to.
*/

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 	TRANSCODER_A = 0,
 	TRANSCODER_B,

TRANSCODER_C,

/*
* The following transcoders can map to any pipe, their enum value
* doesn't need to stay fixed.
*/

Quoted 8 lines

Quoted 7 lines

TRANSCODER_EDP,
TRANSCODER_DSI_0,
TRANSCODER_DSI_1,

Signature · 1 line

2.13.2

Quoted 3 lines

Signature · 2 lines

Ville Syrjälä
Intel

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 33 lines · Imre Deak

Quoted 31 lines · Ville Syrjälä

Quoted 18 lines · Imre Deak

Add a comment to the pipe and transcoder enum definitions about our assumption in the code that pipe==transcoder for PIPE_A-C / TRANSCODER_A-C. This means we have to keep the values for these pipe/transcoder enums fixed.

Cc Ville Syrjälä <[email protected]>

Cc Lucas De Marchi <[email protected]>

Cc Mika Kahola <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/gpu/drm/i915/intel_display.h +0 -0

index 43eb4ebbcc35..cbb5d79d6a4c 100644

drivers/gpu/drm/i915/intel_display.h enum i915_gpio { · line 49

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

@@ -43,6 +43,10 @@ enum i915_gpio {
 	GPIOM,
 };
 
+/*
+ * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
+ * these pipes.
+ */

I suspect the A-C part is going to bitrot. Also I'm sure we make more assupmtions about these values (PIPE_A == 0, PIPE_N+1 == n+1, etc.). We should probably try to spell it all out here.

Quoted 6 lines

Ok, can add instead:

/*
* Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
* rest have consecutive values and match the enum values of transcoders
* with a 1:1 transcoder->pipe mapping.

1:1 transcoder <-> pipe mapping, so it doesn't look like it's a pointer :)

Quoted 21 lines

*/

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum pipe {
 	INVALID_PIPE = -1,
 

@@ -56,6 +60,10 @@ enum pipe {
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 66

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.h +4 -0

 #define pipe_name(p) ((p) + 'A')
 
+/*
+ * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
+ * these transcoders.
+ */

Same issue with the A-C part perhaps.

and here:

Quoted 1 line

Quoted 13 lines

Quoted 1 line

drivers/gpu/drm/i915/intel_display.h enum pipe { · line 67

Quoted 1 line

drivers/gpu/drm/i915/intel_display.h +0 -0

 enum transcoder {

/*
* The following transcoders have a 1:1 transcoder->pipe mapping, keep
* their values fixed: the code assumes that TRANSCODER_A=0, the rest
* have consecutive values and match the enum values of the pipes they map
* to.
*/

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.h +0 -0

 	TRANSCODER_A = 0,
 	TRANSCODER_B,

TRANSCODER_C,

could we additionally do like below?

TRANSCODER_A = PIPE_A,
TRANSCODER_B = PIPE_B,
TRANSCODER_C = PIPE_C,

Or at least a BUILD_BUG_ON(TRANSCODER_A != PIPE_A || TRANSCODER_B != PIPE_B || TRANSCODER_C != PIPE_C)

Lucas De Marchi

Quoted 21 lines

/*
* The following transcoders can map to any pipe, their enum value
* doesn't need to stay fixed.
*/

Quoted 11 lines

Quoted 7 lines

TRANSCODER_EDP,
TRANSCODER_DSI_0,
TRANSCODER_DSI_1,

Signature · 1 line

2.13.2

Signature · 2 lines

Ville Syrjälä
Intel

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Depending on the transcoder enum values to translate from transcoder to pipe/transcoder register addresses can easily break if we add a new transcoder. So remove the dependency by using named initializers.

Suggested-by Ville Syrjälä <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_pci.c +0 -0

index 983ae7fd8217..1b81d7cb209e 100644
@@ -33,16 +33,30 @@
 #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
 

drivers/gpu/drm/i915/i915_pci.c +33 -10

+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+	}
 
 #define GEN_CHV_PIPEOFFSETS \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  CHV_PIPE_C_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   CHV_TRANSCODER_C_OFFSET }
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
+	}
 
 #define CURSOR_OFFSETS \
 	.cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }

@@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
 
 #define GEN11_FEATURES \
 	GEN10_FEATURES, \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
-			  PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
-			   TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+	}, \
 	GEN(11), \
 	.ddb_size = 2048, \
 	.has_logical_ring_elsq = 1

Signature · 1 line

2.13.2

Quoted 18 lines · Imre Deak

Depending on the transcoder enum values to translate from transcoder to pipe/transcoder register addresses can easily break if we add a new transcoder. So remove the dependency by using named initializers.

Suggested-by Ville Syrjälä <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_pci.c +0 -0

index 983ae7fd8217..1b81d7cb209e 100644
@@ -33,16 +33,30 @@
 #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
 

drivers/gpu/drm/i915/i915_pci.c line 40

Quoted 10 lines

drivers/gpu/drm/i915/i915_pci.c +5 -4

 #define GEN_DEFAULT_PIPEOFFSETS \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET,	\
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \

Hmm. We do define this as int pipe_offsets[I915_MAX_TRANSCODERS], and PIPECONF/TRANSCONF is per-transcoder so I suppose using TRANSCODER_foo here make sense.

Couple of things that came to mind while thinking about this:
- pipe_offsets[] & co. could probably be u32 since we don't need negative values

- _PIPE_EDP could be removed, which would maybe shrink a few arrays based on I915_MAX_PIPES

Patch is

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 49 lines

drivers/gpu/drm/i915/i915_pci.c +30 -10

+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+	}
 
 #define GEN_CHV_PIPEOFFSETS \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  CHV_PIPE_C_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   CHV_TRANSCODER_C_OFFSET }
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
+	}
 
 #define CURSOR_OFFSETS \
 	.cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }

@@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
 
 #define GEN11_FEATURES \
 	GEN10_FEATURES, \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
-			  PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
-			   TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \

Quoted 8 lines

drivers/gpu/drm/i915/i915_pci.c +3 -0

+		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+	}, \
 	GEN(11), \
 	.ddb_size = 2048, \
 	.has_logical_ring_elsq = 1

Signature · 1 line

2.13.2

Signature · 2 lines

Ville Syrjälä
Intel

Quoted 34 lines · Ville Syrjälä

Quoted 18 lines · Imre Deak

Depending on the transcoder enum values to translate from transcoder to pipe/transcoder register addresses can easily break if we add a new transcoder. So remove the dependency by using named initializers.

Suggested-by Ville Syrjälä <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_pci.c +0 -0

index 983ae7fd8217..1b81d7cb209e 100644
@@ -33,16 +33,30 @@
 #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
 

drivers/gpu/drm/i915/i915_pci.c line 40

Quoted 10 lines

drivers/gpu/drm/i915/i915_pci.c +5 -4

 #define GEN_DEFAULT_PIPEOFFSETS \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET,	\
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \

Hmm. We do define this as int pipe_offsets[I915_MAX_TRANSCODERS], and PIPECONF/TRANSCONF is per-transcoder so I suppose using TRANSCODER_foo here make sense.

Quoted 3 lines

Couple of things that came to mind while thinking about this:
- pipe_offsets[] & co. could probably be u32 since we don't need negative values

Ok, can follow up with that.

Quoted 2 lines

- _PIPE_EDP could be removed, which would maybe shrink a few arrays based on I915_MAX_PIPES

Agreed. Looks like all the users are in gvt:

- PIPECONF(_PIPE_EDP) should use PIPECONF(TRANSCODER_EDP) instead. The current code would also break if we added a new transcoder.

- AFAICS
PIPEDSL(_PIPE_EDP)
PIPESTAT(_PIPE_EDP)
PIPE_FRMCOUNT_G4X(_PIPE_EDP)
PIPE_FLIPCOUNT_G4X(_PIPE_EDP)
are bogus, since these registers don't exist.

Adding gvt folks for these.

Quoted 65 lines

Patch is

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 57 lines

drivers/gpu/drm/i915/i915_pci.c +33 -10

+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+	}
 
 #define GEN_CHV_PIPEOFFSETS \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  CHV_PIPE_C_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   CHV_TRANSCODER_C_OFFSET }
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
+	}
 
 #define CURSOR_OFFSETS \
 	.cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }

@@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
 
 #define GEN11_FEATURES \
 	GEN10_FEATURES, \
-	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
-			  PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
-			  PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
-	.trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
-			   TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
-			   TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
+	.pipe_offsets = { \
+		[TRANSCODER_A] = PIPE_A_OFFSET, \
+		[TRANSCODER_B] = PIPE_B_OFFSET, \
+		[TRANSCODER_C] = PIPE_C_OFFSET, \
+		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+	}, \
+	.trans_offsets = { \
+		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
+		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+	}, \
 	GEN(11), \
 	.ddb_size = 2048, \
 	.has_logical_ring_elsq = 1

Signature · 1 line

2.13.2

Signature · 2 lines

Ville Syrjälä
Intel