* [PATCH v14 1/4] drm: Move locking into drm_debugfs_crtc_crc_add
2017-01-02 12:59 [PATCH v14 0/4] New debugfs API for capturing CRC of frames Tomeu Vizoso
@ 2017-01-02 12:59 ` Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 2/4] drm: crc: Wait for a frame before returning from open() Tomeu Vizoso
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Tomeu Vizoso @ 2017-01-02 12:59 UTC (permalink / raw)
To: linux-kernel
Cc: Ville Syrjälä,
Sean Paul, Thierry Reding, Emil Velikov, Daniel Vetter,
Jani Nikula, Tomeu Vizoso, dri-devel, David Airlie
There's no reason any more for callers of this function to take the lock
themselves, so just move the lock to the function to avoid confusion and
bugs when more callers are contributed.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
drivers/gpu/drm/drm_debugfs_crc.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 00e771fb7df2..68b171af237b 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -325,16 +325,19 @@ int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
struct drm_crtc_crc_entry *entry;
int head, tail;
- assert_spin_locked(&crc->lock);
+ spin_lock(&crc->lock);
/* Caller may not have noticed yet that userspace has stopped reading */
- if (!crc->opened)
+ if (!crc->opened) {
+ spin_unlock(&crc->lock);
return -EINVAL;
+ }
head = crc->head;
tail = crc->tail;
if (CIRC_SPACE(head, tail, DRM_CRC_ENTRIES_NR) < 1) {
+ spin_unlock(&crc->lock);
DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
return -ENOBUFS;
}
@@ -347,6 +350,8 @@ int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
head = (head + 1) & (DRM_CRC_ENTRIES_NR - 1);
crc->head = head;
+ spin_unlock(&crc->lock);
+
return 0;
}
EXPORT_SYMBOL_GPL(drm_crtc_add_crc_entry);
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v14 2/4] drm: crc: Wait for a frame before returning from open()
2017-01-02 12:59 [PATCH v14 0/4] New debugfs API for capturing CRC of frames Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 1/4] drm: Move locking into drm_debugfs_crtc_crc_add Tomeu Vizoso
@ 2017-01-02 12:59 ` Tomeu Vizoso
2017-01-05 14:14 ` Daniel Vetter
2017-01-02 12:59 ` [PATCH v14 3/4] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 4/4] drm/i915: Put "cooked" vlank counters in frame CRC lines Tomeu Vizoso
3 siblings, 1 reply; 6+ messages in thread
From: Tomeu Vizoso @ 2017-01-02 12:59 UTC (permalink / raw)
To: linux-kernel
Cc: Ville Syrjälä,
Sean Paul, Thierry Reding, Emil Velikov, Daniel Vetter,
Jani Nikula, Tomeu Vizoso, dri-devel, David Airlie
Don't return from the open() call on the crc/data file until the HW has
produced a first frame, as there's great variability in when the HW is
able to do that and userspace shouldn't have to guess when this specific
HW is ready to start giving frame CRCs.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/drm_debugfs_crc.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 68b171af237b..8b0eeeed4d78 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -125,6 +125,12 @@ static const struct file_operations drm_crtc_crc_control_fops = {
.write = crc_control_write
};
+static int crtc_crc_data_count(struct drm_crtc_crc *crc)
+{
+ assert_spin_locked(&crc->lock);
+ return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
+}
+
static int crtc_crc_open(struct inode *inode, struct file *filep)
{
struct drm_crtc *crtc = inode->i_private;
@@ -160,8 +166,19 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
crc->entries = entries;
crc->values_cnt = values_cnt;
crc->opened = true;
+
+ /*
+ * Only return once we got a first frame, so userspace doesn't have to
+ * guess when this particular piece of HW will be ready to start
+ * generating CRCs.
+ */
+ ret = wait_event_interruptible_lock_irq(crc->wq,
+ crtc_crc_data_count(crc),
+ crc->lock);
spin_unlock_irq(&crc->lock);
+ WARN_ON(ret);
+
return 0;
err_disable:
@@ -189,12 +206,6 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
return 0;
}
-static int crtc_crc_data_count(struct drm_crtc_crc *crc)
-{
- assert_spin_locked(&crc->lock);
- return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
-}
-
/*
* 1 frame field of 10 chars plus a number of CRC fields of 10 chars each, space
* separated, with a newline at the end and null-terminated.
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v14 3/4] drm/i915: Use new CRC debugfs API
2017-01-02 12:59 [PATCH v14 0/4] New debugfs API for capturing CRC of frames Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 1/4] drm: Move locking into drm_debugfs_crtc_crc_add Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 2/4] drm: crc: Wait for a frame before returning from open() Tomeu Vizoso
@ 2017-01-02 12:59 ` Tomeu Vizoso
2017-01-02 12:59 ` [PATCH v14 4/4] drm/i915: Put "cooked" vlank counters in frame CRC lines Tomeu Vizoso
3 siblings, 0 replies; 6+ messages in thread
From: Tomeu Vizoso @ 2017-01-02 12:59 UTC (permalink / raw)
To: linux-kernel
Cc: Ville Syrjälä,
Sean Paul, Thierry Reding, Emil Velikov, Daniel Vetter,
Jani Nikula, Tomeu Vizoso, intel-gfx, dri-devel, David Airlie
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.
When handling the pageflip interrupt, we skip 1 or 2 frames depending on
the HW because they contain wrong values. For the legacy ABI for
generating frame CRCs, this was done in userspace but now that we have a
generic ABI it's better if it's not exposed by the kernel.
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.
v6:
- Rework programming of the HW registers to preserve previous
behavior.
v7:
- Address whitespace issue.
- Added a comment on why in the implementation of the new ABI we
skip the 1st or 2nd frames.
v9:
- Add stub for intel_crtc_set_crc_source.
v12:
- Rebased.
- Remove stub for intel_crtc_set_crc_source and instead set the
callback to NULL (Jani Nikula).
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_irq.c | 81 ++++++++++++++++++++----------
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 6 +++
drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++++++++++++++++++++++++++++++-----
5 files changed, 145 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 77d7a079c51b..b8a66cc225ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1886,6 +1886,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 a0e70f5b3aad..24b7620c1005 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1553,41 +1553,70 @@ 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 intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, 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);
+ 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->base, true, frame, crcs);
+ if (!ret)
+ wake_up_interruptible(&crtc->base.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 ef5dde5ab1cf..e3fc348ca3e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14774,6 +14774,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 025e4c8b3e63..e14906090241 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1882,5 +1882,11 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
/* intel_pipe_crc.c */
int intel_pipe_crc_create(struct drm_minor *minor);
void intel_pipe_crc_cleanup(struct drm_minor *minor);
+#ifdef CONFIG_DEBUG_FS
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
+ size_t *values_cnt);
+#else
+#define intel_crtc_set_crc_source NULL
+#endif
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 ef0c0e195164..0f1da810cff0 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -613,6 +613,22 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
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)
+{
+ if (IS_GEN2(dev_priv))
+ return i8xx_pipe_crc_ctl_reg(source, val);
+ else if (INTEL_GEN(dev_priv) < 5)
+ return i9xx_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+ else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+ else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
+ return ilk_pipe_crc_ctl_reg(source, val);
+ else
+ return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+}
+
static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
enum pipe pipe,
enum intel_pipe_crc_source source)
@@ -636,17 +652,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
return -EIO;
}
- if (IS_GEN2(dev_priv))
- ret = i8xx_pipe_crc_ctl_reg(&source, &val);
- else if (INTEL_GEN(dev_priv) < 5)
- ret = i9xx_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
- else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
- ret = vlv_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
- else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
- ret = ilk_pipe_crc_ctl_reg(&source, &val);
- else
- ret = ivb_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
-
+ ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
if (ret != 0)
goto out;
@@ -687,7 +693,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
POSTING_READ(PIPE_CRC_CTL(pipe));
/* real source -> none transition */
- if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+ if (!source) {
struct intel_pipe_crc_entry *entries;
struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
pipe);
@@ -809,6 +815,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;
@@ -937,3 +948,62 @@ 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];
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum intel_display_power_domain power_domain;
+ enum intel_pipe_crc_source source;
+ u32 val = 0; /* shut up gcc */
+ int ret = 0;
+
+ 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 = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+ if (ret != 0)
+ goto out;
+
+ 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(intel_crtc);
+ }
+
+ I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+ 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))
+ vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
+ else if (IS_HASWELL(dev_priv) && crtc->index == PIPE_A)
+ hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
+
+ hsw_enable_ips(intel_crtc);
+ }
+
+ pipe_crc->skipped = 0;
+ *values_cnt = 5;
+
+out:
+ intel_display_power_put(dev_priv, power_domain);
+
+ return ret;
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v14 4/4] drm/i915: Put "cooked" vlank counters in frame CRC lines
2017-01-02 12:59 [PATCH v14 0/4] New debugfs API for capturing CRC of frames Tomeu Vizoso
` (2 preceding siblings ...)
2017-01-02 12:59 ` [PATCH v14 3/4] drm/i915: Use new CRC debugfs API Tomeu Vizoso
@ 2017-01-02 12:59 ` Tomeu Vizoso
3 siblings, 0 replies; 6+ messages in thread
From: Tomeu Vizoso @ 2017-01-02 12:59 UTC (permalink / raw)
To: linux-kernel
Cc: Ville Syrjälä,
Sean Paul, Thierry Reding, Emil Velikov, Daniel Vetter,
Jani Nikula, Tomeu Vizoso, intel-gfx, dri-devel, David Airlie
Use drm_accurate_vblank_count so we have the full 32 bit to represent
the frame counter and userspace has a simpler way of knowing when the
counter wraps around.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
drivers/gpu/drm/i915/i915_irq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 24b7620c1005..c0e3d1c1b4f0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1557,7 +1557,6 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
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) {
@@ -1612,8 +1611,9 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
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->base, true, frame, crcs);
+ ret = drm_crtc_add_crc_entry(&crtc->base, true,
+ drm_accurate_vblank_count(&crtc->base),
+ crcs);
if (!ret)
wake_up_interruptible(&crtc->base.crc.wq);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread