[PATCH 1/4] drm/i915: Remove the "pf" crc source

[PATCH 1/4] drm/i915: Remove the "pf" crc source

[Ville Syrjala]

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

The "pipe" and "pf" crc sources are in fact the same thing.
Remove the "pf" one.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       | 1 -
 drivers/gpu/drm/i915/intel_pipe_crc.c | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 17fe942eaafa..4e11d970cbcf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1196,7 +1196,6 @@ enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_NONE,
 	INTEL_PIPE_CRC_SOURCE_PLANE1,
 	INTEL_PIPE_CRC_SOURCE_PLANE2,
-	INTEL_PIPE_CRC_SOURCE_PF,
 	INTEL_PIPE_CRC_SOURCE_PIPE,
 	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
 	INTEL_PIPE_CRC_SOURCE_TV,
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index a8554dc4f196..a3a3ad760158 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -34,7 +34,6 @@ static const char * const pipe_crc_sources[] = {
 	"none",
 	"plane1",
 	"plane2",
-	"pf",
 	"pipe",
 	"TV",
 	"DP-B",
@@ -396,7 +395,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
-		*source = INTEL_PIPE_CRC_SOURCE_PF;
+		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
 
 	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
@@ -405,7 +404,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	case INTEL_PIPE_CRC_SOURCE_PLANE2:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
-	case INTEL_PIPE_CRC_SOURCE_PF:
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
@@ -532,7 +531,6 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
 	case INTEL_PIPE_CRC_SOURCE_PLANE2:
-	case INTEL_PIPE_CRC_SOURCE_PF:
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		return 0;
 	default:
-- 
2.19.2

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

[PATCH 2/4] drm/i915: Use named initializers for the crc source name array

[Ville Syrjala]

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

We assume that the index of the string in the crc source names
array matches the enum value for the crc source. Let's use named
initializers to make sure that is indeed the case even if someone
rearranges either the enum or the array.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pipe_crc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index a3a3ad760158..fe0ff89b980b 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -31,15 +31,15 @@
 #include "intel_drv.h"
 
 static const char * const pipe_crc_sources[] = {
-	"none",
-	"plane1",
-	"plane2",
-	"pipe",
-	"TV",
-	"DP-B",
-	"DP-C",
-	"DP-D",
-	"auto",
+	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
+	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
+	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
+	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
+	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
+	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
+	[INTEL_PIPE_CRC_SOURCE_DP_C] = "DP-C",
+	[INTEL_PIPE_CRC_SOURCE_DP_D] = "DP-D",
+	[INTEL_PIPE_CRC_SOURCE_AUTO] = "auto",
 };
 
 static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
-- 
2.19.2

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

[PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Ville Syrjala]

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

DP CRCs don't really work on g4x. If you want any CRCs on DP you must
select the CRC source before the port is enabled, otherwise the CRC
source select bits simply ignore any writes to them. And once the port
is enabled we mustn't change the CRC source select until the port is
disabled. That almost works, but not quite :( Eventually the CRC source
select bits get permanently stuck one way or the other, and after that
a reboot (or possibly a display reset) is needed to get working CRCs
on that pipe (not matter which CRC source we try to use).

Additionally the DFT scrambler reset bits we're trying to use don't
seem to exist on g4x. There are some potentially relevant looking bits
in the pipe registers, but when I tried it I got stable looking CRCs
without setting any bits for this.

If there is a way to make DP CRCs work reliably on g4x, I wasn't
able to find it. So let's just remove the broken code we have.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-----------------------
 1 file changed, 11 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index fe0ff89b980b..66bb7b031537 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				 enum intel_pipe_crc_source *source,
 				 u32 *val)
 {
-	bool need_stable_symbols = false;
-
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
 		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe, source);
 		if (ret)
@@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			return -EINVAL;
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
 		break;
-	case INTEL_PIPE_CRC_SOURCE_DP_B:
-		if (!IS_G4X(dev_priv))
-			return -EINVAL;
-		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
-		need_stable_symbols = true;
-		break;
-	case INTEL_PIPE_CRC_SOURCE_DP_C:
-		if (!IS_G4X(dev_priv))
-			return -EINVAL;
-		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
-		need_stable_symbols = true;
-		break;
-	case INTEL_PIPE_CRC_SOURCE_DP_D:
-		if (!IS_G4X(dev_priv))
-			return -EINVAL;
-		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
-		need_stable_symbols = true;
-		break;
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		*val = 0;
 		break;
 	default:
+		/*
+		 * The DP CRC source doesn't work on g4x.
+		 * It can be made to work to some degree by selecting
+		 * the correct CRC source before the port is enabled,
+		 * and not touching the CRC source bits again until
+		 * the port is disabled. But even then the bits
+		 * eventually get stuck and a reboot is needed to get
+		 * working CRCs on the pipe again. Let's simply
+		 * refuse to use DP CRCs on g4x.
+		 */
 		return -EINVAL;
 	}
 
-	/*
-	 * When the pipe CRC tap point is after the transcoders we need
-	 * to tweak symbol-level features to produce a deterministic series of
-	 * symbols for a given frame. We need to reset those features only once
-	 * a frame (instead of every nth symbol):
-	 *   - DC-balance: used to ensure a better clock recovery from the data
-	 *     link (SDVO)
-	 *   - DisplayPort scrambling: used for EMI reduction
-	 */
-	if (need_stable_symbols) {
-		u32 tmp = I915_READ(PORT_DFT2_G4X);
-
-		WARN_ON(!IS_G4X(dev_priv));
-
-		I915_WRITE(PORT_DFT_I9XX,
-			   I915_READ(PORT_DFT_I9XX) | DC_BALANCE_RESET);
-
-		if (pipe == PIPE_A)
-			tmp |= PIPE_A_SCRAMBLE_RESET;
-		else
-			tmp |= PIPE_B_SCRAMBLE_RESET;
-
-		I915_WRITE(PORT_DFT2_G4X, tmp);
-	}
-
 	return 0;
 }
 
@@ -282,24 +247,6 @@ static void vlv_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
 	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
 		tmp &= ~DC_BALANCE_RESET_VLV;
 	I915_WRITE(PORT_DFT2_G4X, tmp);
-
-}
-
-static void g4x_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
-					 enum pipe pipe)
-{
-	u32 tmp = I915_READ(PORT_DFT2_G4X);
-
-	if (pipe == PIPE_A)
-		tmp &= ~PIPE_A_SCRAMBLE_RESET;
-	else
-		tmp &= ~PIPE_B_SCRAMBLE_RESET;
-	I915_WRITE(PORT_DFT2_G4X, tmp);
-
-	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
-		I915_WRITE(PORT_DFT_I9XX,
-			   I915_READ(PORT_DFT_I9XX) & ~DC_BALANCE_RESET);
-	}
 }
 
 static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
@@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
 	switch (source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 	case INTEL_PIPE_CRC_SOURCE_TV:
-	case INTEL_PIPE_CRC_SOURCE_DP_B:
-	case INTEL_PIPE_CRC_SOURCE_DP_C:
-	case INTEL_PIPE_CRC_SOURCE_DP_D:
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		return 0;
 	default:
@@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
 
 	if (!source) {
-		if (IS_G4X(dev_priv))
-			g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
-		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
 		else if ((IS_HASWELL(dev_priv) ||
 			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
-- 
2.19.2

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

[PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Ville Syrjala]

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

On skl the crc registers were extended to provide plane crcs
for up to 7 planes. Add the new crc sources.

The current code uses the ivb+ register definitions for skl+
which does happen to work as the plane1, plane2, and dmux/pf
bits happen the match what ivb+ had. So no bug in the current
code.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  5 ++
 drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 76 ++++++++++++++++++++++++++-
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e11d970cbcf..8607c1e9ed02 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_NONE,
 	INTEL_PIPE_CRC_SOURCE_PLANE1,
 	INTEL_PIPE_CRC_SOURCE_PLANE2,
+	INTEL_PIPE_CRC_SOURCE_PLANE3,
+	INTEL_PIPE_CRC_SOURCE_PLANE4,
+	INTEL_PIPE_CRC_SOURCE_PLANE5,
+	INTEL_PIPE_CRC_SOURCE_PLANE6,
+	INTEL_PIPE_CRC_SOURCE_PLANE7,
 	INTEL_PIPE_CRC_SOURCE_PIPE,
 	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
 	INTEL_PIPE_CRC_SOURCE_TV,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0df8c6e76da7..5286536e9cb8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4017,6 +4017,15 @@ enum {
 /* Pipe A CRC regs */
 #define _PIPE_CRC_CTL_A			0x60050
 #define   PIPE_CRC_ENABLE		(1 << 31)
+/* skl+ source selection */
+#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
+#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
+#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)
 /* ivb+ source selection */
 #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
 #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 66bb7b031537..e521f82ba5d9 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
 	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
 	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
 	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
+	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
+	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
+	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
+	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
+	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
 	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
 	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
 	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
@@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
+static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
+				enum pipe pipe,
+				enum intel_pipe_crc_source *source,
+				uint32_t *val,
+				bool set_wa)
+{
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	switch (*source) {
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_1_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_2_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE3:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_3_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE4:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_4_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE5:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_5_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE6:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_6_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PLANE7:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_7_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DMUX_SKL;
+		break;
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		*val = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
 			       enum intel_pipe_crc_source *source, u32 *val,
@@ -381,8 +430,10 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
 	else if (IS_GEN_RANGE(dev_priv, 5, 6))
 		return ilk_pipe_crc_ctl_reg(source, val);
-	else
+	else if (INTEL_GEN(dev_priv) < 9)
 		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
+	else
+		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int
@@ -482,6 +533,25 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
 	}
 }
 
+static int skl_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_PLANE3:
+	case INTEL_PIPE_CRC_SOURCE_PLANE4:
+	case INTEL_PIPE_CRC_SOURCE_PLANE5:
+	case INTEL_PIPE_CRC_SOURCE_PLANE6:
+	case INTEL_PIPE_CRC_SOURCE_PLANE7:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int
 intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 			  const enum intel_pipe_crc_source source)
@@ -494,8 +564,10 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 		return vlv_crc_source_valid(dev_priv, source);
 	else if (IS_GEN_RANGE(dev_priv, 5, 6))
 		return ilk_crc_source_valid(dev_priv, source);
-	else
+	else if (INTEL_GEN(dev_priv) < 9)
 		return ivb_crc_source_valid(dev_priv, source);
+	else
+		return skl_crc_source_valid(dev_priv, source);
 }
 
 const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
-- 
2.19.2

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915: Remove the "pf" crc source

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Remove the "pf" crc source
URL   : https://patchwork.freedesktop.org/series/56692/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
1d7270fc22b4 drm/i915: Remove the "pf" crc source
4c9ccf4ebd34 drm/i915: Use named initializers for the crc source name array
647cd3d3a7e8 drm/i915: Remove the broken DP CRC support for g4x
0434615d44ce drm/i915: Extend skl+ crc sources with more planes
-:78: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u32' over 'uint32_t'
#78: FILE: drivers/gpu/drm/i915/intel_pipe_crc.c:379:
+				uint32_t *val,

total: 0 errors, 0 warnings, 1 checks, 134 lines checked

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

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915: Remove the "pf" crc source

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Remove the "pf" crc source
URL   : https://patchwork.freedesktop.org/series/56692/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Remove the "pf" crc source
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3566:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3565:16: warning: expression using sizeof(void)

Commit: drm/i915: Use named initializers for the crc source name array
Okay!

Commit: drm/i915: Remove the broken DP CRC support for g4x
Okay!

Commit: drm/i915: Extend skl+ crc sources with more planes
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3565:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3570:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Remove the "pf" crc source

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Remove the "pf" crc source
URL   : https://patchwork.freedesktop.org/series/56692/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5602 -> Patchwork_12224
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56692/revisions/1/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       PASS -> DMESG-WARN [fdo#107709]

  * igt@i915_selftest@live_hangcheck:
    - fi-kbl-7560u:       PASS -> INCOMPLETE [fdo#108044] / [fdo#108744]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@i915_selftest@live_workarounds:
    - {fi-icl-u3}:        INCOMPLETE [fdo#109626] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-byt-clapper:     FAIL [fdo#107362] -> PASS

  * igt@pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       {SKIP} [fdo#109271] -> PASS

  * igt@pm_rpm@basic-rte:
    - fi-bsw-kefka:       FAIL [fdo#108800] -> PASS

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

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109226]: https://bugs.freedesktop.org/show_bug.cgi?id=109226
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109527]: https://bugs.freedesktop.org/show_bug.cgi?id=109527
  [fdo#109528]: https://bugs.freedesktop.org/show_bug.cgi?id=109528
  [fdo#109530]: https://bugs.freedesktop.org/show_bug.cgi?id=109530
  [fdo#109626]: https://bugs.freedesktop.org/show_bug.cgi?id=109626


Participating hosts (48 -> 42)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-glk-j4005 fi-skl-6700hq fi-bdw-samus 


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

    * Linux: CI_DRM_5602 -> Patchwork_12224

  CI_DRM_5602: 570d4d9a80d29c77155979e5fdfb2e1ba76057c7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12224: 0434615d44ce8f4dc908e836f6981eceb17903dc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0434615d44ce drm/i915: Extend skl+ crc sources with more planes
647cd3d3a7e8 drm/i915: Remove the broken DP CRC support for g4x
4c9ccf4ebd34 drm/i915: Use named initializers for the crc source name array
1d7270fc22b4 drm/i915: Remove the "pf" crc source

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Remove the "pf" crc source

[Rodrigo Vivi]

On Thu, Feb 14, 2019 at 09:22:16PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The "pipe" and "pf" crc sources are in fact the same thing.
> Remove the "pf" one.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I wonder where this came from....

Anyway, just by looking the current code:

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



> ---
>  drivers/gpu/drm/i915/i915_drv.h       | 1 -
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 17fe942eaafa..4e11d970cbcf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1196,7 +1196,6 @@ enum intel_pipe_crc_source {
>  	INTEL_PIPE_CRC_SOURCE_NONE,
>  	INTEL_PIPE_CRC_SOURCE_PLANE1,
>  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> -	INTEL_PIPE_CRC_SOURCE_PF,
>  	INTEL_PIPE_CRC_SOURCE_PIPE,
>  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
>  	INTEL_PIPE_CRC_SOURCE_TV,
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a8554dc4f196..a3a3ad760158 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -34,7 +34,6 @@ static const char * const pipe_crc_sources[] = {
>  	"none",
>  	"plane1",
>  	"plane2",
> -	"pf",
>  	"pipe",
>  	"TV",
>  	"DP-B",
> @@ -396,7 +395,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				bool set_wa)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> -		*source = INTEL_PIPE_CRC_SOURCE_PF;
> +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
>  
>  	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> @@ -405,7 +404,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	case INTEL_PIPE_CRC_SOURCE_PLANE2:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
> -	case INTEL_PIPE_CRC_SOURCE_PF:
> +	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  		if (set_wa && (IS_HASWELL(dev_priv) ||
>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>  			hsw_pipe_A_crc_wa(dev_priv, true);
> @@ -532,7 +531,6 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
>  	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> -	case INTEL_PIPE_CRC_SOURCE_PF:
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		return 0;
>  	default:
> -- 
> 2.19.2
> 
> _______________________________________________
> 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

Re: [PATCH 2/4] drm/i915: Use named initializers for the crc source name array

[Rodrigo Vivi]

On Thu, Feb 14, 2019 at 09:22:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We assume that the index of the string in the crc source names
> array matches the enum value for the crc source. Let's use named
> initializers to make sure that is indeed the case even if someone
> rearranges either the enum or the array.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a3a3ad760158..fe0ff89b980b 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -31,15 +31,15 @@
>  #include "intel_drv.h"
>  
>  static const char * const pipe_crc_sources[] = {
> -	"none",
> -	"plane1",
> -	"plane2",
> -	"pipe",
> -	"TV",
> -	"DP-B",
> -	"DP-C",
> -	"DP-D",
> -	"auto",
> +	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> +	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
> +	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
> +	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> +	[INTEL_PIPE_CRC_SOURCE_DP_C] = "DP-C",
> +	[INTEL_PIPE_CRC_SOURCE_DP_D] = "DP-D",
> +	[INTEL_PIPE_CRC_SOURCE_AUTO] = "auto",
>  };
>  
>  static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> -- 
> 2.19.2
> 
> _______________________________________________
> 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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Rodrigo Vivi]

On Thu, Feb 14, 2019 at 09:22:18PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP CRCs don't really work on g4x. If you want any CRCs on DP you must
> select the CRC source before the port is enabled, otherwise the CRC
> source select bits simply ignore any writes to them. And once the port
> is enabled we mustn't change the CRC source select until the port is
> disabled. That almost works, but not quite :( Eventually the CRC source
> select bits get permanently stuck one way or the other, and after that
> a reboot (or possibly a display reset) is needed to get working CRCs
> on that pipe (not matter which CRC source we try to use).
> 
> Additionally the DFT scrambler reset bits we're trying to use don't
> seem to exist on g4x. There are some potentially relevant looking bits
> in the pipe registers, but when I tried it I got stable looking CRCs
> without setting any bits for this.
> 
> If there is a way to make DP CRCs work reliably on g4x, I wasn't
> able to find it. So let's just remove the broken code we have.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-----------------------
>  1 file changed, 11 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index fe0ff89b980b..66bb7b031537 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				 enum intel_pipe_crc_source *source,
>  				 u32 *val)
>  {
> -	bool need_stable_symbols = false;
> -
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
>  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe, source);
>  		if (ret)
> @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			return -EINVAL;
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
>  		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> -		need_stable_symbols = true;
> -		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> -		need_stable_symbols = true;
> -		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> -		need_stable_symbols = true;
> -		break;
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		*val = 0;
>  		break;
>  	default:
> +		/*
> +		 * The DP CRC source doesn't work on g4x.
> +		 * It can be made to work to some degree by selecting
> +		 * the correct CRC source before the port is enabled,
> +		 * and not touching the CRC source bits again until
> +		 * the port is disabled. But even then the bits
> +		 * eventually get stuck and a reboot is needed to get
> +		 * working CRCs on the pipe again. Let's simply
> +		 * refuse to use DP CRCs on g4x.
> +		 */
>  		return -EINVAL;

is this the right return now? maybe ENOENT?
(just brainstorming without looking to igt tests)

But I know how terrible unreliable crcs are and this patch
looks the right way, so:

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



>  	}
>  
> -	/*
> -	 * When the pipe CRC tap point is after the transcoders we need
> -	 * to tweak symbol-level features to produce a deterministic series of
> -	 * symbols for a given frame. We need to reset those features only once
> -	 * a frame (instead of every nth symbol):
> -	 *   - DC-balance: used to ensure a better clock recovery from the data
> -	 *     link (SDVO)
> -	 *   - DisplayPort scrambling: used for EMI reduction
> -	 */
> -	if (need_stable_symbols) {
> -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> -
> -		WARN_ON(!IS_G4X(dev_priv));
> -
> -		I915_WRITE(PORT_DFT_I9XX,
> -			   I915_READ(PORT_DFT_I9XX) | DC_BALANCE_RESET);
> -
> -		if (pipe == PIPE_A)
> -			tmp |= PIPE_A_SCRAMBLE_RESET;
> -		else
> -			tmp |= PIPE_B_SCRAMBLE_RESET;
> -
> -		I915_WRITE(PORT_DFT2_G4X, tmp);
> -	}
> -
>  	return 0;
>  }
>  
> @@ -282,24 +247,6 @@ static void vlv_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
>  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
>  		tmp &= ~DC_BALANCE_RESET_VLV;
>  	I915_WRITE(PORT_DFT2_G4X, tmp);
> -
> -}
> -
> -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
> -					 enum pipe pipe)
> -{
> -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> -
> -	if (pipe == PIPE_A)
> -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> -	else
> -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> -	I915_WRITE(PORT_DFT2_G4X, tmp);
> -
> -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> -		I915_WRITE(PORT_DFT_I9XX,
> -			   I915_READ(PORT_DFT_I9XX) & ~DC_BALANCE_RESET);
> -	}
>  }
>  
>  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
>  	switch (source) {
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  	case INTEL_PIPE_CRC_SOURCE_TV:
> -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> -	case INTEL_PIPE_CRC_SOURCE_DP_D:
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		return 0;
>  	default:
> @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>  
>  	if (!source) {
> -		if (IS_G4X(dev_priv))
> -			g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
> -		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
>  		else if ((IS_HASWELL(dev_priv) ||
>  			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
> -- 
> 2.19.2
> 
> _______________________________________________
> 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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Ville Syrjälä]

On Thu, Feb 14, 2019 at 12:38:22PM -0800, Rodrigo Vivi wrote:
> On Thu, Feb 14, 2019 at 09:22:18PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DP CRCs don't really work on g4x. If you want any CRCs on DP you must
> > select the CRC source before the port is enabled, otherwise the CRC
> > source select bits simply ignore any writes to them. And once the port
> > is enabled we mustn't change the CRC source select until the port is
> > disabled. That almost works, but not quite :( Eventually the CRC source
> > select bits get permanently stuck one way or the other, and after that
> > a reboot (or possibly a display reset) is needed to get working CRCs
> > on that pipe (not matter which CRC source we try to use).
> > 
> > Additionally the DFT scrambler reset bits we're trying to use don't
> > seem to exist on g4x. There are some potentially relevant looking bits
> > in the pipe registers, but when I tried it I got stable looking CRCs
> > without setting any bits for this.
> > 
> > If there is a way to make DP CRCs work reliably on g4x, I wasn't
> > able to find it. So let's just remove the broken code we have.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-----------------------
> >  1 file changed, 11 insertions(+), 69 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index fe0ff89b980b..66bb7b031537 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  				 enum intel_pipe_crc_source *source,
> >  				 u32 *val)
> >  {
> > -	bool need_stable_symbols = false;
> > -
> >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> >  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe, source);
> >  		if (ret)
> > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  			return -EINVAL;
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
> >  		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> >  		*val = 0;
> >  		break;
> >  	default:
> > +		/*
> > +		 * The DP CRC source doesn't work on g4x.
> > +		 * It can be made to work to some degree by selecting
> > +		 * the correct CRC source before the port is enabled,
> > +		 * and not touching the CRC source bits again until
> > +		 * the port is disabled. But even then the bits
> > +		 * eventually get stuck and a reboot is needed to get
> > +		 * working CRCs on the pipe again. Let's simply
> > +		 * refuse to use DP CRCs on g4x.
> > +		 */
> >  		return -EINVAL;
> 
> is this the right return now? maybe ENOENT?
> (just brainstorming without looking to igt tests)

We return -EINVAL for all other unsupported sources, so this
seems consistent.

> 
> But I know how terrible unreliable crcs are and this patch
> looks the right way, so:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> 
> >  	}
> >  
> > -	/*
> > -	 * When the pipe CRC tap point is after the transcoders we need
> > -	 * to tweak symbol-level features to produce a deterministic series of
> > -	 * symbols for a given frame. We need to reset those features only once
> > -	 * a frame (instead of every nth symbol):
> > -	 *   - DC-balance: used to ensure a better clock recovery from the data
> > -	 *     link (SDVO)
> > -	 *   - DisplayPort scrambling: used for EMI reduction
> > -	 */
> > -	if (need_stable_symbols) {
> > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > -
> > -		WARN_ON(!IS_G4X(dev_priv));
> > -
> > -		I915_WRITE(PORT_DFT_I9XX,
> > -			   I915_READ(PORT_DFT_I9XX) | DC_BALANCE_RESET);
> > -
> > -		if (pipe == PIPE_A)
> > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > -		else
> > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > -
> > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > @@ -282,24 +247,6 @@ static void vlv_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
> >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> >  		tmp &= ~DC_BALANCE_RESET_VLV;
> >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > -
> > -}
> > -
> > -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private *dev_priv,
> > -					 enum pipe pipe)
> > -{
> > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > -
> > -	if (pipe == PIPE_A)
> > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > -	else
> > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > -
> > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > -		I915_WRITE(PORT_DFT_I9XX,
> > -			   I915_READ(PORT_DFT_I9XX) & ~DC_BALANCE_RESET);
> > -	}
> >  }
> >  
> >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
> >  	switch (source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> >  		return 0;
> >  	default:
> > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
> >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> >  
> >  	if (!source) {
> > -		if (IS_G4X(dev_priv))
> > -			g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
> > -		else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
> >  		else if ((IS_HASWELL(dev_priv) ||
> >  			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
> > -- 
> > 2.19.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Rodrigo Vivi]

On Thu, Feb 14, 2019 at 09:22:19PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On skl the crc registers were extended to provide plane crcs
> for up to 7 planes. Add the new crc sources.
> 
> The current code uses the ivb+ register definitions for skl+
> which does happen to work as the plane1, plane2, and dmux/pf
> bits happen the match what ivb+ had. So no bug in the current
> code.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  5 ++
>  drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 76 ++++++++++++++++++++++++++-
>  3 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e11d970cbcf..8607c1e9ed02 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
>  	INTEL_PIPE_CRC_SOURCE_NONE,
>  	INTEL_PIPE_CRC_SOURCE_PLANE1,
>  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> +	INTEL_PIPE_CRC_SOURCE_PLANE3,
> +	INTEL_PIPE_CRC_SOURCE_PLANE4,
> +	INTEL_PIPE_CRC_SOURCE_PLANE5,
> +	INTEL_PIPE_CRC_SOURCE_PLANE6,
> +	INTEL_PIPE_CRC_SOURCE_PLANE7,
>  	INTEL_PIPE_CRC_SOURCE_PIPE,
>  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
>  	INTEL_PIPE_CRC_SOURCE_TV,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0df8c6e76da7..5286536e9cb8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4017,6 +4017,15 @@ enum {
>  /* Pipe A CRC regs */
>  #define _PIPE_CRC_CTL_A			0x60050
>  #define   PIPE_CRC_ENABLE		(1 << 31)
> +/* skl+ source selection */
> +#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
> +#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)

I got myself staring at spec for a while trying to
understand the logic of this sequence...


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



>  /* ivb+ source selection */
>  #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
>  #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 66bb7b031537..e521f82ba5d9 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
>  	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
>  	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
>  	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
>  	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
>  	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
>  	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> @@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	return 0;
>  }
>  
> +static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> +				enum pipe pipe,
> +				enum intel_pipe_crc_source *source,
> +				uint32_t *val,
> +				bool set_wa)
> +{
> +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> +
> +	switch (*source) {
> +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_1_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_2_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_3_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_4_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_5_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_6_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_7_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DMUX_SKL;
> +		break;
> +	case INTEL_PIPE_CRC_SOURCE_NONE:
> +		*val = 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
>  			       enum intel_pipe_crc_source *source, u32 *val,
> @@ -381,8 +430,10 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
>  		return ilk_pipe_crc_ctl_reg(source, val);
> -	else
> +	else if (INTEL_GEN(dev_priv) < 9)
>  		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> +	else
> +		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>  }
>  
>  static int
> @@ -482,6 +533,25 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static int skl_crc_source_valid(struct drm_i915_private *dev_priv,
> +				const enum intel_pipe_crc_source source)
> +{
> +	switch (source) {
> +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> +	case INTEL_PIPE_CRC_SOURCE_NONE:
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int
>  intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
>  			  const enum intel_pipe_crc_source source)
> @@ -494,8 +564,10 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
>  		return vlv_crc_source_valid(dev_priv, source);
>  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
>  		return ilk_crc_source_valid(dev_priv, source);
> -	else
> +	else if (INTEL_GEN(dev_priv) < 9)
>  		return ivb_crc_source_valid(dev_priv, source);
> +	else
> +		return skl_crc_source_valid(dev_priv, source);
>  }
>  
>  const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
> -- 
> 2.19.2
> 
> _______________________________________________
> 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

Re: [PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Ville Syrjälä]

On Thu, Feb 14, 2019 at 12:47:23PM -0800, Rodrigo Vivi wrote:
> On Thu, Feb 14, 2019 at 09:22:19PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On skl the crc registers were extended to provide plane crcs
> > for up to 7 planes. Add the new crc sources.
> > 
> > The current code uses the ivb+ register definitions for skl+
> > which does happen to work as the plane1, plane2, and dmux/pf
> > bits happen the match what ivb+ had. So no bug in the current
> > code.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  5 ++
> >  drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 76 ++++++++++++++++++++++++++-
> >  3 files changed, 88 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4e11d970cbcf..8607c1e9ed02 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
> >  	INTEL_PIPE_CRC_SOURCE_NONE,
> >  	INTEL_PIPE_CRC_SOURCE_PLANE1,
> >  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE3,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE4,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE5,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE6,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE7,
> >  	INTEL_PIPE_CRC_SOURCE_PIPE,
> >  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
> >  	INTEL_PIPE_CRC_SOURCE_TV,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0df8c6e76da7..5286536e9cb8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4017,6 +4017,15 @@ enum {
> >  /* Pipe A CRC regs */
> >  #define _PIPE_CRC_CTL_A			0x60050
> >  #define   PIPE_CRC_ENABLE		(1 << 31)
> > +/* skl+ source selection */
> > +#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
> > +#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)
> 
> I got myself staring at spec for a while trying to
> understand the logic of this sequence...

Did you find any logic in it? I honestly can't remember anymore why I
did it like this.

> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> 
> >  /* ivb+ source selection */
> >  #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
> >  #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 66bb7b031537..e521f82ba5d9 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
> >  	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
> >  	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
> >  	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
> >  	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
> >  	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
> >  	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> > @@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  	return 0;
> >  }
> >  
> > +static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > +				enum pipe pipe,
> > +				enum intel_pipe_crc_source *source,
> > +				uint32_t *val,
> > +				bool set_wa)
> > +{
> > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > +
> > +	switch (*source) {
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_1_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_2_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_3_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_4_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_5_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_6_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_7_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DMUX_SKL;
> > +		break;
> > +	case INTEL_PIPE_CRC_SOURCE_NONE:
> > +		*val = 0;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe,
> >  			       enum intel_pipe_crc_source *source, u32 *val,
> > @@ -381,8 +430,10 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  		return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> >  		return ilk_pipe_crc_ctl_reg(source, val);
> > -	else
> > +	else if (INTEL_GEN(dev_priv) < 9)
> >  		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> > +	else
> > +		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> >  }
> >  
> >  static int
> > @@ -482,6 +533,25 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +static int skl_crc_source_valid(struct drm_i915_private *dev_priv,
> > +				const enum intel_pipe_crc_source source)
> > +{
> > +	switch (source) {
> > +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> > +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> > +	case INTEL_PIPE_CRC_SOURCE_NONE:
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int
> >  intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> >  			  const enum intel_pipe_crc_source source)
> > @@ -494,8 +564,10 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> >  		return vlv_crc_source_valid(dev_priv, source);
> >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> >  		return ilk_crc_source_valid(dev_priv, source);
> > -	else
> > +	else if (INTEL_GEN(dev_priv) < 9)
> >  		return ivb_crc_source_valid(dev_priv, source);
> > +	else
> > +		return skl_crc_source_valid(dev_priv, source);
> >  }
> >  
> >  const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
> > -- 
> > 2.19.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Rodrigo Vivi]

On Thu, Feb 14, 2019 at 11:29:08PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 14, 2019 at 12:47:23PM -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 14, 2019 at 09:22:19PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On skl the crc registers were extended to provide plane crcs
> > > for up to 7 planes. Add the new crc sources.
> > > 
> > > The current code uses the ivb+ register definitions for skl+
> > > which does happen to work as the plane1, plane2, and dmux/pf
> > > bits happen the match what ivb+ had. So no bug in the current
> > > code.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h       |  5 ++
> > >  drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
> > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 76 ++++++++++++++++++++++++++-
> > >  3 files changed, 88 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4e11d970cbcf..8607c1e9ed02 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
> > >  	INTEL_PIPE_CRC_SOURCE_NONE,
> > >  	INTEL_PIPE_CRC_SOURCE_PLANE1,
> > >  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> > > +	INTEL_PIPE_CRC_SOURCE_PLANE3,
> > > +	INTEL_PIPE_CRC_SOURCE_PLANE4,
> > > +	INTEL_PIPE_CRC_SOURCE_PLANE5,
> > > +	INTEL_PIPE_CRC_SOURCE_PLANE6,
> > > +	INTEL_PIPE_CRC_SOURCE_PLANE7,
> > >  	INTEL_PIPE_CRC_SOURCE_PIPE,
> > >  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
> > >  	INTEL_PIPE_CRC_SOURCE_TV,
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 0df8c6e76da7..5286536e9cb8 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4017,6 +4017,15 @@ enum {
> > >  /* Pipe A CRC regs */
> > >  #define _PIPE_CRC_CTL_A			0x60050
> > >  #define   PIPE_CRC_ENABLE		(1 << 31)
> > > +/* skl+ source selection */
> > > +#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
> > > +#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
> > > +#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)
> > 
> > I got myself staring at spec for a while trying to
> > understand the logic of this sequence...
> 
> Did you find any logic in it? I honestly can't remember anymore why I
> did it like this.

no no... I'm not criticizing you. Your way is the most
clean and organized one.

My surprise came from the fact that I couldn't find
any logic on how those bits got selected at first place. ;)

> 
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > 
> > 
> > >  /* ivb+ source selection */
> > >  #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
> > >  #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index 66bb7b031537..e521f82ba5d9 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
> > >  	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
> > >  	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
> > >  	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> > > +	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
> > > +	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
> > > +	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
> > > +	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
> > > +	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
> > >  	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
> > >  	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
> > >  	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> > > @@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > >  	return 0;
> > >  }
> > >  
> > > +static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > > +				enum pipe pipe,
> > > +				enum intel_pipe_crc_source *source,
> > > +				uint32_t *val,
> > > +				bool set_wa)
> > > +{
> > > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > +
> > > +	switch (*source) {
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_1_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_2_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_3_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_4_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_5_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_6_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PLANE_7_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > +		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DMUX_SKL;
> > > +		break;
> > > +	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > +		*val = 0;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > >  			       enum pipe pipe,
> > >  			       enum intel_pipe_crc_source *source, u32 *val,
> > > @@ -381,8 +430,10 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > >  		return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> > >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > >  		return ilk_pipe_crc_ctl_reg(source, val);
> > > -	else
> > > +	else if (INTEL_GEN(dev_priv) < 9)
> > >  		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> > > +	else
> > > +		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> > >  }
> > >  
> > >  static int
> > > @@ -482,6 +533,25 @@ static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > +static int skl_crc_source_valid(struct drm_i915_private *dev_priv,
> > > +				const enum intel_pipe_crc_source source)
> > > +{
> > > +	switch (source) {
> > > +	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE3:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE4:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE5:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE6:
> > > +	case INTEL_PIPE_CRC_SOURCE_PLANE7:
> > > +	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > >  static int
> > >  intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> > >  			  const enum intel_pipe_crc_source source)
> > > @@ -494,8 +564,10 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> > >  		return vlv_crc_source_valid(dev_priv, source);
> > >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > >  		return ilk_crc_source_valid(dev_priv, source);
> > > -	else
> > > +	else if (INTEL_GEN(dev_priv) < 9)
> > >  		return ivb_crc_source_valid(dev_priv, source);
> > > +	else
> > > +		return skl_crc_source_valid(dev_priv, source);
> > >  }
> > >  
> > >  const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
> > > -- 
> > > 2.19.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Remove the "pf" crc source

[Dhinakaran Pandiyan]

On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The "pipe" and "pf" crc sources are in fact the same thing.
> Remove the "pf" one.

And BDW+ seem to call it DMUX output.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       | 1 -
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 17fe942eaafa..4e11d970cbcf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1196,7 +1196,6 @@ enum intel_pipe_crc_source {
>  	INTEL_PIPE_CRC_SOURCE_NONE,
>  	INTEL_PIPE_CRC_SOURCE_PLANE1,
>  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> -	INTEL_PIPE_CRC_SOURCE_PF,
>  	INTEL_PIPE_CRC_SOURCE_PIPE,
>  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
>  	INTEL_PIPE_CRC_SOURCE_TV,
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a8554dc4f196..a3a3ad760158 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -34,7 +34,6 @@ static const char * const pipe_crc_sources[] = {
>  	"none",
>  	"plane1",
>  	"plane2",
> -	"pf",
>  	"pipe",
>  	"TV",
>  	"DP-B",
> @@ -396,7 +395,7 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  				bool set_wa)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> -		*source = INTEL_PIPE_CRC_SOURCE_PF;
> +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
>  
>  	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> @@ -405,7 +404,7 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  	case INTEL_PIPE_CRC_SOURCE_PLANE2:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
> -	case INTEL_PIPE_CRC_SOURCE_PF:
> +	case INTEL_PIPE_CRC_SOURCE_PIPE:

Ah, source == "pipe" would have returned a failure here
although ivb_crc_source_valid() considers it a valid source. This
patch fixes that too.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

>  		if (set_wa && (IS_HASWELL(dev_priv) ||
>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>  			hsw_pipe_A_crc_wa(dev_priv, true);
> @@ -532,7 +531,6 @@ static int ivb_crc_source_valid(struct
> drm_i915_private *dev_priv,
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
>  	case INTEL_PIPE_CRC_SOURCE_PLANE2:
> -	case INTEL_PIPE_CRC_SOURCE_PF:
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		return 0;
>  	default:

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

Re: [PATCH 1/4] drm/i915: Remove the "pf" crc source

[Dhinakaran Pandiyan]

On Thu, 2019-02-14 at 17:32 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The "pipe" and "pf" crc sources are in fact the same thing.
> > Remove the "pf" one.
> 
> And BDW+ seem to call it DMUX output.

Oh, we need to remove this in IGT too.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---



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

Re: [PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Dhinakaran Pandiyan]

On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On skl the crc registers were extended to provide plane crcs
> for up to 7 planes. Add the new crc sources.
> 
> The current code uses the ivb+ register definitions for skl+
> which does happen to work as the plane1, plane2, and dmux/pf
> bits happen the match what ivb+ had. So no bug in the current
> code.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  5 ++
>  drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 76
> ++++++++++++++++++++++++++-
>  3 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4e11d970cbcf..8607c1e9ed02 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
>  	INTEL_PIPE_CRC_SOURCE_NONE,
>  	INTEL_PIPE_CRC_SOURCE_PLANE1,
>  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> +	INTEL_PIPE_CRC_SOURCE_PLANE3,
> +	INTEL_PIPE_CRC_SOURCE_PLANE4,
> +	INTEL_PIPE_CRC_SOURCE_PLANE5,
> +	INTEL_PIPE_CRC_SOURCE_PLANE6,
> +	INTEL_PIPE_CRC_SOURCE_PLANE7,
>  	INTEL_PIPE_CRC_SOURCE_PIPE,
>  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
>  	INTEL_PIPE_CRC_SOURCE_TV,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 0df8c6e76da7..5286536e9cb8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4017,6 +4017,15 @@ enum {
>  /* Pipe A CRC regs */
>  #define _PIPE_CRC_CTL_A			0x60050
>  #define   PIPE_CRC_ENABLE		(1 << 31)
> +/* skl+ source selection */
> +#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
> +#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
> +#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)
>  /* ivb+ source selection */
>  #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
>  #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 66bb7b031537..e521f82ba5d9 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
>  	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
>  	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
>  	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
> +	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
>  	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
>  	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
>  	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> @@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  	return 0;
>  }
>  
> +static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> +				enum pipe pipe,
> +				enum intel_pipe_crc_source *source,
> +				uint32_t *val,
> +				bool set_wa)

set_wa is unused. 




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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Dhinakaran Pandiyan]

On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP CRCs don't really work on g4x. If you want any CRCs on DP you must
> select the CRC source before the port is enabled, otherwise the CRC
> source select bits simply ignore any writes to them. And once the
> port
> is enabled we mustn't change the CRC source select until the port is
> disabled. That almost works, but not quite :( Eventually the CRC
> source
> select bits get permanently stuck one way or the other, and after
> that
> a reboot (or possibly a display reset) is needed to get working CRCs
> on that pipe (not matter which CRC source we try to use).
> 
> Additionally the DFT scrambler reset bits we're trying to use don't
> seem to exist on g4x. There are some potentially relevant looking
> bits
> in the pipe registers, but when I tried it I got stable looking CRCs
> without setting any bits for this.
> 
> If there is a way to make DP CRCs work reliably on g4x, I wasn't
> able to find it. So let's just remove the broken code we have.


I think we can modify i9xx_pipe_crc_auto_source() to pick "pipe" CRC
when userspace selects "auto" and the output is DP/eDP.


> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++---------------------
> --
>  1 file changed, 11 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index fe0ff89b980b..66bb7b031537 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  				 enum intel_pipe_crc_source *source,
>  				 u32 *val)
>  {
> -	bool need_stable_symbols = false;
> -
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
>  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe,
> source);
>  		if (ret)
> @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  			return -EINVAL;
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
>  		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> -		need_stable_symbols = true;
> -		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> -		need_stable_symbols = true;
> -		break;
> -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> -		if (!IS_G4X(dev_priv))
> -			return -EINVAL;
> -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> -		need_stable_symbols = true;
> -		break;
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		*val = 0;
>  		break;
>  	default:
> +		/*
> +		 * The DP CRC source doesn't work on g4x.
> +		 * It can be made to work to some degree by selecting
> +		 * the correct CRC source before the port is enabled,
> +		 * and not touching the CRC source bits again until
> +		 * the port is disabled. But even then the bits
> +		 * eventually get stuck and a reboot is needed to get
> +		 * working CRCs on the pipe again. Let's simply
> +		 * refuse to use DP CRCs on g4x.
> +		 */z
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * When the pipe CRC tap point is after the transcoders we need
> -	 * to tweak symbol-level features to produce a deterministic
> series of
> -	 * symbols for a given frame. We need to reset those features
> only once
> -	 * a frame (instead of every nth symbol):
> -	 *   - DC-balance: used to ensure a better clock recovery from
> the data
> -	 *     link (SDVO)
> -	 *   - DisplayPort scrambling: used for EMI reduction
> -	 */
> -	if (need_stable_symbols) {
> -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> -
> -		WARN_ON(!IS_G4X(dev_priv));
> -
> -		I915_WRITE(PORT_DFT_I9XX,
> -			   I915_READ(PORT_DFT_I9XX) |
> DC_BALANCE_RESET);
> -
> -		if (pipe == PIPE_A)
> -			tmp |= PIPE_A_SCRAMBLE_RESET;
> -		else
> -			tmp |= PIPE_B_SCRAMBLE_RESET;
> -
> -		I915_WRITE(PORT_DFT2_G4X, tmp);
> -	}
> -
>  	return 0;
>  }
>  
> @@ -282,24 +247,6 @@ static void vlv_undo_pipe_scramble_reset(struct
> drm_i915_private *dev_priv,
>  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
>  		tmp &= ~DC_BALANCE_RESET_VLV;
>  	I915_WRITE(PORT_DFT2_G4X, tmp);
> -
> -}
> -
> -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private
> *dev_priv,
> -					 enum pipe pipe)
> -{
> -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> -
> -	if (pipe == PIPE_A)
> -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> -	else
> -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> -	I915_WRITE(PORT_DFT2_G4X, tmp);
> -
> -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> -		I915_WRITE(PORT_DFT_I9XX,
> -			   I915_READ(PORT_DFT_I9XX) &
> ~DC_BALANCE_RESET);
> -	}
>  }
>  
>  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> drm_i915_private *dev_priv,
>  	switch (source) {
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  	case INTEL_PIPE_CRC_SOURCE_TV:
> -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> -	case INTEL_PIPE_CRC_SOURCE_DP_D:
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
>  		return 0;
>  	default:
> @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>  
>  	if (!source) {
> -		if (IS_G4X(dev_priv))
> -			g4x_undo_pipe_scramble_reset(dev_priv, crtc-
> >index);
> -		else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> >index);
>  		else if ((IS_HASWELL(dev_priv) ||
>  			  IS_BROADWELL(dev_priv)) && crtc->index ==
> PIPE_A)

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

✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915: Remove the "pf" crc source

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Remove the "pf" crc source
URL   : https://patchwork.freedesktop.org/series/56692/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5602_full -> Patchwork_12224_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12224_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12224_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12224_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
    - shard-apl:          PASS -> FAIL +2

  * igt@pm_rpm@gem-evict-pwrite:
    - shard-apl:          PASS -> DMESG-WARN

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@kms_psr2_su@page_flip}:
    - shard-iclb:         NOTRUN -> {SKIP}

  * {igt@runner@aborted}:
    - shard-apl:          NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-suspend:
    - shard-snb:          PASS -> FAIL [fdo#103375]

  * igt@gem_exec_reloc@basic-cpu-gtt-noreloc:
    - shard-apl:          PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] / [fdo#109373]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_color@pipe-c-legacy-gamma:
    - shard-glk:          PASS -> FAIL [fdo#104782]

  * igt@kms_cursor_crc@cursor-128x42-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_cursor_crc@cursor-128x42-sliding:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_fbcon_fbt@psr:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103833]

  * igt@kms_flip@modeset-vs-vblank-race-interruptible:
    - shard-glk:          PASS -> FAIL [fdo#103060]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-glk:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +2

  * igt@kms_plane@pixel-format-pipe-a-planes:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-iclb:         PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +1

  * igt@kms_psr@no_drrs:
    - shard-iclb:         PASS -> FAIL [fdo#108341]

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-apl:          NOTRUN -> FAIL [fdo#103166]

  * igt@kms_universal_plane@universal-plane-pipe-b-functional:
    - shard-apl:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          PASS -> FAIL [fdo#104894]

  * igt@pm_rpm@cursor:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724]

  
#### Possible fixes ####

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS

  * igt@kms_chv_cursor_fail@pipe-b-64x64-bottom-edge:
    - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +18

  * igt@kms_color@pipe-b-degamma:
    - shard-apl:          FAIL [fdo#104782] -> PASS

  * igt@kms_color@pipe-b-legacy-gamma:
    - shard-glk:          FAIL [fdo#104782] -> PASS

  * igt@kms_color@pipe-c-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-apl:          FAIL [fdo#109350] -> PASS

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-glk:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-glk:          FAIL [fdo#103167] / [fdo#105682] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +2

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602] -> PASS +1

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-iclb:         FAIL [fdo#103166] -> PASS +1

  * igt@pm_rpm@basic-rte:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +3

  * igt@pm_rpm@legacy-planes:
    - shard-iclb:         INCOMPLETE [fdo#108840] / [fdo#109369] -> PASS

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - shard-iclb:         DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569] / [fdo#109226]

  * igt@i915_suspend@shrink:
    - shard-snb:          INCOMPLETE [fdo#105411] / [fdo#106886] -> DMESG-WARN [fdo#109244]

  * igt@kms_cursor_crc@cursor-128x128-onscreen:
    - shard-kbl:          DMESG-FAIL [fdo#103232] / [fdo#103558] / [fdo#105602] -> FAIL [fdo#103232] +1

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-kbl:          DMESG-FAIL [fdo#103558] / [fdo#105602] / [fdo#108145] -> FAIL [fdo#108145] / [fdo#108590]

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

  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103833]: https://bugs.freedesktop.org/show_bug.cgi?id=103833
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109226]: https://bugs.freedesktop.org/show_bug.cgi?id=109226
  [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109293]: https://bugs.freedesktop.org/show_bug.cgi?id=109293
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109369]: https://bugs.freedesktop.org/show_bug.cgi?id=109369
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109625]: https://bugs.freedesktop.org/show_bug.cgi?id=109625


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


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

    * Linux: CI_DRM_5602 -> Patchwork_12224

  CI_DRM_5602: 570d4d9a80d29c77155979e5fdfb2e1ba76057c7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12224: 0434615d44ce8f4dc908e836f6981eceb17903dc @ 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_12224/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Ville Syrjälä]

On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DP CRCs don't really work on g4x. If you want any CRCs on DP you must
> > select the CRC source before the port is enabled, otherwise the CRC
> > source select bits simply ignore any writes to them. And once the
> > port
> > is enabled we mustn't change the CRC source select until the port is
> > disabled. That almost works, but not quite :( Eventually the CRC
> > source
> > select bits get permanently stuck one way or the other, and after
> > that
> > a reboot (or possibly a display reset) is needed to get working CRCs
> > on that pipe (not matter which CRC source we try to use).
> > 
> > Additionally the DFT scrambler reset bits we're trying to use don't
> > seem to exist on g4x. There are some potentially relevant looking
> > bits
> > in the pipe registers, but when I tried it I got stable looking CRCs
> > without setting any bits for this.
> > 
> > If there is a way to make DP CRCs work reliably on g4x, I wasn't
> > able to find it. So let's just remove the broken code we have.
> 
> 
> I think we can modify i9xx_pipe_crc_auto_source() to pick "pipe" CRC
> when userspace selects "auto" and the output is DP/eDP.

Nope. Spec says:
"Pipe CRC should not be run when Display Port or TV is enabled on this
 pipe."

and

"CRC Source Select: These bits select the source of the data to put into
 the CRC logic.
 000: Pipe A (Not available when DisplayPort or TV is enabled on this
 pipe)"

Though I must admit I've never actually tried it to see what actually
happens.

> 
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++---------------------
> > --
> >  1 file changed, 11 insertions(+), 69 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index fe0ff89b980b..66bb7b031537 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  				 enum intel_pipe_crc_source *source,
> >  				 u32 *val)
> >  {
> > -	bool need_stable_symbols = false;
> > -
> >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> >  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe,
> > source);
> >  		if (ret)
> > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  			return -EINVAL;
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
> >  		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > -		if (!IS_G4X(dev_priv))
> > -			return -EINVAL;
> > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> > -		need_stable_symbols = true;
> > -		break;
> >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> >  		*val = 0;
> >  		break;
> >  	default:
> > +		/*
> > +		 * The DP CRC source doesn't work on g4x.
> > +		 * It can be made to work to some degree by selecting
> > +		 * the correct CRC source before the port is enabled,
> > +		 * and not touching the CRC source bits again until
> > +		 * the port is disabled. But even then the bits
> > +		 * eventually get stuck and a reboot is needed to get
> > +		 * working CRCs on the pipe again. Let's simply
> > +		 * refuse to use DP CRCs on g4x.
> > +		 */z
> >  		return -EINVAL;
> >  	}
> >  
> > -	/*
> > -	 * When the pipe CRC tap point is after the transcoders we need
> > -	 * to tweak symbol-level features to produce a deterministic
> > series of
> > -	 * symbols for a given frame. We need to reset those features
> > only once
> > -	 * a frame (instead of every nth symbol):
> > -	 *   - DC-balance: used to ensure a better clock recovery from
> > the data
> > -	 *     link (SDVO)
> > -	 *   - DisplayPort scrambling: used for EMI reduction
> > -	 */
> > -	if (need_stable_symbols) {
> > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > -
> > -		WARN_ON(!IS_G4X(dev_priv));
> > -
> > -		I915_WRITE(PORT_DFT_I9XX,
> > -			   I915_READ(PORT_DFT_I9XX) |
> > DC_BALANCE_RESET);
> > -
> > -		if (pipe == PIPE_A)
> > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > -		else
> > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > -
> > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > @@ -282,24 +247,6 @@ static void vlv_undo_pipe_scramble_reset(struct
> > drm_i915_private *dev_priv,
> >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> >  		tmp &= ~DC_BALANCE_RESET_VLV;
> >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > -
> > -}
> > -
> > -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private
> > *dev_priv,
> > -					 enum pipe pipe)
> > -{
> > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > -
> > -	if (pipe == PIPE_A)
> > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > -	else
> > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > -
> > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > -		I915_WRITE(PORT_DFT_I9XX,
> > -			   I915_READ(PORT_DFT_I9XX) &
> > ~DC_BALANCE_RESET);
> > -	}
> >  }
> >  
> >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > drm_i915_private *dev_priv,
> >  	switch (source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> >  		return 0;
> >  	default:
> > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> >  
> >  	if (!source) {
> > -		if (IS_G4X(dev_priv))
> > -			g4x_undo_pipe_scramble_reset(dev_priv, crtc-
> > >index);
> > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > >index);
> >  		else if ((IS_HASWELL(dev_priv) ||
> >  			  IS_BROADWELL(dev_priv)) && crtc->index ==
> > PIPE_A)

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

Re: [PATCH 1/4] drm/i915: Remove the "pf" crc source

[Ville Syrjälä]

On Thu, Feb 14, 2019 at 05:45:31PM -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-14 at 17:32 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The "pipe" and "pf" crc sources are in fact the same thing.
> > > Remove the "pf" one.
> > 
> > And BDW+ seem to call it DMUX output.
> 
> Oh, we need to remove this in IGT too.

Hrm. I wonder why we even have any hardcoded names in igt. It should
only really care about "auto", and if there is any test that wants
something more specific it could just hardocode the string.

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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Dhinakaran Pandiyan]

On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > DP CRCs don't really work on g4x. If you want any CRCs on DP you
> > > must
> > > select the CRC source before the port is enabled, otherwise the
> > > CRC
> > > source select bits simply ignore any writes to them. And once the
> > > port
> > > is enabled we mustn't change the CRC source select until the port
> > > is
> > > disabled. That almost works, but not quite :( Eventually the CRC
> > > source
> > > select bits get permanently stuck one way or the other, and after
> > > that
> > > a reboot (or possibly a display reset) is needed to get working
> > > CRCs
> > > on that pipe (not matter which CRC source we try to use).
> > > 
> > > Additionally the DFT scrambler reset bits we're trying to use
> > > don't
> > > seem to exist on g4x. There are some potentially relevant looking
> > > bits
> > > in the pipe registers, but when I tried it I got stable looking
> > > CRCs
> > > without setting any bits for this.
> > > 
> > > If there is a way to make DP CRCs work reliably on g4x, I wasn't
> > > able to find it. So let's just remove the broken code we have.
> > 
> > 
> > I think we can modify i9xx_pipe_crc_auto_source() to pick "pipe"
> > CRC
> > when userspace selects "auto" and the output is DP/eDP.
> 
> Nope. Spec says:
> "Pipe CRC should not be run when Display Port or TV is enabled on
> this
>  pipe."
> 
> and
> 
> "CRC Source Select: These bits select the source of the data to put
> into
>  the CRC logic.
>  000: Pipe A (Not available when DisplayPort or TV is enabled on this
>  pipe)"
> 
After digging through some old specs, I do see this restriction for
gen-4 and VLV, but for some reason not for gen-3 or CHV. 

There is no good choice for "auto" other than DP and since DP does not
work, returning -EINVAL makes sense.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Though I must admit I've never actually tried it to see what actually
> happens.
> 
> > 
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-----------------
> > > ----
> > > --
> > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index fe0ff89b980b..66bb7b031537 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > >  				 enum intel_pipe_crc_source *source,
> > >  				 u32 *val)
> > >  {
> > > -	bool need_stable_symbols = false;
> > > -
> > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe,
> > > source);
> > >  		if (ret)
> > > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > >  			return -EINVAL;
> > >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
> > >  		break;
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > -		if (!IS_G4X(dev_priv))
> > > -			return -EINVAL;
> > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> > > -		need_stable_symbols = true;
> > > -		break;
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > -		if (!IS_G4X(dev_priv))
> > > -			return -EINVAL;
> > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> > > -		need_stable_symbols = true;
> > > -		break;
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > -		if (!IS_G4X(dev_priv))
> > > -			return -EINVAL;
> > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> > > -		need_stable_symbols = true;
> > > -		break;
> > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > >  		*val = 0;
> > >  		break;
> > >  	default:
> > > +		/*
> > > +		 * The DP CRC source doesn't work on g4x.
> > > +		 * It can be made to work to some degree by selecting
> > > +		 * the correct CRC source before the port is enabled,
> > > +		 * and not touching the CRC source bits again until
> > > +		 * the port is disabled. But even then the bits
> > > +		 * eventually get stuck and a reboot is needed to get
> > > +		 * working CRCs on the pipe again. Let's simply
> > > +		 * refuse to use DP CRCs on g4x.
> > > +		 */z
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	/*
> > > -	 * When the pipe CRC tap point is after the transcoders we need
> > > -	 * to tweak symbol-level features to produce a deterministic
> > > series of
> > > -	 * symbols for a given frame. We need to reset those features
> > > only once
> > > -	 * a frame (instead of every nth symbol):
> > > -	 *   - DC-balance: used to ensure a better clock recovery from
> > > the data
> > > -	 *     link (SDVO)
> > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > -	 */
> > > -	if (need_stable_symbols) {
> > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > -
> > > -		WARN_ON(!IS_G4X(dev_priv));
> > > -
> > > -		I915_WRITE(PORT_DFT_I9XX,
> > > -			   I915_READ(PORT_DFT_I9XX) |
> > > DC_BALANCE_RESET);
> > > -
> > > -		if (pipe == PIPE_A)
> > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > -		else
> > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > -
> > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > -	}
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > @@ -282,24 +247,6 @@ static void
> > > vlv_undo_pipe_scramble_reset(struct
> > > drm_i915_private *dev_priv,
> > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > -
> > > -}
> > > -
> > > -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private
> > > *dev_priv,
> > > -					 enum pipe pipe)
> > > -{
> > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > -
> > > -	if (pipe == PIPE_A)
> > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > -	else
> > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > -
> > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > -		I915_WRITE(PORT_DFT_I9XX,
> > > -			   I915_READ(PORT_DFT_I9XX) &
> > > ~DC_BALANCE_RESET);
> > > -	}
> > >  }
> > >  
> > >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source
> > > *source,
> > > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > > drm_i915_private *dev_priv,
> > >  	switch (source) {
> > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > >  		return 0;
> > >  	default:
> > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > >  
> > >  	if (!source) {
> > > -		if (IS_G4X(dev_priv))
> > > -			g4x_undo_pipe_scramble_reset(dev_priv, crtc-
> > > > index);
> > > 
> > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv))
> > > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > > > index);
> > > 
> > >  		else if ((IS_HASWELL(dev_priv) ||
> > >  			  IS_BROADWELL(dev_priv)) && crtc->index ==
> > > PIPE_A)
> 
> 

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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Ville Syrjälä]

On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > DP CRCs don't really work on g4x. If you want any CRCs on DP you
> > > > must
> > > > select the CRC source before the port is enabled, otherwise the
> > > > CRC
> > > > source select bits simply ignore any writes to them. And once the
> > > > port
> > > > is enabled we mustn't change the CRC source select until the port
> > > > is
> > > > disabled. That almost works, but not quite :( Eventually the CRC
> > > > source
> > > > select bits get permanently stuck one way or the other, and after
> > > > that
> > > > a reboot (or possibly a display reset) is needed to get working
> > > > CRCs
> > > > on that pipe (not matter which CRC source we try to use).
> > > > 
> > > > Additionally the DFT scrambler reset bits we're trying to use
> > > > don't
> > > > seem to exist on g4x. There are some potentially relevant looking
> > > > bits
> > > > in the pipe registers, but when I tried it I got stable looking
> > > > CRCs
> > > > without setting any bits for this.
> > > > 
> > > > If there is a way to make DP CRCs work reliably on g4x, I wasn't
> > > > able to find it. So let's just remove the broken code we have.
> > > 
> > > 
> > > I think we can modify i9xx_pipe_crc_auto_source() to pick "pipe"
> > > CRC
> > > when userspace selects "auto" and the output is DP/eDP.
> > 
> > Nope. Spec says:
> > "Pipe CRC should not be run when Display Port or TV is enabled on
> > this
> >  pipe."
> > 
> > and
> > 
> > "CRC Source Select: These bits select the source of the data to put
> > into
> >  the CRC logic.
> >  000: Pipe A (Not available when DisplayPort or TV is enabled on this
> >  pipe)"
> > 
> After digging through some old specs, I do see this restriction for
> gen-4 and VLV, but for some reason not for gen-3 or CHV. 

gen3 predates DP (g4x being the first platform that has it). I don't
think SDVO->DP was ever a thing. SDVO->HDMI did happen but even that
one is quite rare.

The display engine side of CHV is 99.9% VLV, with a few extra
bits and pieces glued on top.

> 
> There is no good choice for "auto" other than DP and since DP does not
> work, returning -EINVAL makes sense.
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> > Though I must admit I've never actually tried it to see what actually
> > happens.
> > 
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-----------------
> > > > ----
> > > > --
> > > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index fe0ff89b980b..66bb7b031537 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > >  				 enum intel_pipe_crc_source *source,
> > > >  				 u32 *val)
> > > >  {
> > > > -	bool need_stable_symbols = false;
> > > > -
> > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv, pipe,
> > > > source);
> > > >  		if (ret)
> > > > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > >  			return -EINVAL;
> > > >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
> > > >  		break;
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > -		if (!IS_G4X(dev_priv))
> > > > -			return -EINVAL;
> > > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
> > > > -		need_stable_symbols = true;
> > > > -		break;
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > -		if (!IS_G4X(dev_priv))
> > > > -			return -EINVAL;
> > > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
> > > > -		need_stable_symbols = true;
> > > > -		break;
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > -		if (!IS_G4X(dev_priv))
> > > > -			return -EINVAL;
> > > > -		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
> > > > -		need_stable_symbols = true;
> > > > -		break;
> > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > >  		*val = 0;
> > > >  		break;
> > > >  	default:
> > > > +		/*
> > > > +		 * The DP CRC source doesn't work on g4x.
> > > > +		 * It can be made to work to some degree by selecting
> > > > +		 * the correct CRC source before the port is enabled,
> > > > +		 * and not touching the CRC source bits again until
> > > > +		 * the port is disabled. But even then the bits
> > > > +		 * eventually get stuck and a reboot is needed to get
> > > > +		 * working CRCs on the pipe again. Let's simply
> > > > +		 * refuse to use DP CRCs on g4x.
> > > > +		 */z
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * When the pipe CRC tap point is after the transcoders we need
> > > > -	 * to tweak symbol-level features to produce a deterministic
> > > > series of
> > > > -	 * symbols for a given frame. We need to reset those features
> > > > only once
> > > > -	 * a frame (instead of every nth symbol):
> > > > -	 *   - DC-balance: used to ensure a better clock recovery from
> > > > the data
> > > > -	 *     link (SDVO)
> > > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > > -	 */
> > > > -	if (need_stable_symbols) {
> > > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > -
> > > > -		WARN_ON(!IS_G4X(dev_priv));
> > > > -
> > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > -			   I915_READ(PORT_DFT_I9XX) |
> > > > DC_BALANCE_RESET);
> > > > -
> > > > -		if (pipe == PIPE_A)
> > > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > -		else
> > > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > -
> > > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > -	}
> > > > -
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -282,24 +247,6 @@ static void
> > > > vlv_undo_pipe_scramble_reset(struct
> > > > drm_i915_private *dev_priv,
> > > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > -
> > > > -}
> > > > -
> > > > -static void g4x_undo_pipe_scramble_reset(struct drm_i915_private
> > > > *dev_priv,
> > > > -					 enum pipe pipe)
> > > > -{
> > > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > -
> > > > -	if (pipe == PIPE_A)
> > > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > -	else
> > > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > -
> > > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > -			   I915_READ(PORT_DFT_I9XX) &
> > > > ~DC_BALANCE_RESET);
> > > > -	}
> > > >  }
> > > >  
> > > >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source
> > > > *source,
> > > > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > > > drm_i915_private *dev_priv,
> > > >  	switch (source) {
> > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > >  		return 0;
> > > >  	default:
> > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > > *crtc, const char *source_name)
> > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > >  
> > > >  	if (!source) {
> > > > -		if (IS_G4X(dev_priv))
> > > > -			g4x_undo_pipe_scramble_reset(dev_priv, crtc-
> > > > > index);
> > > > 
> > > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > > IS_CHERRYVIEW(dev_priv))
> > > > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > >  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > > > > index);
> > > > 
> > > >  		else if ((IS_HASWELL(dev_priv) ||
> > > >  			  IS_BROADWELL(dev_priv)) && crtc->index ==
> > > > PIPE_A)
> > 
> > 

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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Pandiyan, Dhinakaran]

On Fri, 2019-02-15 at 23:34 +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan
> > > wrote:
> > > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > DP CRCs don't really work on g4x. If you want any CRCs on DP
> > > > > you
> > > > > must
> > > > > select the CRC source before the port is enabled, otherwise
> > > > > the
> > > > > CRC
> > > > > source select bits simply ignore any writes to them. And once
> > > > > the
> > > > > port
> > > > > is enabled we mustn't change the CRC source select until the
> > > > > port
> > > > > is
> > > > > disabled. That almost works, but not quite :( Eventually the
> > > > > CRC
> > > > > source
> > > > > select bits get permanently stuck one way or the other, and
> > > > > after
> > > > > that
> > > > > a reboot (or possibly a display reset) is needed to get
> > > > > working
> > > > > CRCs
> > > > > on that pipe (not matter which CRC source we try to use).
> > > > > 
> > > > > Additionally the DFT scrambler reset bits we're trying to use
> > > > > don't
> > > > > seem to exist on g4x. There are some potentially relevant
> > > > > looking
> > > > > bits
> > > > > in the pipe registers, but when I tried it I got stable
> > > > > looking
> > > > > CRCs
> > > > > without setting any bits for this.
> > > > > 
> > > > > If there is a way to make DP CRCs work reliably on g4x, I
> > > > > wasn't
> > > > > able to find it. So let's just remove the broken code we
> > > > > have.
> > > > 
> > > > 
> > > > I think we can modify i9xx_pipe_crc_auto_source() to pick
> > > > "pipe"
> > > > CRC
> > > > when userspace selects "auto" and the output is DP/eDP.
> > > 
> > > Nope. Spec says:
> > > "Pipe CRC should not be run when Display Port or TV is enabled on
> > > this
> > >  pipe."
> > > 
> > > and
> > > 
> > > "CRC Source Select: These bits select the source of the data to
> > > put
> > > into
> > >  the CRC logic.
> > >  000: Pipe A (Not available when DisplayPort or TV is enabled on
> > > this
> > >  pipe)"
> > > 
> > 
> > After digging through some old specs, I do see this restriction for
> > gen-4 and VLV, but for some reason not for gen-3 or CHV. 
> 
> gen3 predates DP (g4x being the first platform that has it). I don't
> think SDVO->DP was ever a thing. SDVO->HDMI did happen but even that
> one is quite rare.

TV? I see TV initialization for a couple of gen-3 platforms but the
spec does not say that pipe CRCs are not available.
> 
> The display engine side of CHV is 99.9% VLV, with a few extra
> bits and pieces glued on top.

Thanks for the clarification, the CHV spec for some reason make it a
point to specify VLV in paranthesis

0000: Pipe C (Not available when DisplayPort or TV is enabled on this
pipe) [VLV]

> 
> > 
> > There is no good choice for "auto" other than DP and since DP does
> > not
> > work, returning -EINVAL makes sense.
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > > Though I must admit I've never actually tried it to see what
> > > actually
> > > happens.
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-------------
> > > > > ----
> > > > > ----
> > > > > --
> > > > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > index fe0ff89b980b..66bb7b031537 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  				 enum intel_pipe_crc_source
> > > > > *source,
> > > > >  				 u32 *val)
> > > > >  {
> > > > > -	bool need_stable_symbols = false;
> > > > > -
> > > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv,
> > > > > pipe,
> > > > > source);
> > > > >  		if (ret)
> > > > > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  			return -EINVAL;
> > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_TV_PRE;
> > > > >  		break;
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > -		if (!IS_G4X(dev_priv))
> > > > > -			return -EINVAL;
> > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_DP_B_G4X;
> > > > > -		need_stable_symbols = true;
> > > > > -		break;
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > -		if (!IS_G4X(dev_priv))
> > > > > -			return -EINVAL;
> > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_DP_C_G4X;
> > > > > -		need_stable_symbols = true;
> > > > > -		break;
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > -		if (!IS_G4X(dev_priv))
> > > > > -			return -EINVAL;
> > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_DP_D_G4X;
> > > > > -		need_stable_symbols = true;
> > > > > -		break;
> > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > >  		*val = 0;
> > > > >  		break;
> > > > >  	default:
> > > > > +		/*
> > > > > +		 * The DP CRC source doesn't work on g4x.
> > > > > +		 * It can be made to work to some degree by
> > > > > selecting
> > > > > +		 * the correct CRC source before the port is
> > > > > enabled,
> > > > > +		 * and not touching the CRC source bits again
> > > > > until
> > > > > +		 * the port is disabled. But even then the bits
> > > > > +		 * eventually get stuck and a reboot is needed
> > > > > to get
> > > > > +		 * working CRCs on the pipe again. Let's simply
> > > > > +		 * refuse to use DP CRCs on g4x.
> > > > > +		 */z
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	/*
> > > > > -	 * When the pipe CRC tap point is after the transcoders
> > > > > we need
> > > > > -	 * to tweak symbol-level features to produce a
> > > > > deterministic
> > > > > series of
> > > > > -	 * symbols for a given frame. We need to reset those
> > > > > features
> > > > > only once
> > > > > -	 * a frame (instead of every nth symbol):
> > > > > -	 *   - DC-balance: used to ensure a better clock
> > > > > recovery from
> > > > > the data
> > > > > -	 *     link (SDVO)
> > > > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > > > -	 */
> > > > > -	if (need_stable_symbols) {
> > > > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > -
> > > > > -		WARN_ON(!IS_G4X(dev_priv));
> > > > > -
> > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > -			   I915_READ(PORT_DFT_I9XX) |
> > > > > DC_BALANCE_RESET);
> > > > > -
> > > > > -		if (pipe == PIPE_A)
> > > > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > > -		else
> > > > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > > -
> > > > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > -	}
> > > > > -
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > @@ -282,24 +247,6 @@ static void
> > > > > vlv_undo_pipe_scramble_reset(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > > > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > -
> > > > > -}
> > > > > -
> > > > > -static void g4x_undo_pipe_scramble_reset(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > -					 enum pipe pipe)
> > > > > -{
> > > > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > -
> > > > > -	if (pipe == PIPE_A)
> > > > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > > -	else
> > > > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > -
> > > > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > -			   I915_READ(PORT_DFT_I9XX) &
> > > > > ~DC_BALANCE_RESET);
> > > > > -	}
> > > > >  }
> > > > >  
> > > > >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source
> > > > > *source,
> > > > > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	switch (source) {
> > > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > >  		return 0;
> > > > >  	default:
> > > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct
> > > > > drm_crtc
> > > > > *crtc, const char *source_name)
> > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > >  
> > > > >  	if (!source) {
> > > > > -		if (IS_G4X(dev_priv))
> > > > > -			g4x_undo_pipe_scramble_reset(dev_priv,
> > > > > crtc-
> > > > > > index);
> > > > > 
> > > > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > > > IS_CHERRYVIEW(dev_priv))
> > > > > +		if (IS_VALLEYVIEW(dev_priv) ||
> > > > > IS_CHERRYVIEW(dev_priv))
> > > > >  			vlv_undo_pipe_scramble_reset(dev_priv,
> > > > > crtc-
> > > > > > index);
> > > > > 
> > > > >  		else if ((IS_HASWELL(dev_priv) ||
> > > > >  			  IS_BROADWELL(dev_priv)) && crtc-
> > > > > >index ==
> > > > > PIPE_A)
> > > 
> > > 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Ville Syrjälä]

On Fri, Feb 15, 2019 at 09:43:37PM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2019-02-15 at 23:34 +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > > > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan
> > > > wrote:
> > > > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > DP CRCs don't really work on g4x. If you want any CRCs on DP
> > > > > > you
> > > > > > must
> > > > > > select the CRC source before the port is enabled, otherwise
> > > > > > the
> > > > > > CRC
> > > > > > source select bits simply ignore any writes to them. And once
> > > > > > the
> > > > > > port
> > > > > > is enabled we mustn't change the CRC source select until the
> > > > > > port
> > > > > > is
> > > > > > disabled. That almost works, but not quite :( Eventually the
> > > > > > CRC
> > > > > > source
> > > > > > select bits get permanently stuck one way or the other, and
> > > > > > after
> > > > > > that
> > > > > > a reboot (or possibly a display reset) is needed to get
> > > > > > working
> > > > > > CRCs
> > > > > > on that pipe (not matter which CRC source we try to use).
> > > > > > 
> > > > > > Additionally the DFT scrambler reset bits we're trying to use
> > > > > > don't
> > > > > > seem to exist on g4x. There are some potentially relevant
> > > > > > looking
> > > > > > bits
> > > > > > in the pipe registers, but when I tried it I got stable
> > > > > > looking
> > > > > > CRCs
> > > > > > without setting any bits for this.
> > > > > > 
> > > > > > If there is a way to make DP CRCs work reliably on g4x, I
> > > > > > wasn't
> > > > > > able to find it. So let's just remove the broken code we
> > > > > > have.
> > > > > 
> > > > > 
> > > > > I think we can modify i9xx_pipe_crc_auto_source() to pick
> > > > > "pipe"
> > > > > CRC
> > > > > when userspace selects "auto" and the output is DP/eDP.
> > > > 
> > > > Nope. Spec says:
> > > > "Pipe CRC should not be run when Display Port or TV is enabled on
> > > > this
> > > >  pipe."
> > > > 
> > > > and
> > > > 
> > > > "CRC Source Select: These bits select the source of the data to
> > > > put
> > > > into
> > > >  the CRC logic.
> > > >  000: Pipe A (Not available when DisplayPort or TV is enabled on
> > > > this
> > > >  pipe)"
> > > > 
> > > 
> > > After digging through some old specs, I do see this restriction for
> > > gen-4 and VLV, but for some reason not for gen-3 or CHV. 
> > 
> > gen3 predates DP (g4x being the first platform that has it). I don't
> > think SDVO->DP was ever a thing. SDVO->HDMI did happen but even that
> > one is quite rare.
> 
> TV? I see TV initialization for a couple of gen-3 platforms but the
> spec does not say that pipe CRCs are not available.

Could just be an omission in the spec. I don't think I actually
tested to see what happens when you try to use the pipe CRC with the
TV encoder. Presumably it might give you something useful, but it
certainly wouldn't account for anything done by the TV encoder.
Not that we normally care about that stuff anyway.

> > 
> > The display engine side of CHV is 99.9% VLV, with a few extra
> > bits and pieces glued on top.
> 
> Thanks for the clarification, the CHV spec for some reason make it a
> point to specify VLV in paranthesis

They basically just did 'cp VLVspec CHVspec', and then edited
it minimally. So you should generally interpret the "DevVLV*"
to mean "VLV/CHV".

> 
> 0000: Pipe C (Not available when DisplayPort or TV is enabled on this
> pipe) [VLV]
> 
> > 
> > > 
> > > There is no good choice for "auto" other than DP and since DP does
> > > not
> > > work, returning -EINVAL makes sense.
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > 
> > > > Though I must admit I've never actually tried it to see what
> > > > actually
> > > > happens.
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-------------
> > > > > > ----
> > > > > > ----
> > > > > > --
> > > > > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > index fe0ff89b980b..66bb7b031537 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  				 enum intel_pipe_crc_source
> > > > > > *source,
> > > > > >  				 u32 *val)
> > > > > >  {
> > > > > > -	bool need_stable_symbols = false;
> > > > > > -
> > > > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > > > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv,
> > > > > > pipe,
> > > > > > source);
> > > > > >  		if (ret)
> > > > > > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  			return -EINVAL;
> > > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_TV_PRE;
> > > > > >  		break;
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > -			return -EINVAL;
> > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_B_G4X;
> > > > > > -		need_stable_symbols = true;
> > > > > > -		break;
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > -			return -EINVAL;
> > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_C_G4X;
> > > > > > -		need_stable_symbols = true;
> > > > > > -		break;
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > -			return -EINVAL;
> > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_D_G4X;
> > > > > > -		need_stable_symbols = true;
> > > > > > -		break;
> > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > >  		*val = 0;
> > > > > >  		break;
> > > > > >  	default:
> > > > > > +		/*
> > > > > > +		 * The DP CRC source doesn't work on g4x.
> > > > > > +		 * It can be made to work to some degree by
> > > > > > selecting
> > > > > > +		 * the correct CRC source before the port is
> > > > > > enabled,
> > > > > > +		 * and not touching the CRC source bits again
> > > > > > until
> > > > > > +		 * the port is disabled. But even then the bits
> > > > > > +		 * eventually get stuck and a reboot is needed
> > > > > > to get
> > > > > > +		 * working CRCs on the pipe again. Let's simply
> > > > > > +		 * refuse to use DP CRCs on g4x.
> > > > > > +		 */z
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >  
> > > > > > -	/*
> > > > > > -	 * When the pipe CRC tap point is after the transcoders
> > > > > > we need
> > > > > > -	 * to tweak symbol-level features to produce a
> > > > > > deterministic
> > > > > > series of
> > > > > > -	 * symbols for a given frame. We need to reset those
> > > > > > features
> > > > > > only once
> > > > > > -	 * a frame (instead of every nth symbol):
> > > > > > -	 *   - DC-balance: used to ensure a better clock
> > > > > > recovery from
> > > > > > the data
> > > > > > -	 *     link (SDVO)
> > > > > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > > > > -	 */
> > > > > > -	if (need_stable_symbols) {
> > > > > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > -
> > > > > > -		WARN_ON(!IS_G4X(dev_priv));
> > > > > > -
> > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > -			   I915_READ(PORT_DFT_I9XX) |
> > > > > > DC_BALANCE_RESET);
> > > > > > -
> > > > > > -		if (pipe == PIPE_A)
> > > > > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > > > -		else
> > > > > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > > > -
> > > > > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > -	}
> > > > > > -
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > @@ -282,24 +247,6 @@ static void
> > > > > > vlv_undo_pipe_scramble_reset(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > > > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > > > > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > -
> > > > > > -}
> > > > > > -
> > > > > > -static void g4x_undo_pipe_scramble_reset(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv,
> > > > > > -					 enum pipe pipe)
> > > > > > -{
> > > > > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > -
> > > > > > -	if (pipe == PIPE_A)
> > > > > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > > > -	else
> > > > > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > -
> > > > > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > -			   I915_READ(PORT_DFT_I9XX) &
> > > > > > ~DC_BALANCE_RESET);
> > > > > > -	}
> > > > > >  }
> > > > > >  
> > > > > >  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source
> > > > > > *source,
> > > > > > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  	switch (source) {
> > > > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > >  		return 0;
> > > > > >  	default:
> > > > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct
> > > > > > drm_crtc
> > > > > > *crtc, const char *source_name)
> > > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > >  
> > > > > >  	if (!source) {
> > > > > > -		if (IS_G4X(dev_priv))
> > > > > > -			g4x_undo_pipe_scramble_reset(dev_priv,
> > > > > > crtc-
> > > > > > > index);
> > > > > > 
> > > > > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > +		if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > >  			vlv_undo_pipe_scramble_reset(dev_priv,
> > > > > > crtc-
> > > > > > > index);
> > > > > > 
> > > > > >  		else if ((IS_HASWELL(dev_priv) ||
> > > > > >  			  IS_BROADWELL(dev_priv)) && crtc-
> > > > > > >index ==
> > > > > > PIPE_A)
> > > > 
> > > > 
> > 
> > 

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

Re: ✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915: Remove the "pf" crc source

[Ville Syrjälä]

On Fri, Feb 15, 2019 at 04:16:46AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/4] drm/i915: Remove the "pf" crc source
> URL   : https://patchwork.freedesktop.org/series/56692/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_5602_full -> Patchwork_12224_full
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_12224_full absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_12224_full, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_12224_full:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
>     - shard-apl:          PASS -> FAIL +2

I still don't understand what this is telling me. Two things in fact
failed, but they were not the same subtest like this is indicating,
nor even the same machine.

igt@kms_atomic_transition@1x-modeset-transitions-nonblocking failed on shard-apl
igt@kms_atomic_transition@1x-modeset-transitions-fencing failed on shard-skl

Anyways...


CRC fail on skl:
(kms_atomic_transition:1179) igt_debugfs-DEBUG: CRC mismatch at index 0: 0x1f69e314 != 0x4fb7c284
(kms_atomic_transition:1179) igt_debugfs-CRITICAL: Test assertion failure function igt_assert_crc_equal, file ../lib/igt_debugfs.c:419:
(kms_atomic_transition:1179) igt_debugfs-CRITICAL: Failed assertion: !mismatch

Can't really be due to this series on account of
#define   PIPE_CRC_SOURCE_DMUX_SKL     (4 << 28)
==
#define   PIPE_CRC_SOURCE_PF_IVB        (2 << 29)

and probably all crc test should have failed in case I made
a mistake with those. Or else the tests aren't testing anything.


Some kind of event delivery fail on bxt:
(kms_atomic_transition:5786) DEBUG: Event mask: 4, waiting for 1 events
(kms_atomic_transition:5786) igt_core-INFO: Timed out: Timed out while
reading drm_fd

No idea about that one.

> 
>   * igt@pm_rpm@gem-evict-pwrite:
>     - shard-apl:          PASS -> DMESG-WARN

<3> [969.472585] [drm:intel_hdcp_auth [i915]] *ERROR* Timed out waiting for R0 ready

No clue about hdcp.

> 
>   
> #### Suppressed ####
> 
>   The following results come from untrusted machines, tests, or statuses.
>   They do not affect the overall result.
> 
>   * {igt@kms_psr2_su@page_flip}:
>     - shard-iclb:         NOTRUN -> {SKIP}
> 
>   * {igt@runner@aborted}:
>     - shard-apl:          NOTRUN -> FAIL
> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_12224_full that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_eio@in-flight-suspend:
>     - shard-snb:          PASS -> FAIL [fdo#103375]
> 
>   * igt@gem_exec_reloc@basic-cpu-gtt-noreloc:
>     - shard-apl:          PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] / [fdo#109373]
> 
>   * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
>     - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]
> 
>   * igt@kms_color@pipe-c-legacy-gamma:
>     - shard-glk:          PASS -> FAIL [fdo#104782]
> 
>   * igt@kms_cursor_crc@cursor-128x42-onscreen:
>     - shard-apl:          PASS -> FAIL [fdo#103232] +2
> 
>   * igt@kms_cursor_crc@cursor-128x42-sliding:
>     - shard-iclb:         NOTRUN -> FAIL [fdo#103232] +1
> 
>   * igt@kms_cursor_crc@cursor-64x64-suspend:
>     - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]
> 
>   * igt@kms_fbcon_fbt@psr:
>     - shard-iclb:         NOTRUN -> FAIL [fdo#103833]
> 
>   * igt@kms_flip@modeset-vs-vblank-race-interruptible:
>     - shard-glk:          PASS -> FAIL [fdo#103060]
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
>     - shard-apl:          PASS -> FAIL [fdo#103167] +2
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
>     - shard-glk:          PASS -> FAIL [fdo#103167] +1
> 
>   * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
>     - shard-iclb:         PASS -> FAIL [fdo#103167] +2
> 
>   * igt@kms_plane@pixel-format-pipe-a-planes:
>     - shard-iclb:         NOTRUN -> FAIL [fdo#103166]
> 
>   * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
>     - shard-iclb:         PASS -> FAIL [fdo#103166]
> 
>   * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
>     - shard-glk:          PASS -> FAIL [fdo#103166]
> 
>   * igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
>     - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +1
> 
>   * igt@kms_psr@no_drrs:
>     - shard-iclb:         PASS -> FAIL [fdo#108341]
> 
>   * igt@kms_rotation_crc@multiplane-rotation:
>     - shard-kbl:          PASS -> FAIL [fdo#109016]
> 
>   * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
>     - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]
> 
>   * igt@kms_universal_plane@universal-plane-pipe-a-functional:
>     - shard-apl:          NOTRUN -> FAIL [fdo#103166]
> 
>   * igt@kms_universal_plane@universal-plane-pipe-b-functional:
>     - shard-apl:          PASS -> FAIL [fdo#103166] +2
> 
>   * igt@kms_vblank@pipe-c-ts-continuation-suspend:
>     - shard-apl:          PASS -> FAIL [fdo#104894]
> 
>   * igt@pm_rpm@cursor:
>     - shard-iclb:         PASS -> DMESG-WARN [fdo#107724]
> 
>   
> #### Possible fixes ####
> 
>   * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
>     - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS
> 
>   * igt@kms_chv_cursor_fail@pipe-b-64x64-bottom-edge:
>     - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +18
> 
>   * igt@kms_color@pipe-b-degamma:
>     - shard-apl:          FAIL [fdo#104782] -> PASS
> 
>   * igt@kms_color@pipe-b-legacy-gamma:
>     - shard-glk:          FAIL [fdo#104782] -> PASS
> 
>   * igt@kms_color@pipe-c-ctm-max:
>     - shard-apl:          FAIL [fdo#108147] -> PASS
> 
>   * igt@kms_cursor_crc@cursor-64x21-random:
>     - shard-apl:          FAIL [fdo#103232] -> PASS +2
> 
>   * igt@kms_cursor_crc@cursor-alpha-opaque:
>     - shard-apl:          FAIL [fdo#109350] -> PASS
> 
>   * igt@kms_cursor_crc@cursor-size-change:
>     - shard-glk:          FAIL [fdo#103232] -> PASS
> 
>   * igt@kms_flip@flip-vs-expired-vblank-interruptible:
>     - shard-glk:          FAIL [fdo#102887] / [fdo#105363] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
>     - shard-apl:          FAIL [fdo#103167] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-rte:
>     - shard-glk:          FAIL [fdo#103167] / [fdo#105682] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-wc:
>     - shard-glk:          FAIL [fdo#103167] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
>     - shard-iclb:         FAIL [fdo#103167] -> PASS +2
> 
>   * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
>     - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602] -> PASS +1
> 
>   * igt@kms_plane@plane-position-covered-pipe-a-planes:
>     - shard-apl:          FAIL [fdo#103166] -> PASS +1
> 
>   * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
>     - shard-iclb:         FAIL [fdo#103166] -> PASS +1
> 
>   * igt@pm_rpm@basic-rte:
>     - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +3
> 
>   * igt@pm_rpm@legacy-planes:
>     - shard-iclb:         INCOMPLETE [fdo#108840] / [fdo#109369] -> PASS
> 
>   
> #### Warnings ####
> 
>   * igt@i915_selftest@live_contexts:
>     - shard-iclb:         DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569] / [fdo#109226]
> 
>   * igt@i915_suspend@shrink:
>     - shard-snb:          INCOMPLETE [fdo#105411] / [fdo#106886] -> DMESG-WARN [fdo#109244]
> 
>   * igt@kms_cursor_crc@cursor-128x128-onscreen:
>     - shard-kbl:          DMESG-FAIL [fdo#103232] / [fdo#103558] / [fdo#105602] -> FAIL [fdo#103232] +1
> 
>   * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
>     - shard-kbl:          DMESG-FAIL [fdo#103558] / [fdo#105602] / [fdo#108145] -> FAIL [fdo#108145] / [fdo#108590]
> 
>   
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
>   [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
>   [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
>   [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
>   [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
>   [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
>   [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
>   [fdo#103833]: https://bugs.freedesktop.org/show_bug.cgi?id=103833
>   [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
>   [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
>   [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
>   [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
>   [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
>   [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
>   [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
>   [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
>   [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
>   [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
>   [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
>   [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
>   [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
>   [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
>   [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
>   [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
>   [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
>   [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
>   [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
>   [fdo#109226]: https://bugs.freedesktop.org/show_bug.cgi?id=109226
>   [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
>   [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
>   [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
>   [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
>   [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
>   [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
>   [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
>   [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
>   [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
>   [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
>   [fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
>   [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
>   [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
>   [fdo#109293]: https://bugs.freedesktop.org/show_bug.cgi?id=109293
>   [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
>   [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
>   [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
>   [fdo#109369]: https://bugs.freedesktop.org/show_bug.cgi?id=109369
>   [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
>   [fdo#109625]: https://bugs.freedesktop.org/show_bug.cgi?id=109625
> 
> 
> Participating hosts (7 -> 6)
> ------------------------------
> 
>   Missing    (1): shard-skl 
> 
> 
> Build changes
> -------------
> 
>     * Linux: CI_DRM_5602 -> Patchwork_12224
> 
>   CI_DRM_5602: 570d4d9a80d29c77155979e5fdfb2e1ba76057c7 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_12224: 0434615d44ce8f4dc908e836f6981eceb17903dc @ 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_12224/
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x

[Dhinakaran Pandiyan]

On Mon, 2019-02-18 at 19:57 +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2019 at 09:43:37PM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2019-02-15 at 23:34 +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan
> > > wrote:
> > > > On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > > > > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan
> > > > > wrote:
> > > > > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > 
> > > > > > > DP CRCs don't really work on g4x. If you want any CRCs on
> > > > > > > DP
> > > > > > > you
> > > > > > > must
> > > > > > > select the CRC source before the port is enabled,
> > > > > > > otherwise
> > > > > > > the
> > > > > > > CRC
> > > > > > > source select bits simply ignore any writes to them. And
> > > > > > > once
> > > > > > > the
> > > > > > > port
> > > > > > > is enabled we mustn't change the CRC source select until
> > > > > > > the
> > > > > > > port
> > > > > > > is
> > > > > > > disabled. That almost works, but not quite :( Eventually
> > > > > > > the
> > > > > > > CRC
> > > > > > > source
> > > > > > > select bits get permanently stuck one way or the other,
> > > > > > > and
> > > > > > > after
> > > > > > > that
> > > > > > > a reboot (or possibly a display reset) is needed to get
> > > > > > > working
> > > > > > > CRCs
> > > > > > > on that pipe (not matter which CRC source we try to use).
> > > > > > > 
> > > > > > > Additionally the DFT scrambler reset bits we're trying to
> > > > > > > use
> > > > > > > don't
> > > > > > > seem to exist on g4x. There are some potentially relevant
> > > > > > > looking
> > > > > > > bits
> > > > > > > in the pipe registers, but when I tried it I got stable
> > > > > > > looking
> > > > > > > CRCs
> > > > > > > without setting any bits for this.
> > > > > > > 
> > > > > > > If there is a way to make DP CRCs work reliably on g4x, I
> > > > > > > wasn't
> > > > > > > able to find it. So let's just remove the broken code we
> > > > > > > have.
> > > > > > 
> > > > > > 
> > > > > > I think we can modify i9xx_pipe_crc_auto_source() to pick
> > > > > > "pipe"
> > > > > > CRC
> > > > > > when userspace selects "auto" and the output is DP/eDP.
> > > > > 
> > > > > Nope. Spec says:
> > > > > "Pipe CRC should not be run when Display Port or TV is
> > > > > enabled on
> > > > > this
> > > > >  pipe."
> > > > > 
> > > > > and
> > > > > 
> > > > > "CRC Source Select: These bits select the source of the data
> > > > > to
> > > > > put
> > > > > into
> > > > >  the CRC logic.
> > > > >  000: Pipe A (Not available when DisplayPort or TV is enabled
> > > > > on
> > > > > this
> > > > >  pipe)"
> > > > > 
> > > > 
> > > > After digging through some old specs, I do see this restriction
> > > > for
> > > > gen-4 and VLV, but for some reason not for gen-3 or CHV. 
> > > 
> > > gen3 predates DP (g4x being the first platform that has it). I
> > > don't
> > > think SDVO->DP was ever a thing. SDVO->HDMI did happen but even
> > > that
> > > one is quite rare.
> > 
> > TV? I see TV initialization for a couple of gen-3 platforms but the
> > spec does not say that pipe CRCs are not available.
> 
> Could just be an omission in the spec. I don't think I actually
> tested to see what happens when you try to use the pipe CRC with the
> TV encoder. Presumably it might give you something useful, but it
> certainly wouldn't account for anything done by the TV encoder.
> Not that we normally care about that stuff anyway.

PSR2 might be one of those cases that makes encoder CRCs interesting -
for e.g., compare DDI CRC of a PSR2 SU update region against a
reference. I don't know how we can generate a reference CRC for a SU
update region though.

> 
> > > 
> > > The display engine side of CHV is 99.9% VLV, with a few extra
> > > bits and pieces glued on top.
> > 
> > Thanks for the clarification, the CHV spec for some reason make it
> > a
> > point to specify VLV in paranthesis
> 
> They basically just did 'cp VLVspec CHVspec', and then edited
> it minimally. So you should generally interpret the "DevVLV*"
> to mean "VLV/CHV".

Got it, thanks for explaining :)

-DK

> 
> > 
> > 0000: Pipe C (Not available when DisplayPort or TV is enabled on
> > this
> > pipe) [VLV]
> > 
> > > 
> > > > 
> > > > There is no good choice for "auto" other than DP and since DP
> > > > does
> > > > not
> > > > work, returning -EINVAL makes sense.
> > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > 
> > > > > Though I must admit I've never actually tried it to see what
> > > > > actually
> > > > > happens.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Ville Syrjälä <
> > > > > > > ville.syrjala@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++---------
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > > --
> > > > > > >  1 file changed, 11 insertions(+), 69 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > index fe0ff89b980b..66bb7b031537 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > > @@ -191,8 +191,6 @@ static int
> > > > > > > i9xx_pipe_crc_ctl_reg(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  				 enum intel_pipe_crc_source
> > > > > > > *source,
> > > > > > >  				 u32 *val)
> > > > > > >  {
> > > > > > > -	bool need_stable_symbols = false;
> > > > > > > -
> > > > > > >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > > > > >  		int ret = i9xx_pipe_crc_auto_source(dev_priv,
> > > > > > > pipe,
> > > > > > > source);
> > > > > > >  		if (ret)
> > > > > > > @@ -208,56 +206,23 @@ static int
> > > > > > > i9xx_pipe_crc_ctl_reg(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  			return -EINVAL;
> > > > > > >  		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_TV_PRE;
> > > > > > >  		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_B_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_C_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > > -		if (!IS_G4X(dev_priv))
> > > > > > > -			return -EINVAL;
> > > > > > > -		*val = PIPE_CRC_ENABLE |
> > > > > > > PIPE_CRC_SOURCE_DP_D_G4X;
> > > > > > > -		need_stable_symbols = true;
> > > > > > > -		break;
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > >  		*val = 0;
> > > > > > >  		break;
> > > > > > >  	default:
> > > > > > > +		/*
> > > > > > > +		 * The DP CRC source doesn't work on g4x.
> > > > > > > +		 * It can be made to work to some degree by
> > > > > > > selecting
> > > > > > > +		 * the correct CRC source before the port is
> > > > > > > enabled,
> > > > > > > +		 * and not touching the CRC source bits again
> > > > > > > until
> > > > > > > +		 * the port is disabled. But even then the bits
> > > > > > > +		 * eventually get stuck and a reboot is needed
> > > > > > > to get
> > > > > > > +		 * working CRCs on the pipe again. Let's simply
> > > > > > > +		 * refuse to use DP CRCs on g4x.
> > > > > > > +		 */z
> > > > > > >  		return -EINVAL;
> > > > > > >  	}
> > > > > > >  
> > > > > > > -	/*
> > > > > > > -	 * When the pipe CRC tap point is after the transcoders
> > > > > > > we need
> > > > > > > -	 * to tweak symbol-level features to produce a
> > > > > > > deterministic
> > > > > > > series of
> > > > > > > -	 * symbols for a given frame. We need to reset those
> > > > > > > features
> > > > > > > only once
> > > > > > > -	 * a frame (instead of every nth symbol):
> > > > > > > -	 *   - DC-balance: used to ensure a better clock
> > > > > > > recovery from
> > > > > > > the data
> > > > > > > -	 *     link (SDVO)
> > > > > > > -	 *   - DisplayPort scrambling: used for EMI reduction
> > > > > > > -	 */
> > > > > > > -	if (need_stable_symbols) {
> > > > > > > -		u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > > -
> > > > > > > -		WARN_ON(!IS_G4X(dev_priv));
> > > > > > > -
> > > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > > -			   I915_READ(PORT_DFT_I9XX) |
> > > > > > > DC_BALANCE_RESET);
> > > > > > > -
> > > > > > > -		if (pipe == PIPE_A)
> > > > > > > -			tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > > > > -		else
> > > > > > > -			tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > > > > -
> > > > > > > -		I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -	}
> > > > > > > -
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -282,24 +247,6 @@ static void
> > > > > > > vlv_undo_pipe_scramble_reset(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > > > > >  		tmp &= ~DC_BALANCE_RESET_VLV;
> > > > > > >  	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -
> > > > > > > -}
> > > > > > > -
> > > > > > > -static void g4x_undo_pipe_scramble_reset(struct
> > > > > > > drm_i915_private
> > > > > > > *dev_priv,
> > > > > > > -					 enum pipe pipe)
> > > > > > > -{
> > > > > > > -	u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > > -
> > > > > > > -	if (pipe == PIPE_A)
> > > > > > > -		tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > > > > -	else
> > > > > > > -		tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > > > > -	I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > > -
> > > > > > > -	if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > > > > -		I915_WRITE(PORT_DFT_I9XX,
> > > > > > > -			   I915_READ(PORT_DFT_I9XX) &
> > > > > > > ~DC_BALANCE_RESET);
> > > > > > > -	}
> > > > > > >  }
> > > > > > >  
> > > > > > >  static int ilk_pipe_crc_ctl_reg(enum
> > > > > > > intel_pipe_crc_source
> > > > > > > *source,
> > > > > > > @@ -485,9 +432,6 @@ static int
> > > > > > > i9xx_crc_source_valid(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >  	switch (source) {
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_TV:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > > -	case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > >  		return 0;
> > > > > > >  	default:
> > > > > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct
> > > > > > > drm_crtc
> > > > > > > *crtc, const char *source_name)
> > > > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > > >  
> > > > > > >  	if (!source) {
> > > > > > > -		if (IS_G4X(dev_priv))
> > > > > > > -			g4x_undo_pipe_scramble_reset(dev_priv,
> > > > > > > crtc-
> > > > > > > > index);
> > > > > > > 
> > > > > > > -		else if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > > +		if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > >  			vlv_undo_pipe_scramble_reset(dev_priv,
> > > > > > > crtc-
> > > > > > > > index);
> > > > > > > 
> > > > > > >  		else if ((IS_HASWELL(dev_priv) ||
> > > > > > >  			  IS_BROADWELL(dev_priv)) && crtc-
> > > > > > > > index ==
> > > > > > > 
> > > > > > > PIPE_A)
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 

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

Re: [PATCH 4/4] drm/i915: Extend skl+ crc sources with more planes

[Ville Syrjälä]

On Thu, Feb 14, 2019 at 06:07:05PM -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On skl the crc registers were extended to provide plane crcs
> > for up to 7 planes. Add the new crc sources.
> > 
> > The current code uses the ivb+ register definitions for skl+
> > which does happen to work as the plane1, plane2, and dmux/pf
> > bits happen the match what ivb+ had. So no bug in the current
> > code.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  5 ++
> >  drivers/gpu/drm/i915/i915_reg.h       |  9 ++++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 76
> > ++++++++++++++++++++++++++-
> >  3 files changed, 88 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 4e11d970cbcf..8607c1e9ed02 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1196,6 +1196,11 @@ enum intel_pipe_crc_source {
> >  	INTEL_PIPE_CRC_SOURCE_NONE,
> >  	INTEL_PIPE_CRC_SOURCE_PLANE1,
> >  	INTEL_PIPE_CRC_SOURCE_PLANE2,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE3,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE4,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE5,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE6,
> > +	INTEL_PIPE_CRC_SOURCE_PLANE7,
> >  	INTEL_PIPE_CRC_SOURCE_PIPE,
> >  	/* TV/DP on pre-gen5/vlv can't use the pipe source. */
> >  	INTEL_PIPE_CRC_SOURCE_TV,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 0df8c6e76da7..5286536e9cb8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4017,6 +4017,15 @@ enum {
> >  /* Pipe A CRC regs */
> >  #define _PIPE_CRC_CTL_A			0x60050
> >  #define   PIPE_CRC_ENABLE		(1 << 31)
> > +/* skl+ source selection */
> > +#define   PIPE_CRC_SOURCE_PLANE_1_SKL	(0 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_2_SKL	(2 << 28)
> > +#define   PIPE_CRC_SOURCE_DMUX_SKL	(4 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_3_SKL	(6 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_4_SKL	(7 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_5_SKL	(5 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_6_SKL	(3 << 28)
> > +#define   PIPE_CRC_SOURCE_PLANE_7_SKL	(1 << 28)
> >  /* ivb+ source selection */
> >  #define   PIPE_CRC_SOURCE_PRIMARY_IVB	(0 << 29)
> >  #define   PIPE_CRC_SOURCE_SPRITE_IVB	(1 << 29)
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 66bb7b031537..e521f82ba5d9 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -34,6 +34,11 @@ static const char * const pipe_crc_sources[] = {
> >  	[INTEL_PIPE_CRC_SOURCE_NONE] = "none",
> >  	[INTEL_PIPE_CRC_SOURCE_PLANE1] = "plane1",
> >  	[INTEL_PIPE_CRC_SOURCE_PLANE2] = "plane2",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE3] = "plane3",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE4] = "plane4",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE5] = "plane5",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE6] = "plane6",
> > +	[INTEL_PIPE_CRC_SOURCE_PLANE7] = "plane7",
> >  	[INTEL_PIPE_CRC_SOURCE_PIPE] = "pipe",
> >  	[INTEL_PIPE_CRC_SOURCE_TV] = "TV",
> >  	[INTEL_PIPE_CRC_SOURCE_DP_B] = "DP-B",
> > @@ -368,6 +373,50 @@ static int ivb_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  	return 0;
> >  }
> >  
> > +static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > +				enum pipe pipe,
> > +				enum intel_pipe_crc_source *source,
> > +				uint32_t *val,
> > +				bool set_wa)
> 
> set_wa is unused. 

Dropped. And pushed.

Thanks for the reviews.

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