[PATCH v5 3/4] drm/i915: Use new CRC debugfs API

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
To: linux-kernel@vger.kernel.org
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Sean Paul" <seanpaul@chromium.org>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Emil Velikov" <emil.velikov@collabora.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Tomeu Vizoso" <tomeu.vizoso@collabora.com>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	"David Airlie" <airlied@linux.ie>
Subject: [PATCH v5 3/4] drm/i915: Use new CRC debugfs API
Date: Mon,  5 Sep 2016 11:58:16 +0200	[thread overview]
Message-ID: <1473069497-1585-4-git-send-email-tomeu.vizoso@collabora.com> (raw)
In-Reply-To: <1473069497-1585-1-git-send-email-tomeu.vizoso@collabora.com>

The core provides now an ABI to userspace for generation of frame CRCs,
so implement the ->set_crc_source() callback and reuse as much code as
possible with the previous ABI implementation.

v2:
    - Leave the legacy implementation in place as the ABI implementation
      in the core is incompatible with it.
v3:
    - Use the "cooked" vblank counter so we have a whole 32 bits.
    - Make sure we don't mess with the state of the legacy CRC capture
      ABI implementation.
v4:
    - Keep use of get_vblank_counter as in the legacy code, will be
      changed in a followup commit.

v5:
    - Skip first frame or two as it's known that they contain wrong
      data.
    - A few fixes suggested by Emil Velikov.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 drivers/gpu/drm/i915/i915_drv.h       |   1 +
 drivers/gpu/drm/i915/i915_irq.c       |  83 +++++++++++++++-------
 drivers/gpu/drm/i915/intel_display.c  |   1 +
 drivers/gpu/drm/i915/intel_drv.h      |   2 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 130 ++++++++++++++++++++++++----------
 5 files changed, 153 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 403c074a29f4..77d05807adc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1672,6 +1672,7 @@ struct intel_pipe_crc {
 	enum intel_pipe_crc_source source;
 	int head, tail;
 	wait_queue_head_t wq;
+	int skipped;
 };
 
 struct i915_frontbuffer_tracking {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7610eca4f687..413667497ce0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1485,41 +1485,72 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 {
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
-	int head, tail;
+	struct drm_crtc *crtc = intel_get_crtc_for_pipe(&dev_priv->drm, pipe);
+	struct drm_driver *driver = dev_priv->drm.driver;
+	uint32_t crcs[5];
+	int head, tail, ret;
+	u32 frame;
 
 	spin_lock(&pipe_crc->lock);
+	if (pipe_crc->source) {
+		if (!pipe_crc->entries) {
+			spin_unlock(&pipe_crc->lock);
+			DRM_DEBUG_KMS("spurious interrupt\n");
+			return;
+		}
 
-	if (!pipe_crc->entries) {
-		spin_unlock(&pipe_crc->lock);
-		DRM_DEBUG_KMS("spurious interrupt\n");
-		return;
-	}
-
-	head = pipe_crc->head;
-	tail = pipe_crc->tail;
+		head = pipe_crc->head;
+		tail = pipe_crc->tail;
 
-	if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
-		spin_unlock(&pipe_crc->lock);
-		DRM_ERROR("CRC buffer overflowing\n");
-		return;
-	}
+		if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+			spin_unlock(&pipe_crc->lock);
+			DRM_ERROR("CRC buffer overflowing\n");
+			return;
+		}
 
-	entry = &pipe_crc->entries[head];
+		entry = &pipe_crc->entries[head];
 
-	entry->frame = dev_priv->drm.driver->get_vblank_counter(&dev_priv->drm,
-								 pipe);
-	entry->crc[0] = crc0;
-	entry->crc[1] = crc1;
-	entry->crc[2] = crc2;
-	entry->crc[3] = crc3;
-	entry->crc[4] = crc4;
+		entry->frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+		entry->crc[0] = crc0;
+		entry->crc[1] = crc1;
+		entry->crc[2] = crc2;
+		entry->crc[3] = crc3;
+		entry->crc[4] = crc4;
 
-	head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-	pipe_crc->head = head;
+		head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+		pipe_crc->head = head;
 
-	spin_unlock(&pipe_crc->lock);
+		spin_unlock(&pipe_crc->lock);
 
-	wake_up_interruptible(&pipe_crc->wq);
+		wake_up_interruptible(&pipe_crc->wq);
+	} else {
+		/*
+		 * For some not yet identified reason, the first CRC is
+		 * bonkers. So let's just wait for the next vblank and read
+		 * out the buggy result.
+		 *
+		 * On CHV sometimes the second CRC is bonkers as well, so
+		 * don't trust that one either.
+		 */
+		if (pipe_crc->skipped == 0 ||
+		    (IS_CHERRYVIEW(dev_priv) && pipe_crc->skipped == 1)) {
+			pipe_crc->skipped++;
+			spin_unlock(&pipe_crc->lock);
+			return;
+		}
+		spin_unlock(&pipe_crc->lock);
+		spin_lock(&crtc->crc.lock);
+		crcs[0] = crc0;
+		crcs[1] = crc1;
+		crcs[2] = crc2;
+		crcs[3] = crc3;
+		crcs[4] = crc4;
+		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+		ret = drm_crtc_add_crc_entry(crtc, true, frame, crcs);
+		spin_unlock(&crtc->crc.lock);
+		if (!ret)
+			wake_up_interruptible(&crtc->crc.wq);
+	}
 }
 #else
 static inline void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e74660a21b7..07bbf7487dfa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14543,6 +14543,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
+	.set_crc_source = intel_crtc_set_crc_source,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cace88246a03..b9c1d18397d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1831,6 +1831,8 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
 void intel_pipe_crc_cleanup(struct drm_minor *minor);
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
+			      size_t *values_cnt);
 extern const struct file_operations i915_display_crc_ctl_fops;
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index e7e1ddfa0f9d..2ef8c1a663ba 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -624,15 +624,62 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 	return 0;
 }
 
+static int do_set_crc_source(struct drm_device *dev, enum pipe pipe,
+			     enum intel_pipe_crc_source source)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
+									pipe));
+	u32 val = 0; /* shut up gcc */
+	int ret;
+
+	if (IS_GEN2(dev))
+		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
+	else if (INTEL_INFO(dev)->gen < 5)
+		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
+	else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+		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);
+	else
+		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
+
+	if (ret)
+		return ret;
+
+	if (source) {
+		/*
+		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+		 * enabled and disabled dynamically based on package C states,
+		 * user space can't make reliable use of the CRCs, so let's just
+		 * completely disable it.
+		 */
+		hsw_disable_ips(crtc);
+	}
+
+	I915_WRITE(PIPE_CRC_CTL(pipe), val);
+	POSTING_READ(PIPE_CRC_CTL(pipe));
+
+	if (!source) {
+		if (IS_G4X(dev))
+			g4x_undo_pipe_scramble_reset(dev, pipe);
+		else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+			vlv_undo_pipe_scramble_reset(dev, pipe);
+		else if (IS_HASWELL(dev) && pipe == PIPE_A)
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
+
+		hsw_enable_ips(crtc);
+	}
+
+	return 0;
+}
+
 static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 			       enum intel_pipe_crc_source source)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-	struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
-									pipe));
 	enum intel_display_power_domain power_domain;
-	u32 val = 0; /* shut up gcc */
 	int ret;
 
 	if (pipe_crc->source == source)
@@ -648,20 +695,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		return -EIO;
 	}
 
-	if (IS_GEN2(dev))
-		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
-	else if (INTEL_INFO(dev)->gen < 5)
-		ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
-	else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
-		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);
-	else
-		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
-
-	if (ret != 0)
-		goto out;
-
 	/* none -> real source transition */
 	if (source) {
 		struct intel_pipe_crc_entry *entries;
@@ -677,14 +710,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 			goto out;
 		}
 
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		hsw_disable_ips(crtc);
-
 		spin_lock_irq(&pipe_crc->lock);
 		kfree(pipe_crc->entries);
 		pipe_crc->entries = entries;
@@ -693,13 +718,14 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		spin_unlock_irq(&pipe_crc->lock);
 	}
 
-	pipe_crc->source = source;
+	ret = do_set_crc_source(dev, pipe, source);
+	if (ret)
+		goto out;
 
-	I915_WRITE(PIPE_CRC_CTL(pipe), val);
-	POSTING_READ(PIPE_CRC_CTL(pipe));
+	pipe_crc->source = source;
 
 	/* real source -> none transition */
-	if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+	if (!source) {
 		struct intel_pipe_crc_entry *entries;
 		struct intel_crtc *crtc =
 			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -720,15 +746,6 @@ 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);
-		else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
-			vlv_undo_pipe_scramble_reset(dev, pipe);
-		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev, false);
-
-		hsw_enable_ips(crtc);
 	}
 
 	ret = 0;
@@ -821,6 +838,11 @@ display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
 {
 	int i;
 
+	if (!buf) {
+		*s = INTEL_PIPE_CRC_SOURCE_NONE;
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
 		if (!strcmp(buf, pipe_crc_sources[i])) {
 			*s = i;
@@ -949,3 +971,35 @@ void intel_pipe_crc_cleanup(struct drm_minor *minor)
 		drm_debugfs_remove_files(info_list, 1, minor);
 	}
 }
+
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
+			      size_t *values_cnt)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+	enum intel_display_power_domain power_domain;
+	enum intel_pipe_crc_source source;
+	int ret;
+
+	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
+		return -EINVAL;
+	}
+
+	power_domain = POWER_DOMAIN_PIPE(crtc->index);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
+		DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
+		return -EIO;
+	}
+
+	ret = do_set_crc_source(crtc->dev, crtc->index, source);
+
+	intel_display_power_put(dev_priv, power_domain);
+
+	if (!ret) {
+		pipe_crc->skipped = 0;
+		*values_cnt = 5;
+	}
+
+	return ret;
+}
-- 
2.7.4
