[PATCH] drm/i916: add "auto" pipe CRC source

[PATCH] drm/i916: add "auto" pipe CRC source

[Daniel Vetter]

On gmch platforms the normal pipe source CRC registers don't work for
DP and TV encoders. And on newer platforms the single pipe CRC has
been replaced by a set of CRC at different stages in the platform.

Now most of our userspace tests don't care one bit about the exact
CRC, they simply want something that reflects any changes on the
screen. Hence add a new auto target for platform agnostic tests to
use.

v2: Pass back the adjusted source so that it can be shown in debugfs.

v3: I seem to be unable to get a stable CRC for DP ports. So let's
just disable them for now when using the auto mode. Note that
testcases need to be restructured so that they can dynamically skip
connectors. They also first need to set up the desired mode
configuration, since otherwise the auto mode won't do the right thing.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 90 ++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 2 files changed, 76 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5c45e9e..b4fbbd4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1936,6 +1936,7 @@ static const char * const pipe_crc_sources[] = {
 	"DP-B",
 	"DP-C",
 	"DP-D",
+	"auto",
 };
 
 static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
@@ -1964,10 +1965,13 @@ static int display_crc_ctl_open(struct inode *inode, struct file *file)
 	return single_open(file, display_crc_ctl_show, dev);
 }
 
-static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_INCLUDE_BORDER_I8XX;
 		break;
@@ -1981,10 +1985,53 @@ static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i9xx_pipe_crc_auto_sourc(struct drm_device *dev, enum pipe pipe,
+				    enum intel_pipe_crc_source *source)
+{
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc;
+
+	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	mutex_lock(&dev->mode_config.mutex);
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (!encoder->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(encoder->base.crtc);
+
+		if (crtc->pipe != pipe)
+			continue;
+
+		switch (encoder->type) {
+		case INTEL_OUTPUT_TVOUT:
+			*source = INTEL_PIPE_CRC_SOURCE_TV;
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+		case INTEL_OUTPUT_EDP:
+			/* We can't get stable CRCs for DP ports somehow. */
+			return -ENODEV;
+			break;
+		}
+	}
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return 0;
+}
+
+static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
+				enum pipe pipe,
+				enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_sourc(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_VLV;
 		break;
@@ -2005,10 +2052,17 @@ static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 }
 
 static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
-				 enum intel_pipe_crc_source source,
+				 enum pipe pipe,
+				 enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_sourc(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_I9XX;
 		break;
@@ -2042,10 +2096,13 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 	return 0;
 }
 
-static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	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_PRIMARY_ILK;
 		break;
@@ -2065,10 +2122,13 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PF;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
 		break;
@@ -2104,15 +2164,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		return -EINVAL;
 
 	if (IS_GEN2(dev))
-		ret = i8xx_pipe_crc_ctl_reg(source, &val);
+		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
 	else if (INTEL_INFO(dev)->gen < 5)
-		ret = i9xx_pipe_crc_ctl_reg(dev, source, &val);
+		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
 	else if (IS_VALLEYVIEW(dev))
-		ret = vlv_pipe_crc_ctl_reg(source, &val);
+		ret = vlv_pipe_crc_ctl_reg(dev,pipe, &source, &val);
 	else if (IS_GEN5(dev) || IS_GEN6(dev))
-		ret = ilk_pipe_crc_ctl_reg(source, &val);
+		ret = ilk_pipe_crc_ctl_reg(&source, &val);
 	else
-		ret = ivb_pipe_crc_ctl_reg(source, &val);
+		ret = ivb_pipe_crc_ctl_reg(&source, &val);
 
 	if (ret != 0)
 		return ret;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..b12d942 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1253,6 +1253,7 @@ enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_DP_B,
 	INTEL_PIPE_CRC_SOURCE_DP_C,
 	INTEL_PIPE_CRC_SOURCE_DP_D,
+	INTEL_PIPE_CRC_SOURCE_AUTO,
 	INTEL_PIPE_CRC_SOURCE_MAX,
 };
 
-- 
1.8.4.rc3

[PATCH] drm/i916: add "auto" pipe CRC source

[Daniel Vetter]

On gmch platforms the normal pipe source CRC registers don't work for
DP and TV encoders. And on newer platforms the single pipe CRC has
been replaced by a set of CRC at different stages in the platform.

Now most of our userspace tests don't care one bit about the exact
CRC, they simply want something that reflects any changes on the
screen. Hence add a new auto target for platform agnostic tests to
use.

v2: Pass back the adjusted source so that it can be shown in debugfs.

v3: I seem to be unable to get a stable CRC for DP ports. So let's
just disable them for now when using the auto mode. Note that
testcases need to be restructured so that they can dynamically skip
connectors. They also first need to set up the desired mode
configuration, since otherwise the auto mode won't do the right thing.

v4: Don't leak the modeset mutex on error paths.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 91 +++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 2 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5c45e9e..9e71cf0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1936,6 +1936,7 @@ static const char * const pipe_crc_sources[] = {
 	"DP-B",
 	"DP-C",
 	"DP-D",
+	"auto",
 };
 
 static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
@@ -1964,10 +1965,13 @@ static int display_crc_ctl_open(struct inode *inode, struct file *file)
 	return single_open(file, display_crc_ctl_show, dev);
 }
 
-static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_INCLUDE_BORDER_I8XX;
 		break;
@@ -1981,10 +1985,54 @@ static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i9xx_pipe_crc_auto_sourc(struct drm_device *dev, enum pipe pipe,
+				    enum intel_pipe_crc_source *source)
+{
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc;
+	int ret = 0;
+
+	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	mutex_lock(&dev->mode_config.mutex);
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (!encoder->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(encoder->base.crtc);
+
+		if (crtc->pipe != pipe)
+			continue;
+
+		switch (encoder->type) {
+		case INTEL_OUTPUT_TVOUT:
+			*source = INTEL_PIPE_CRC_SOURCE_TV;
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+		case INTEL_OUTPUT_EDP:
+			/* We can't get stable CRCs for DP ports somehow. */
+			ret = -ENODEV;
+			break;
+		}
+	}
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
+}
+
+static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
+				enum pipe pipe,
+				enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_sourc(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_VLV;
 		break;
@@ -2005,10 +2053,17 @@ static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 }
 
 static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
-				 enum intel_pipe_crc_source source,
+				 enum pipe pipe,
+				 enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_sourc(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_I9XX;
 		break;
@@ -2042,10 +2097,13 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 	return 0;
 }
 
-static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	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_PRIMARY_ILK;
 		break;
@@ -2065,10 +2123,13 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PF;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
 		break;
@@ -2104,15 +2165,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		return -EINVAL;
 
 	if (IS_GEN2(dev))
-		ret = i8xx_pipe_crc_ctl_reg(source, &val);
+		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
 	else if (INTEL_INFO(dev)->gen < 5)
-		ret = i9xx_pipe_crc_ctl_reg(dev, source, &val);
+		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
 	else if (IS_VALLEYVIEW(dev))
-		ret = vlv_pipe_crc_ctl_reg(source, &val);
+		ret = vlv_pipe_crc_ctl_reg(dev,pipe, &source, &val);
 	else if (IS_GEN5(dev) || IS_GEN6(dev))
-		ret = ilk_pipe_crc_ctl_reg(source, &val);
+		ret = ilk_pipe_crc_ctl_reg(&source, &val);
 	else
-		ret = ivb_pipe_crc_ctl_reg(source, &val);
+		ret = ivb_pipe_crc_ctl_reg(&source, &val);
 
 	if (ret != 0)
 		return ret;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..b12d942 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1253,6 +1253,7 @@ enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_DP_B,
 	INTEL_PIPE_CRC_SOURCE_DP_C,
 	INTEL_PIPE_CRC_SOURCE_DP_D,
+	INTEL_PIPE_CRC_SOURCE_AUTO,
 	INTEL_PIPE_CRC_SOURCE_MAX,
 };
 
-- 
1.8.4.rc3

[PATCH 1/4] drm/i916: add "auto" pipe CRC source

[Daniel Vetter]

On gmch platforms the normal pipe source CRC registers don't work for
DP and TV encoders. And on newer platforms the single pipe CRC has
been replaced by a set of CRC at different stages in the platform.

Now most of our userspace tests don't care one bit about the exact
CRC, they simply want something that reflects any changes on the
screen. Hence add a new auto target for platform agnostic tests to
use.

v2: Pass back the adjusted source so that it can be shown in debugfs.

v3: I seem to be unable to get a stable CRC for DP ports. So let's
just disable them for now when using the auto mode. Note that
testcases need to be restructured so that they can dynamically skip
connectors. They also first need to set up the desired mode
configuration, since otherwise the auto mode won't do the right thing.

v4: Don't leak the modeset mutex on error paths.

v5: Spelling fix for the i9xx auto_source function.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 91 +++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 2 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5c45e9e..7c29a88 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1936,6 +1936,7 @@ static const char * const pipe_crc_sources[] = {
 	"DP-B",
 	"DP-C",
 	"DP-D",
+	"auto",
 };
 
 static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
@@ -1964,10 +1965,13 @@ static int display_crc_ctl_open(struct inode *inode, struct file *file)
 	return single_open(file, display_crc_ctl_show, dev);
 }
 
-static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_INCLUDE_BORDER_I8XX;
 		break;
@@ -1981,10 +1985,54 @@ static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
+				     enum intel_pipe_crc_source *source)
+{
+	struct intel_encoder *encoder;
+	struct intel_crtc *crtc;
+	int ret = 0;
+
+	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+	mutex_lock(&dev->mode_config.mutex);
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (!encoder->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(encoder->base.crtc);
+
+		if (crtc->pipe != pipe)
+			continue;
+
+		switch (encoder->type) {
+		case INTEL_OUTPUT_TVOUT:
+			*source = INTEL_PIPE_CRC_SOURCE_TV;
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+		case INTEL_OUTPUT_EDP:
+			/* We can't get stable CRCs for DP ports somehow. */
+			ret = -ENODEV;
+			break;
+		}
+	}
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
+}
+
+static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
+				enum pipe pipe,
+				enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_VLV;
 		break;
@@ -2005,10 +2053,17 @@ static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 }
 
 static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
-				 enum intel_pipe_crc_source source,
+				 enum pipe pipe,
+				 enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
+		if (ret)
+			return ret;
+	}
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_I9XX;
 		break;
@@ -2042,10 +2097,13 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 	return 0;
 }
 
-static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	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_PRIMARY_ILK;
 		break;
@@ -2065,10 +2123,13 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
 	return 0;
 }
 
-static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
+static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
-	switch (source) {
+	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
+		*source = INTEL_PIPE_CRC_SOURCE_PF;
+
+	switch (*source) {
 	case INTEL_PIPE_CRC_SOURCE_PLANE1:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
 		break;
@@ -2104,15 +2165,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		return -EINVAL;
 
 	if (IS_GEN2(dev))
-		ret = i8xx_pipe_crc_ctl_reg(source, &val);
+		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
 	else if (INTEL_INFO(dev)->gen < 5)
-		ret = i9xx_pipe_crc_ctl_reg(dev, source, &val);
+		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
 	else if (IS_VALLEYVIEW(dev))
-		ret = vlv_pipe_crc_ctl_reg(source, &val);
+		ret = vlv_pipe_crc_ctl_reg(dev,pipe, &source, &val);
 	else if (IS_GEN5(dev) || IS_GEN6(dev))
-		ret = ilk_pipe_crc_ctl_reg(source, &val);
+		ret = ilk_pipe_crc_ctl_reg(&source, &val);
 	else
-		ret = ivb_pipe_crc_ctl_reg(source, &val);
+		ret = ivb_pipe_crc_ctl_reg(&source, &val);
 
 	if (ret != 0)
 		return ret;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..b12d942 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1253,6 +1253,7 @@ enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_DP_B,
 	INTEL_PIPE_CRC_SOURCE_DP_C,
 	INTEL_PIPE_CRC_SOURCE_DP_D,
+	INTEL_PIPE_CRC_SOURCE_AUTO,
 	INTEL_PIPE_CRC_SOURCE_MAX,
 };
 
-- 
1.8.4.rc3

[PATCH 2/4] drm/i915: scramble reset support for DP port CRC on g4x

[Daniel Vetter]

We need to reset the DP scrambler on every vsync to get stable CRCs.
And since we can't use the normal pipe CRC on DP ports on g4x we
really need them to be able to test modesetting issues on (e)DP
outputs.

Note that the DC balance reset is for SDVO port CRCs so we don't
strictly need it. But better safe than sorry (and it's a nice template
in case we ever want to grab port CRCs for e.g. audio checking).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 44 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |  8 +++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7c29a88..194ca5f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2057,6 +2057,9 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 				 enum intel_pipe_crc_source *source,
 				 uint32_t *val)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool need_scramble_reset = false;
+
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
 		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
 		if (ret)
@@ -2076,16 +2079,19 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 		if (!IS_G4X(dev))
 			return -EINVAL;
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_G4X;
+		need_scramble_reset = true;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_DP_C:
 		if (!IS_G4X(dev))
 			return -EINVAL;
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_G4X;
+		need_scramble_reset = true;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_DP_D:
 		if (!IS_G4X(dev))
 			return -EINVAL;
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_G4X;
+		need_scramble_reset = true;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		*val = 0;
@@ -2094,9 +2100,44 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	/* DP port CRC needs slight spec mis-compliance to get stable CRCs. */
+	if (need_scramble_reset) {
+		uint32_t tmp = I915_READ(PORT_DFT2_G4X);
+
+		WARN_ON(!IS_G4X(dev));
+
+		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;
 }
 
+static void g4x_undo_pipe_scramble_reset(struct drm_device *dev,
+					 enum pipe pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t 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,
 				uint32_t *val)
 {
@@ -2215,6 +2256,9 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		spin_unlock_irq(&pipe_crc->lock);
 
 		kfree(entries);
+
+		if (IS_G4X(dev))
+			g4x_undo_pipe_scramble_reset(dev, pipe);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 447fd83..01ad002 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2158,6 +2158,14 @@
 #define PCH_HDMIC	0xe1150
 #define PCH_HDMID	0xe1160
 
+#define PORT_DFT_I9XX				0x61150
+#define   DC_BALANCE_RESET			(1 << 25)
+#define PORT_DFT2_G4X				0x61154
+#define   DC_BALANCE_RESET_VLV			(1 << 31)
+#define   PIPE_SCRAMBLE_RESET_MASK		(0x3 << 0)
+#define   PIPE_B_SCRAMBLE_RESET			(1 << 1)
+#define   PIPE_A_SCRAMBLE_RESET			(1 << 0)
+
 /* Gen 3 SDVO bits: */
 #define   SDVO_ENABLE				(1 << 31)
 #define   SDVO_PIPE_SEL(pipe)			((pipe) << 30)
-- 
1.8.4.rc3

[PATCH 3/4] drm/i915: scramble reset support for DP port CRC on vlv

[Daniel Vetter]

They've moved the DC balance reset bit around. Again I don't think we
need it, but better safe than sorry and maybe HDMI port CRC will prove
useful for checking infoframes or hdmi audio.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 38 +++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 194ca5f..5942362 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2026,6 +2026,9 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
 				enum intel_pipe_crc_source *source,
 				uint32_t *val)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool need_scramble_reset = false;
+
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
 		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
 		if (ret)
@@ -2038,9 +2041,11 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_DP_B:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_VLV;
+		need_scramble_reset = true;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_DP_C:
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_VLV;
+		need_scramble_reset = true;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_NONE:
 		*val = 0;
@@ -2049,6 +2054,21 @@ static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	/* DP port CRC needs slight spec mis-compliance to get stable CRCs. */
+	if (need_scramble_reset) {
+		uint32_t tmp = I915_READ(PORT_DFT2_G4X);
+
+		WARN_ON(!IS_G4X(dev));
+
+		tmp |= DC_BALANCE_RESET_VLV;
+		if (pipe == PIPE_A)
+			tmp |= PIPE_A_SCRAMBLE_RESET;
+		else
+			tmp |= PIPE_B_SCRAMBLE_RESET;
+
+		I915_WRITE(PORT_DFT2_G4X, tmp);
+	}
+
 	return 0;
 }
 
@@ -2120,6 +2140,22 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
 	return 0;
 }
 
+static void vlv_undo_pipe_scramble_reset(struct drm_device *dev,
+					 enum pipe pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t tmp = I915_READ(PORT_DFT2_G4X);
+
+	if (pipe == PIPE_A)
+		tmp &= ~PIPE_A_SCRAMBLE_RESET;
+	else
+		tmp &= ~PIPE_B_SCRAMBLE_RESET;
+	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_device *dev,
 					 enum pipe pipe)
 {
@@ -2259,6 +2295,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 		if (IS_G4X(dev))
 			g4x_undo_pipe_scramble_reset(dev, pipe);
+		else if (IS_VALLEYVIEW(dev))
+			vlv_undo_pipe_scramble_reset(dev, pipe);
 	}
 
 	return 0;
-- 
1.8.4.rc3

[PATCH 4/4] drm/i915: Enable DP port CRC for the "auto" source on g4x/vlv

[Daniel Vetter]

Now that DP port CRCs are stable, we can use it for generic CRC tests.
Yay, the auto CRC source should now work everywhere!

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5942362..7325de9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1990,6 +1990,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
 {
 	struct intel_encoder *encoder;
 	struct intel_crtc *crtc;
+	struct intel_digital_port *dig_port;
 	int ret = 0;
 
 	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
@@ -2011,8 +2012,22 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
 			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
 		case INTEL_OUTPUT_EDP:
-			/* We can't get stable CRCs for DP ports somehow. */
-			ret = -ENODEV;
+			dig_port = enc_to_dig_port(&encoder->base);
+			switch (dig_port->port) {
+			case PORT_B:
+				*source = INTEL_PIPE_CRC_SOURCE_DP_B;
+				break;
+			case PORT_C:
+				*source = INTEL_PIPE_CRC_SOURCE_DP_C;
+				break;
+			case PORT_D:
+				*source = INTEL_PIPE_CRC_SOURCE_DP_D;
+				break;
+			default:
+				WARN(1, "nonexisting DP port %c\n",
+				     port_name(dig_port->port));
+				break;
+			}
 			break;
 		}
 	}
-- 
1.8.4.rc3

Re: [PATCH 2/4] drm/i915: scramble reset support for DP port CRC on g4x

[Damien Lespiau]

On Fri, Nov 01, 2013 at 10:50:21AM +0100, Daniel Vetter wrote:
> +	/* DP port CRC needs slight spec mis-compliance to get stable CRCs. */

We could add a slighty better explanation. How about:

/*
 * 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_scramble_reset) {

so then maybe rename this with 'need_stable_symbols'

> +		uint32_t tmp = I915_READ(PORT_DFT2_G4X);
> +
> +		WARN_ON(!IS_G4X(dev));
> +
> +		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;
>  }
>  
> +static void g4x_undo_pipe_scramble_reset(struct drm_device *dev,
> +					 enum pipe pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t 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);
> +	}
> +}

It took me a bit of time to realize that you were only clearing the
"DC-Balance reset once a frame" bit when you're sure that we have no
pipe CRC needing stable symbols running at all on either pipe. And this
by using the SCRAMBLER_RESET bits in DFT2 as the state tracker.

Maybe add a small comment here to avoid those 20s of head scratching
when reading that code again in a bit.

-- 
Damien

Re: [PATCH 1/4] drm/i916: add "auto" pipe CRC source

[Damien Lespiau]

On Fri, Nov 01, 2013 at 10:50:20AM +0100, Daniel Vetter wrote:
> On gmch platforms the normal pipe source CRC registers don't work for
> DP and TV encoders. And on newer platforms the single pipe CRC has
> been replaced by a set of CRC at different stages in the platform.
> 
> Now most of our userspace tests don't care one bit about the exact
> CRC, they simply want something that reflects any changes on the
> screen. Hence add a new auto target for platform agnostic tests to
> use.
> 
> v2: Pass back the adjusted source so that it can be shown in debugfs.
> 
> v3: I seem to be unable to get a stable CRC for DP ports. So let's
> just disable them for now when using the auto mode. Note that
> testcases need to be restructured so that they can dynamically skip
> connectors. They also first need to set up the desired mode
> configuration, since otherwise the auto mode won't do the right thing.
> 
> v4: Don't leak the modeset mutex on error paths.
> 
> v5: Spelling fix for the i9xx auto_source function.
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On the series (the improved comments and I sent earlier would be a nice
addition):
  Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 91 +++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  2 files changed, 77 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5c45e9e..7c29a88 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1936,6 +1936,7 @@ static const char * const pipe_crc_sources[] = {
>  	"DP-B",
>  	"DP-C",
>  	"DP-D",
> +	"auto",
>  };
>  
>  static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
> @@ -1964,10 +1965,13 @@ static int display_crc_ctl_open(struct inode *inode, struct file *file)
>  	return single_open(file, display_crc_ctl_show, dev);
>  }
>  
> -static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> +static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  				 uint32_t *val)
>  {
> -	switch (source) {
> +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> +
> +	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_INCLUDE_BORDER_I8XX;
>  		break;
> @@ -1981,10 +1985,54 @@ static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
>  	return 0;
>  }
>  
> -static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> +static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
> +				     enum intel_pipe_crc_source *source)
> +{
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *crtc;
> +	int ret = 0;
> +
> +	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +			    base.head) {
> +		if (!encoder->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(encoder->base.crtc);
> +
> +		if (crtc->pipe != pipe)
> +			continue;
> +
> +		switch (encoder->type) {
> +		case INTEL_OUTPUT_TVOUT:
> +			*source = INTEL_PIPE_CRC_SOURCE_TV;
> +			break;
> +		case INTEL_OUTPUT_DISPLAYPORT:
> +		case INTEL_OUTPUT_EDP:
> +			/* We can't get stable CRCs for DP ports somehow. */
> +			ret = -ENODEV;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	return ret;
> +}
> +
> +static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
> +				enum pipe pipe,
> +				enum intel_pipe_crc_source *source,
>  				uint32_t *val)
>  {
> -	switch (source) {
> +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> +		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_VLV;
>  		break;
> @@ -2005,10 +2053,17 @@ static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
>  }
>  
>  static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
> -				 enum intel_pipe_crc_source source,
> +				 enum pipe pipe,
> +				 enum intel_pipe_crc_source *source,
>  				 uint32_t *val)
>  {
> -	switch (source) {
> +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> +		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_I9XX;
>  		break;
> @@ -2042,10 +2097,13 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> +static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  				uint32_t *val)
>  {
> -	switch (source) {
> +	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_PRIMARY_ILK;
>  		break;
> @@ -2065,10 +2123,13 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
>  	return 0;
>  }
>  
> -static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> +static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  				uint32_t *val)
>  {
> -	switch (source) {
> +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> +		*source = INTEL_PIPE_CRC_SOURCE_PF;
> +
> +	switch (*source) {
>  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
>  		break;
> @@ -2104,15 +2165,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		return -EINVAL;
>  
>  	if (IS_GEN2(dev))
> -		ret = i8xx_pipe_crc_ctl_reg(source, &val);
> +		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
>  	else if (INTEL_INFO(dev)->gen < 5)
> -		ret = i9xx_pipe_crc_ctl_reg(dev, source, &val);
> +		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>  	else if (IS_VALLEYVIEW(dev))
> -		ret = vlv_pipe_crc_ctl_reg(source, &val);
> +		ret = vlv_pipe_crc_ctl_reg(dev,pipe, &source, &val);
>  	else if (IS_GEN5(dev) || IS_GEN6(dev))
> -		ret = ilk_pipe_crc_ctl_reg(source, &val);
> +		ret = ilk_pipe_crc_ctl_reg(&source, &val);
>  	else
> -		ret = ivb_pipe_crc_ctl_reg(source, &val);
> +		ret = ivb_pipe_crc_ctl_reg(&source, &val);
>  
>  	if (ret != 0)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2c1921d..b12d942 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1253,6 +1253,7 @@ enum intel_pipe_crc_source {
>  	INTEL_PIPE_CRC_SOURCE_DP_B,
>  	INTEL_PIPE_CRC_SOURCE_DP_C,
>  	INTEL_PIPE_CRC_SOURCE_DP_D,
> +	INTEL_PIPE_CRC_SOURCE_AUTO,
>  	INTEL_PIPE_CRC_SOURCE_MAX,
>  };
>  
> -- 
> 1.8.4.rc3
> 

Re: [PATCH 1/4] drm/i916: add "auto" pipe CRC source

[Daniel Vetter]

On Fri, Nov 01, 2013 at 01:05:34PM +0000, Damien Lespiau wrote:
> On Fri, Nov 01, 2013 at 10:50:20AM +0100, Daniel Vetter wrote:
> > On gmch platforms the normal pipe source CRC registers don't work for
> > DP and TV encoders. And on newer platforms the single pipe CRC has
> > been replaced by a set of CRC at different stages in the platform.
> > 
> > Now most of our userspace tests don't care one bit about the exact
> > CRC, they simply want something that reflects any changes on the
> > screen. Hence add a new auto target for platform agnostic tests to
> > use.
> > 
> > v2: Pass back the adjusted source so that it can be shown in debugfs.
> > 
> > v3: I seem to be unable to get a stable CRC for DP ports. So let's
> > just disable them for now when using the auto mode. Note that
> > testcases need to be restructured so that they can dynamically skip
> > connectors. They also first need to set up the desired mode
> > configuration, since otherwise the auto mode won't do the right thing.
> > 
> > v4: Don't leak the modeset mutex on error paths.
> > 
> > v5: Spelling fix for the i9xx auto_source function.
> > 
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> On the series (the improved comments and I sent earlier would be a nice
> addition):
>   Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Bikesheds applied and patches merged, thanks for your review.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 91 +++++++++++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  2 files changed, 77 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5c45e9e..7c29a88 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1936,6 +1936,7 @@ static const char * const pipe_crc_sources[] = {
> >  	"DP-B",
> >  	"DP-C",
> >  	"DP-D",
> > +	"auto",
> >  };
> >  
> >  static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
> > @@ -1964,10 +1965,13 @@ static int display_crc_ctl_open(struct inode *inode, struct file *file)
> >  	return single_open(file, display_crc_ctl_show, dev);
> >  }
> >  
> > -static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> > +static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> >  				 uint32_t *val)
> >  {
> > -	switch (source) {
> > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > +		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > +
> > +	switch (*source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_INCLUDE_BORDER_I8XX;
> >  		break;
> > @@ -1981,10 +1985,54 @@ static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> >  	return 0;
> >  }
> >  
> > -static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> > +static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
> > +				     enum intel_pipe_crc_source *source)
> > +{
> > +	struct intel_encoder *encoder;
> > +	struct intel_crtc *crtc;
> > +	int ret = 0;
> > +
> > +	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > +
> > +	mutex_lock(&dev->mode_config.mutex);
> > +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> > +			    base.head) {
> > +		if (!encoder->base.crtc)
> > +			continue;
> > +
> > +		crtc = to_intel_crtc(encoder->base.crtc);
> > +
> > +		if (crtc->pipe != pipe)
> > +			continue;
> > +
> > +		switch (encoder->type) {
> > +		case INTEL_OUTPUT_TVOUT:
> > +			*source = INTEL_PIPE_CRC_SOURCE_TV;
> > +			break;
> > +		case INTEL_OUTPUT_DISPLAYPORT:
> > +		case INTEL_OUTPUT_EDP:
> > +			/* We can't get stable CRCs for DP ports somehow. */
> > +			ret = -ENODEV;
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&dev->mode_config.mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int vlv_pipe_crc_ctl_reg(struct drm_device *dev,
> > +				enum pipe pipe,
> > +				enum intel_pipe_crc_source *source,
> >  				uint32_t *val)
> >  {
> > -	switch (source) {
> > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > +		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	switch (*source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_VLV;
> >  		break;
> > @@ -2005,10 +2053,17 @@ static int vlv_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> >  }
> >  
> >  static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
> > -				 enum intel_pipe_crc_source source,
> > +				 enum pipe pipe,
> > +				 enum intel_pipe_crc_source *source,
> >  				 uint32_t *val)
> >  {
> > -	switch (source) {
> > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > +		int ret = i9xx_pipe_crc_auto_source(dev, pipe, source);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	switch (*source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_I9XX;
> >  		break;
> > @@ -2042,10 +2097,13 @@ static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
> >  	return 0;
> >  }
> >  
> > -static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> > +static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> >  				uint32_t *val)
> >  {
> > -	switch (source) {
> > +	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_PRIMARY_ILK;
> >  		break;
> > @@ -2065,10 +2123,13 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> >  	return 0;
> >  }
> >  
> > -static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source source,
> > +static int ivb_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> >  				uint32_t *val)
> >  {
> > -	switch (source) {
> > +	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > +		*source = INTEL_PIPE_CRC_SOURCE_PF;
> > +
> > +	switch (*source) {
> >  	case INTEL_PIPE_CRC_SOURCE_PLANE1:
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PRIMARY_IVB;
> >  		break;
> > @@ -2104,15 +2165,15 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> >  		return -EINVAL;
> >  
> >  	if (IS_GEN2(dev))
> > -		ret = i8xx_pipe_crc_ctl_reg(source, &val);
> > +		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
> >  	else if (INTEL_INFO(dev)->gen < 5)
> > -		ret = i9xx_pipe_crc_ctl_reg(dev, source, &val);
> > +		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
> >  	else if (IS_VALLEYVIEW(dev))
> > -		ret = vlv_pipe_crc_ctl_reg(source, &val);
> > +		ret = vlv_pipe_crc_ctl_reg(dev,pipe, &source, &val);
> >  	else if (IS_GEN5(dev) || IS_GEN6(dev))
> > -		ret = ilk_pipe_crc_ctl_reg(source, &val);
> > +		ret = ilk_pipe_crc_ctl_reg(&source, &val);
> >  	else
> > -		ret = ivb_pipe_crc_ctl_reg(source, &val);
> > +		ret = ivb_pipe_crc_ctl_reg(&source, &val);
> >  
> >  	if (ret != 0)
> >  		return ret;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2c1921d..b12d942 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1253,6 +1253,7 @@ enum intel_pipe_crc_source {
> >  	INTEL_PIPE_CRC_SOURCE_DP_B,
> >  	INTEL_PIPE_CRC_SOURCE_DP_C,
> >  	INTEL_PIPE_CRC_SOURCE_DP_D,
> > +	INTEL_PIPE_CRC_SOURCE_AUTO,
> >  	INTEL_PIPE_CRC_SOURCE_MAX,
> >  };
> >  
> > -- 
> > 1.8.4.rc3
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch