* [PATCH v3 0/3] New debugfs API for capturing CRC of frames
@ 2016-07-22 14:10 Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Tomeu Vizoso @ 2016-07-22 14:10 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Thierry Reding, Tomeu Vizoso,
linux-doc, Jani Nikula, intel-gfx, dri-devel, Jonathan Corbet,
David Airlie
Hi,
this series basically takes the facility for continuously capturing CRCs
of frames from the i915 driver and into the DRM core.
The idea is that test suites such as IGT use this information to check
that frames that are exected to be identical, also have identical CRC
values.
Other drivers for hardware that can provide frame CRCs (including eDP
panels that support self-refresh) can easily implement the new callback
and provide userspace with the CRC values.
Thanks,
Tomeu
Tomeu Vizoso (3):
drm/i915/debugfs: Move out pipe CRC code
drm: Add API for capturing frame CRCs
drm/i915: Use new CRC debugfs API
Documentation/gpu/drm-uapi.rst | 6 +
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/drm_crtc.c | 12 +
drivers/gpu/drm/drm_debugfs.c | 36 +-
drivers/gpu/drm/drm_debugfs_crc.c | 370 ++++++++++++
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_internal.h | 10 +
drivers/gpu/drm/i915/Makefile | 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 892 +----------------------------
drivers/gpu/drm/i915/i915_irq.c | 69 ++-
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 7 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 1001 +++++++++++++++++++++++++++++++++
include/drm/drmP.h | 5 +
include/drm/drm_crtc.h | 41 ++
include/drm/drm_debugfs_crc.h | 74 +++
16 files changed, 1621 insertions(+), 917 deletions(-)
create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
create mode 100644 drivers/gpu/drm/i915/intel_pipe_crc.c
create mode 100644 include/drm/drm_debugfs_crc.h
--
2.5.5
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/3] drm/i915/debugfs: Move out pipe CRC code
2016-07-22 14:10 [PATCH v3 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
@ 2016-07-22 14:10 ` Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2 siblings, 0 replies; 12+ messages in thread
From: Tomeu Vizoso @ 2016-07-22 14:10 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Thierry Reding, Tomeu Vizoso,
Jani Nikula, intel-gfx, dri-devel, David Airlie
In preparation to using a generic API in the DRM core for continuous CRC
generation, move the related code out of i915_debugfs.c into a new file.
Eventually, only the Intel-specific code will remain in this new file.
v2: Rebased.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/i915/Makefile | 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 892 +------------------------------
drivers/gpu/drm/i915/intel_drv.h | 5 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 951 ++++++++++++++++++++++++++++++++++
4 files changed, 961 insertions(+), 889 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_pipe_crc.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 684fc1cd08fa..9419e0c2e2c0 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -19,7 +19,7 @@ i915-y := i915_drv.o \
intel_runtime_pm.o
i915-$(CONFIG_COMPAT) += i915_ioc32.o
-i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
+i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o
# GEM code
i915-y += i915_cmd_parser.o \
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a59e0caeda64..c37bfda32e53 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -26,19 +26,9 @@
*
*/
-#include <linux/seq_file.h>
-#include <linux/circ_buf.h>
-#include <linux/ctype.h>
#include <linux/debugfs.h>
-#include <linux/slab.h>
-#include <linux/export.h>
#include <linux/list_sort.h>
-#include <asm/msr-index.h>
-#include <drm/drmP.h>
#include "intel_drv.h"
-#include "intel_ringbuffer.h"
-#include <drm/i915_drm.h>
-#include "i915_drv.h"
enum {
ACTIVE_LIST,
@@ -3490,12 +3480,6 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
return 0;
}
-struct pipe_crc_info {
- const char *name;
- struct drm_device *dev;
- enum pipe pipe;
-};
-
static int i915_dp_mst_info(struct seq_file *m, void *unused)
{
struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -3525,853 +3509,6 @@ static int i915_dp_mst_info(struct seq_file *m, void *unused)
return 0;
}
-static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
-{
- struct pipe_crc_info *info = inode->i_private;
- struct drm_i915_private *dev_priv = to_i915(info->dev);
- struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
-
- if (info->pipe >= INTEL_INFO(info->dev)->num_pipes)
- return -ENODEV;
-
- spin_lock_irq(&pipe_crc->lock);
-
- if (pipe_crc->opened) {
- spin_unlock_irq(&pipe_crc->lock);
- return -EBUSY; /* already open */
- }
-
- pipe_crc->opened = true;
- filep->private_data = inode->i_private;
-
- spin_unlock_irq(&pipe_crc->lock);
-
- return 0;
-}
-
-static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
-{
- struct pipe_crc_info *info = inode->i_private;
- struct drm_i915_private *dev_priv = to_i915(info->dev);
- struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
-
- spin_lock_irq(&pipe_crc->lock);
- pipe_crc->opened = false;
- spin_unlock_irq(&pipe_crc->lock);
-
- return 0;
-}
-
-/* (6 fields, 8 chars each, space separated (5) + '\n') */
-#define PIPE_CRC_LINE_LEN (6 * 8 + 5 + 1)
-/* account for \'0' */
-#define PIPE_CRC_BUFFER_LEN (PIPE_CRC_LINE_LEN + 1)
-
-static int pipe_crc_data_count(struct intel_pipe_crc *pipe_crc)
-{
- assert_spin_locked(&pipe_crc->lock);
- return CIRC_CNT(pipe_crc->head, pipe_crc->tail,
- INTEL_PIPE_CRC_ENTRIES_NR);
-}
-
-static ssize_t
-i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
- loff_t *pos)
-{
- struct pipe_crc_info *info = filep->private_data;
- struct drm_device *dev = info->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
- char buf[PIPE_CRC_BUFFER_LEN];
- int n_entries;
- ssize_t bytes_read;
-
- /*
- * Don't allow user space to provide buffers not big enough to hold
- * a line of data.
- */
- if (count < PIPE_CRC_LINE_LEN)
- return -EINVAL;
-
- if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
- return 0;
-
- /* nothing to read */
- spin_lock_irq(&pipe_crc->lock);
- while (pipe_crc_data_count(pipe_crc) == 0) {
- int ret;
-
- if (filep->f_flags & O_NONBLOCK) {
- spin_unlock_irq(&pipe_crc->lock);
- return -EAGAIN;
- }
-
- ret = wait_event_interruptible_lock_irq(pipe_crc->wq,
- pipe_crc_data_count(pipe_crc), pipe_crc->lock);
- if (ret) {
- spin_unlock_irq(&pipe_crc->lock);
- return ret;
- }
- }
-
- /* We now have one or more entries to read */
- n_entries = count / PIPE_CRC_LINE_LEN;
-
- bytes_read = 0;
- while (n_entries > 0) {
- struct intel_pipe_crc_entry *entry =
- &pipe_crc->entries[pipe_crc->tail];
- int ret;
-
- if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
- INTEL_PIPE_CRC_ENTRIES_NR) < 1)
- break;
-
- BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
- pipe_crc->tail = (pipe_crc->tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-
- bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
- "%8u %8x %8x %8x %8x %8x\n",
- entry->frame, entry->crc[0],
- entry->crc[1], entry->crc[2],
- entry->crc[3], entry->crc[4]);
-
- spin_unlock_irq(&pipe_crc->lock);
-
- ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
- if (ret == PIPE_CRC_LINE_LEN)
- return -EFAULT;
-
- user_buf += PIPE_CRC_LINE_LEN;
- n_entries--;
-
- spin_lock_irq(&pipe_crc->lock);
- }
-
- spin_unlock_irq(&pipe_crc->lock);
-
- return bytes_read;
-}
-
-static const struct file_operations i915_pipe_crc_fops = {
- .owner = THIS_MODULE,
- .open = i915_pipe_crc_open,
- .read = i915_pipe_crc_read,
- .release = i915_pipe_crc_release,
-};
-
-static struct pipe_crc_info i915_pipe_crc_data[I915_MAX_PIPES] = {
- {
- .name = "i915_pipe_A_crc",
- .pipe = PIPE_A,
- },
- {
- .name = "i915_pipe_B_crc",
- .pipe = PIPE_B,
- },
- {
- .name = "i915_pipe_C_crc",
- .pipe = PIPE_C,
- },
-};
-
-static int i915_pipe_crc_create(struct dentry *root, struct drm_minor *minor,
- enum pipe pipe)
-{
- struct drm_device *dev = minor->dev;
- struct dentry *ent;
- struct pipe_crc_info *info = &i915_pipe_crc_data[pipe];
-
- info->dev = dev;
- ent = debugfs_create_file(info->name, S_IRUGO, root, info,
- &i915_pipe_crc_fops);
- if (!ent)
- return -ENOMEM;
-
- return drm_add_fake_info_node(minor, ent, info);
-}
-
-static const char * const pipe_crc_sources[] = {
- "none",
- "plane1",
- "plane2",
- "pf",
- "pipe",
- "TV",
- "DP-B",
- "DP-C",
- "DP-D",
- "auto",
-};
-
-static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
-{
- BUILD_BUG_ON(ARRAY_SIZE(pipe_crc_sources) != INTEL_PIPE_CRC_SOURCE_MAX);
- return pipe_crc_sources[source];
-}
-
-static int display_crc_ctl_show(struct seq_file *m, void *data)
-{
- struct drm_device *dev = m->private;
- struct drm_i915_private *dev_priv = to_i915(dev);
- int i;
-
- for (i = 0; i < I915_MAX_PIPES; i++)
- seq_printf(m, "%c %s\n", pipe_name(i),
- pipe_crc_source_name(dev_priv->pipe_crc[i].source));
-
- return 0;
-}
-
-static int display_crc_ctl_open(struct inode *inode, struct file *file)
-{
- struct drm_device *dev = inode->i_private;
-
- return single_open(file, display_crc_ctl_show, dev);
-}
-
-static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
- uint32_t *val)
-{
- 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;
- case INTEL_PIPE_CRC_SOURCE_NONE:
- *val = 0;
- break;
- default:
- return -EINVAL;
- }
-
- return 0;
-}
-
-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;
- struct intel_digital_port *dig_port;
- int ret = 0;
-
- *source = INTEL_PIPE_CRC_SOURCE_PIPE;
-
- drm_modeset_lock_all(dev);
- for_each_intel_encoder(dev, encoder) {
- 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:
- 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;
- default:
- break;
- }
- }
- drm_modeset_unlock_all(dev);
-
- 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)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- bool need_stable_symbols = false;
-
- 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;
- case INTEL_PIPE_CRC_SOURCE_DP_B:
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_VLV;
- need_stable_symbols = true;
- break;
- case INTEL_PIPE_CRC_SOURCE_DP_C:
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_VLV;
- need_stable_symbols = true;
- break;
- case INTEL_PIPE_CRC_SOURCE_DP_D:
- if (!IS_CHERRYVIEW(dev))
- return -EINVAL;
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_VLV;
- need_stable_symbols = true;
- break;
- case INTEL_PIPE_CRC_SOURCE_NONE:
- *val = 0;
- break;
- default:
- 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) {
- uint32_t tmp = I915_READ(PORT_DFT2_G4X);
-
- tmp |= DC_BALANCE_RESET_VLV;
- switch (pipe) {
- case PIPE_A:
- tmp |= PIPE_A_SCRAMBLE_RESET;
- break;
- case PIPE_B:
- tmp |= PIPE_B_SCRAMBLE_RESET;
- break;
- case PIPE_C:
- tmp |= PIPE_C_SCRAMBLE_RESET;
- break;
- default:
- return -EINVAL;
- }
- I915_WRITE(PORT_DFT2_G4X, tmp);
- }
-
- return 0;
-}
-
-static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
- enum pipe pipe,
- enum intel_pipe_crc_source *source,
- uint32_t *val)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- bool need_stable_symbols = false;
-
- 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;
- case INTEL_PIPE_CRC_SOURCE_TV:
- if (!SUPPORTS_TV(dev))
- return -EINVAL;
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
- break;
- case INTEL_PIPE_CRC_SOURCE_DP_B:
- if (!IS_G4X(dev))
- 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))
- 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))
- 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:
- 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) {
- 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 vlv_undo_pipe_scramble_reset(struct drm_device *dev,
- enum pipe pipe)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- uint32_t tmp = I915_READ(PORT_DFT2_G4X);
-
- switch (pipe) {
- case PIPE_A:
- tmp &= ~PIPE_A_SCRAMBLE_RESET;
- break;
- case PIPE_B:
- tmp &= ~PIPE_B_SCRAMBLE_RESET;
- break;
- case PIPE_C:
- tmp &= ~PIPE_C_SCRAMBLE_RESET;
- break;
- default:
- return;
- }
- 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)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- 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)
-{
- 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;
- case INTEL_PIPE_CRC_SOURCE_PLANE2:
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_ILK;
- break;
- case INTEL_PIPE_CRC_SOURCE_PIPE:
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_ILK;
- break;
- case INTEL_PIPE_CRC_SOURCE_NONE:
- *val = 0;
- break;
- default:
- return -EINVAL;
- }
-
- return 0;
-}
-
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_crtc *crtc =
- to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
- struct intel_crtc_state *pipe_config;
- struct drm_atomic_state *state;
- int ret = 0;
-
- drm_modeset_lock_all(dev);
- state = drm_atomic_state_alloc(dev);
- if (!state) {
- ret = -ENOMEM;
- goto out;
- }
-
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
- pipe_config = intel_atomic_get_crtc_state(state, crtc);
- if (IS_ERR(pipe_config)) {
- ret = PTR_ERR(pipe_config);
- goto out;
- }
-
- pipe_config->pch_pfit.force_thru = enable;
- if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
- pipe_config->pch_pfit.enabled != enable)
- pipe_config->base.connectors_changed = true;
-
- ret = drm_atomic_commit(state);
-out:
- drm_modeset_unlock_all(dev);
- WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
- if (ret)
- drm_atomic_state_free(state);
-}
-
-static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
- enum pipe pipe,
- enum intel_pipe_crc_source *source,
- uint32_t *val)
-{
- 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;
- case INTEL_PIPE_CRC_SOURCE_PLANE2:
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
- break;
- case INTEL_PIPE_CRC_SOURCE_PF:
- if (IS_HASWELL(dev) && pipe == PIPE_A)
- hsw_trans_edp_pipe_A_crc_wa(dev, true);
-
- *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
- break;
- case INTEL_PIPE_CRC_SOURCE_NONE:
- *val = 0;
- break;
- default:
- return -EINVAL;
- }
-
- 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 = to_i915(dev);
- 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)
- return 0;
-
- /* forbid changing the source without going back to 'none' */
- if (pipe_crc->source && source)
- return -EINVAL;
-
- power_domain = POWER_DOMAIN_PIPE(pipe);
- 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;
- }
-
- 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;
-
- DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
- pipe_name(pipe), pipe_crc_source_name(source));
-
- entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
- sizeof(pipe_crc->entries[0]),
- GFP_KERNEL);
- if (!entries) {
- ret = -ENOMEM;
- 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;
- pipe_crc->head = 0;
- pipe_crc->tail = 0;
- spin_unlock_irq(&pipe_crc->lock);
- }
-
- pipe_crc->source = source;
-
- I915_WRITE(PIPE_CRC_CTL(pipe), val);
- POSTING_READ(PIPE_CRC_CTL(pipe));
-
- /* real source -> none transition */
- if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
- struct intel_pipe_crc_entry *entries;
- struct intel_crtc *crtc =
- to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-
- DRM_DEBUG_DRIVER("stopping CRCs for pipe %c\n",
- pipe_name(pipe));
-
- drm_modeset_lock(&crtc->base.mutex, NULL);
- if (crtc->base.state->active)
- intel_wait_for_vblank(dev, pipe);
- drm_modeset_unlock(&crtc->base.mutex);
-
- spin_lock_irq(&pipe_crc->lock);
- entries = pipe_crc->entries;
- pipe_crc->entries = NULL;
- pipe_crc->head = 0;
- pipe_crc->tail = 0;
- 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;
-
-out:
- intel_display_power_put(dev_priv, power_domain);
-
- return ret;
-}
-
-/*
- * Parse pipe CRC command strings:
- * command: wsp* object wsp+ name wsp+ source wsp*
- * object: 'pipe'
- * name: (A | B | C)
- * source: (none | plane1 | plane2 | pf)
- * wsp: (#0x20 | #0x9 | #0xA)+
- *
- * eg.:
- * "pipe A plane1" -> Start CRC computations on plane1 of pipe A
- * "pipe A none" -> Stop CRC
- */
-static int display_crc_ctl_tokenize(char *buf, char *words[], int max_words)
-{
- int n_words = 0;
-
- while (*buf) {
- char *end;
-
- /* skip leading white space */
- buf = skip_spaces(buf);
- if (!*buf)
- break; /* end of buffer */
-
- /* find end of word */
- for (end = buf; *end && !isspace(*end); end++)
- ;
-
- if (n_words == max_words) {
- DRM_DEBUG_DRIVER("too many words, allowed <= %d\n",
- max_words);
- return -EINVAL; /* ran out of words[] before bytes */
- }
-
- if (*end)
- *end++ = '\0';
- words[n_words++] = buf;
- buf = end;
- }
-
- return n_words;
-}
-
-enum intel_pipe_crc_object {
- PIPE_CRC_OBJECT_PIPE,
-};
-
-static const char * const pipe_crc_objects[] = {
- "pipe",
-};
-
-static int
-display_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *o)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(pipe_crc_objects); i++)
- if (!strcmp(buf, pipe_crc_objects[i])) {
- *o = i;
- return 0;
- }
-
- return -EINVAL;
-}
-
-static int display_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
-{
- const char name = buf[0];
-
- if (name < 'A' || name >= pipe_name(I915_MAX_PIPES))
- return -EINVAL;
-
- *pipe = name - 'A';
-
- return 0;
-}
-
-static int
-display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
- if (!strcmp(buf, pipe_crc_sources[i])) {
- *s = i;
- return 0;
- }
-
- return -EINVAL;
-}
-
-static int display_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
-{
-#define N_WORDS 3
- int n_words;
- char *words[N_WORDS];
- enum pipe pipe;
- enum intel_pipe_crc_object object;
- enum intel_pipe_crc_source source;
-
- n_words = display_crc_ctl_tokenize(buf, words, N_WORDS);
- if (n_words != N_WORDS) {
- DRM_DEBUG_DRIVER("tokenize failed, a command is %d words\n",
- N_WORDS);
- return -EINVAL;
- }
-
- if (display_crc_ctl_parse_object(words[0], &object) < 0) {
- DRM_DEBUG_DRIVER("unknown object %s\n", words[0]);
- return -EINVAL;
- }
-
- if (display_crc_ctl_parse_pipe(words[1], &pipe) < 0) {
- DRM_DEBUG_DRIVER("unknown pipe %s\n", words[1]);
- return -EINVAL;
- }
-
- if (display_crc_ctl_parse_source(words[2], &source) < 0) {
- DRM_DEBUG_DRIVER("unknown source %s\n", words[2]);
- return -EINVAL;
- }
-
- return pipe_crc_set_source(dev, pipe, source);
-}
-
-static ssize_t display_crc_ctl_write(struct file *file, const char __user *ubuf,
- size_t len, loff_t *offp)
-{
- struct seq_file *m = file->private_data;
- struct drm_device *dev = m->private;
- char *tmpbuf;
- int ret;
-
- if (len == 0)
- return 0;
-
- if (len > PAGE_SIZE - 1) {
- DRM_DEBUG_DRIVER("expected <%lu bytes into pipe crc control\n",
- PAGE_SIZE);
- return -E2BIG;
- }
-
- tmpbuf = kmalloc(len + 1, GFP_KERNEL);
- if (!tmpbuf)
- return -ENOMEM;
-
- if (copy_from_user(tmpbuf, ubuf, len)) {
- ret = -EFAULT;
- goto out;
- }
- tmpbuf[len] = '\0';
-
- ret = display_crc_ctl_parse(dev, tmpbuf, len);
-
-out:
- kfree(tmpbuf);
- if (ret < 0)
- return ret;
-
- *offp += len;
- return len;
-}
-
-static const struct file_operations i915_display_crc_ctl_fops = {
- .owner = THIS_MODULE,
- .open = display_crc_ctl_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release,
- .write = display_crc_ctl_write
-};
-
static ssize_t i915_displayport_test_active_write(struct file *file,
const char __user *ubuf,
size_t len, loff_t *offp)
@@ -5467,20 +4604,6 @@ static const struct i915_debugfs_files {
{"i915_dp_test_active", &i915_displayport_test_active_fops}
};
-void intel_display_crc_init(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = to_i915(dev);
- enum pipe pipe;
-
- for_each_pipe(dev_priv, pipe) {
- struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
-
- pipe_crc->opened = false;
- spin_lock_init(&pipe_crc->lock);
- init_waitqueue_head(&pipe_crc->wq);
- }
-}
-
int i915_debugfs_register(struct drm_i915_private *dev_priv)
{
struct drm_minor *minor = dev_priv->drm.primary;
@@ -5490,11 +4613,9 @@ int i915_debugfs_register(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
- ret = i915_pipe_crc_create(minor->debugfs_root, minor, i);
- if (ret)
- return ret;
- }
+ ret = intel_pipe_crc_create(minor);
+ if (ret)
+ return ret;
for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
ret = i915_debugfs_create(minor->debugfs_root, minor,
@@ -5520,12 +4641,7 @@ void i915_debugfs_unregister(struct drm_i915_private *dev_priv)
drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
1, minor);
- for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
- struct drm_info_list *info_list =
- (struct drm_info_list *)&i915_pipe_crc_data[i];
-
- drm_debugfs_remove_files(info_list, 1, minor);
- }
+ intel_pipe_crc_cleanup(minor);
for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
struct drm_info_list *info_list =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e6a24d2154d1..5d06cb2425a1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1779,4 +1779,9 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
void intel_color_set_csc(struct drm_crtc_state *crtc_state);
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);
+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
new file mode 100644
index 000000000000..4e6c28ce8ecf
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -0,0 +1,951 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Author: Damien Lespiau <damien.lespiau@intel.com>
+ *
+ */
+
+#include <linux/seq_file.h>
+#include <linux/circ_buf.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
+#include "intel_drv.h"
+
+struct pipe_crc_info {
+ const char *name;
+ struct drm_device *dev;
+ enum pipe pipe;
+};
+
+/* As the drm_debugfs_init() routines are called before dev->dev_private is
+ * allocated we need to hook into the minor for release.
+ */
+static int drm_add_fake_info_node(struct drm_minor *minor,
+ struct dentry *ent, const void *key)
+{
+ struct drm_info_node *node;
+
+ node = kmalloc(sizeof(*node), GFP_KERNEL);
+ if (node == NULL) {
+ debugfs_remove(ent);
+ return -ENOMEM;
+ }
+
+ node->minor = minor;
+ node->dent = ent;
+ node->info_ent = (void *) key;
+
+ mutex_lock(&minor->debugfs_lock);
+ list_add(&node->list, &minor->debugfs_list);
+ mutex_unlock(&minor->debugfs_lock);
+
+ return 0;
+}
+
+static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
+{
+ struct pipe_crc_info *info = inode->i_private;
+ struct drm_i915_private *dev_priv = info->dev->dev_private;
+ struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+
+ if (info->pipe >= INTEL_INFO(info->dev)->num_pipes)
+ return -ENODEV;
+
+ spin_lock_irq(&pipe_crc->lock);
+
+ if (pipe_crc->opened) {
+ spin_unlock_irq(&pipe_crc->lock);
+ return -EBUSY; /* already open */
+ }
+
+ pipe_crc->opened = true;
+ filep->private_data = inode->i_private;
+
+ spin_unlock_irq(&pipe_crc->lock);
+
+ return 0;
+}
+
+static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
+{
+ struct pipe_crc_info *info = inode->i_private;
+ struct drm_i915_private *dev_priv = info->dev->dev_private;
+ struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+
+ spin_lock_irq(&pipe_crc->lock);
+ pipe_crc->opened = false;
+ spin_unlock_irq(&pipe_crc->lock);
+
+ return 0;
+}
+
+/* (6 fields, 8 chars each, space separated (5) + '\n') */
+#define PIPE_CRC_LINE_LEN (6 * 8 + 5 + 1)
+/* account for \'0' */
+#define PIPE_CRC_BUFFER_LEN (PIPE_CRC_LINE_LEN + 1)
+
+static int pipe_crc_data_count(struct intel_pipe_crc *pipe_crc)
+{
+ assert_spin_locked(&pipe_crc->lock);
+ return CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+ INTEL_PIPE_CRC_ENTRIES_NR);
+}
+
+static ssize_t
+i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
+ loff_t *pos)
+{
+ struct pipe_crc_info *info = filep->private_data;
+ struct drm_device *dev = info->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+ char buf[PIPE_CRC_BUFFER_LEN];
+ int n_entries;
+ ssize_t bytes_read;
+
+ /*
+ * Don't allow user space to provide buffers not big enough to hold
+ * a line of data.
+ */
+ if (count < PIPE_CRC_LINE_LEN)
+ return -EINVAL;
+
+ if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
+ return 0;
+
+ /* nothing to read */
+ spin_lock_irq(&pipe_crc->lock);
+ while (pipe_crc_data_count(pipe_crc) == 0) {
+ int ret;
+
+ if (filep->f_flags & O_NONBLOCK) {
+ spin_unlock_irq(&pipe_crc->lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_lock_irq(pipe_crc->wq,
+ pipe_crc_data_count(pipe_crc), pipe_crc->lock);
+ if (ret) {
+ spin_unlock_irq(&pipe_crc->lock);
+ return ret;
+ }
+ }
+
+ /* We now have one or more entries to read */
+ n_entries = count / PIPE_CRC_LINE_LEN;
+
+ bytes_read = 0;
+ while (n_entries > 0) {
+ struct intel_pipe_crc_entry *entry =
+ &pipe_crc->entries[pipe_crc->tail];
+ int ret;
+
+ if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+ INTEL_PIPE_CRC_ENTRIES_NR) < 1)
+ break;
+
+ BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
+ pipe_crc->tail = (pipe_crc->tail + 1) &
+ (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+
+ bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN,
+ "%8u %8x %8x %8x %8x %8x\n",
+ entry->frame, entry->crc[0],
+ entry->crc[1], entry->crc[2],
+ entry->crc[3], entry->crc[4]);
+
+ spin_unlock_irq(&pipe_crc->lock);
+
+ ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
+ if (ret == PIPE_CRC_LINE_LEN)
+ return -EFAULT;
+
+ user_buf += PIPE_CRC_LINE_LEN;
+ n_entries--;
+
+ spin_lock_irq(&pipe_crc->lock);
+ }
+
+ spin_unlock_irq(&pipe_crc->lock);
+
+ return bytes_read;
+}
+
+static const struct file_operations i915_pipe_crc_fops = {
+ .owner = THIS_MODULE,
+ .open = i915_pipe_crc_open,
+ .read = i915_pipe_crc_read,
+ .release = i915_pipe_crc_release,
+};
+
+static struct pipe_crc_info i915_pipe_crc_data[I915_MAX_PIPES] = {
+ {
+ .name = "i915_pipe_A_crc",
+ .pipe = PIPE_A,
+ },
+ {
+ .name = "i915_pipe_B_crc",
+ .pipe = PIPE_B,
+ },
+ {
+ .name = "i915_pipe_C_crc",
+ .pipe = PIPE_C,
+ },
+};
+
+static int i915_pipe_crc_create(struct dentry *root, struct drm_minor *minor,
+ enum pipe pipe)
+{
+ struct drm_device *dev = minor->dev;
+ struct dentry *ent;
+ struct pipe_crc_info *info = &i915_pipe_crc_data[pipe];
+
+ info->dev = dev;
+ ent = debugfs_create_file(info->name, S_IRUGO, root, info,
+ &i915_pipe_crc_fops);
+ if (!ent)
+ return -ENOMEM;
+
+ return drm_add_fake_info_node(minor, ent, info);
+}
+
+static const char * const pipe_crc_sources[] = {
+ "none",
+ "plane1",
+ "plane2",
+ "pf",
+ "pipe",
+ "TV",
+ "DP-B",
+ "DP-C",
+ "DP-D",
+ "auto",
+};
+
+static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
+{
+ BUILD_BUG_ON(ARRAY_SIZE(pipe_crc_sources) != INTEL_PIPE_CRC_SOURCE_MAX);
+ return pipe_crc_sources[source];
+}
+
+static int display_crc_ctl_show(struct seq_file *m, void *data)
+{
+ struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ int i;
+
+ for (i = 0; i < I915_MAX_PIPES; i++)
+ seq_printf(m, "%c %s\n", pipe_name(i),
+ pipe_crc_source_name(dev_priv->pipe_crc[i].source));
+
+ return 0;
+}
+
+static int display_crc_ctl_open(struct inode *inode, struct file *file)
+{
+ struct drm_device *dev = inode->i_private;
+
+ return single_open(file, display_crc_ctl_show, dev);
+}
+
+static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
+ uint32_t *val)
+{
+ 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;
+ case INTEL_PIPE_CRC_SOURCE_NONE:
+ *val = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+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;
+ struct intel_digital_port *dig_port;
+ int ret = 0;
+
+ *source = INTEL_PIPE_CRC_SOURCE_PIPE;
+
+ drm_modeset_lock_all(dev);
+ for_each_intel_encoder(dev, encoder) {
+ 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:
+ 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;
+ default:
+ break;
+ }
+ }
+ drm_modeset_unlock_all(dev);
+
+ 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)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ bool need_stable_symbols = false;
+ int ret;
+
+ if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+ 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;
+ case INTEL_PIPE_CRC_SOURCE_DP_B:
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_B_VLV;
+ need_stable_symbols = true;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_DP_C:
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_C_VLV;
+ need_stable_symbols = true;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_DP_D:
+ if (!IS_CHERRYVIEW(dev))
+ return -EINVAL;
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_DP_D_VLV;
+ need_stable_symbols = true;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_NONE:
+ *val = 0;
+ break;
+ default:
+ 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) {
+ uint32_t tmp = I915_READ(PORT_DFT2_G4X);
+
+ tmp |= DC_BALANCE_RESET_VLV;
+ switch (pipe) {
+ case PIPE_A:
+ tmp |= PIPE_A_SCRAMBLE_RESET;
+ break;
+ case PIPE_B:
+ tmp |= PIPE_B_SCRAMBLE_RESET;
+ break;
+ case PIPE_C:
+ tmp |= PIPE_C_SCRAMBLE_RESET;
+ break;
+ default:
+ return -EINVAL;
+ }
+ I915_WRITE(PORT_DFT2_G4X, tmp);
+ }
+
+ return 0;
+}
+
+static int i9xx_pipe_crc_ctl_reg(struct drm_device *dev,
+ enum pipe pipe,
+ enum intel_pipe_crc_source *source,
+ uint32_t *val)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ bool need_stable_symbols = false;
+ int ret;
+
+ if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
+ 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;
+ case INTEL_PIPE_CRC_SOURCE_TV:
+ if (!SUPPORTS_TV(dev))
+ return -EINVAL;
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_TV_PRE;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_DP_B:
+ if (!IS_G4X(dev))
+ 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))
+ 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))
+ 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:
+ 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) {
+ 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 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);
+
+ switch (pipe) {
+ case PIPE_A:
+ tmp &= ~PIPE_A_SCRAMBLE_RESET;
+ break;
+ case PIPE_B:
+ tmp &= ~PIPE_B_SCRAMBLE_RESET;
+ break;
+ case PIPE_C:
+ tmp &= ~PIPE_C_SCRAMBLE_RESET;
+ break;
+ default:
+ return;
+ }
+ 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)
+{
+ 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)
+{
+ 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;
+ case INTEL_PIPE_CRC_SOURCE_PLANE2:
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_ILK;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_PIPE:
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PIPE_ILK;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_NONE:
+ *val = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_crtc *crtc =
+ to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
+ struct intel_crtc_state *pipe_config;
+ struct drm_atomic_state *state;
+ int ret = 0;
+
+ drm_modeset_lock_all(dev);
+ state = drm_atomic_state_alloc(dev);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+ pipe_config = intel_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(pipe_config)) {
+ ret = PTR_ERR(pipe_config);
+ goto out;
+ }
+
+ pipe_config->pch_pfit.force_thru = enable;
+ if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+ pipe_config->pch_pfit.enabled != enable)
+ pipe_config->base.connectors_changed = true;
+
+ ret = drm_atomic_commit(state);
+out:
+ drm_modeset_unlock_all(dev);
+ WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+ if (ret)
+ drm_atomic_state_free(state);
+}
+
+static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
+ enum pipe pipe,
+ enum intel_pipe_crc_source *source,
+ uint32_t *val)
+{
+ 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;
+ case INTEL_PIPE_CRC_SOURCE_PLANE2:
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_PF:
+ if (IS_HASWELL(dev) && pipe == PIPE_A)
+ hsw_trans_edp_pipe_A_crc_wa(dev, true);
+
+ *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
+ break;
+ case INTEL_PIPE_CRC_SOURCE_NONE:
+ *val = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ 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)
+ return 0;
+
+ /* forbid changing the source without going back to 'none' */
+ if (pipe_crc->source && source)
+ return -EINVAL;
+
+ power_domain = POWER_DOMAIN_PIPE(pipe);
+ 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;
+ }
+
+ 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;
+
+ DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
+ pipe_name(pipe), pipe_crc_source_name(source));
+
+ entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
+ sizeof(pipe_crc->entries[0]),
+ GFP_KERNEL);
+ if (!entries) {
+ ret = -ENOMEM;
+ 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;
+ pipe_crc->head = 0;
+ pipe_crc->tail = 0;
+ spin_unlock_irq(&pipe_crc->lock);
+ }
+
+ pipe_crc->source = source;
+
+ I915_WRITE(PIPE_CRC_CTL(pipe), val);
+ POSTING_READ(PIPE_CRC_CTL(pipe));
+
+ /* real source -> none transition */
+ if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+ struct intel_pipe_crc_entry *entries;
+ struct intel_crtc *crtc =
+ to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+
+ DRM_DEBUG_DRIVER("stopping CRCs for pipe %c\n",
+ pipe_name(pipe));
+
+ drm_modeset_lock(&crtc->base.mutex, NULL);
+ if (crtc->base.state->active)
+ intel_wait_for_vblank(dev, pipe);
+ drm_modeset_unlock(&crtc->base.mutex);
+
+ spin_lock_irq(&pipe_crc->lock);
+ entries = pipe_crc->entries;
+ pipe_crc->entries = NULL;
+ pipe_crc->head = 0;
+ pipe_crc->tail = 0;
+ 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;
+
+out:
+ intel_display_power_put(dev_priv, power_domain);
+
+ return ret;
+}
+
+/*
+ * Parse pipe CRC command strings:
+ * command: wsp* object wsp+ name wsp+ source wsp*
+ * object: 'pipe'
+ * name: (A | B | C)
+ * source: (none | plane1 | plane2 | pf)
+ * wsp: (#0x20 | #0x9 | #0xA)+
+ *
+ * eg.:
+ * "pipe A plane1" -> Start CRC computations on plane1 of pipe A
+ * "pipe A none" -> Stop CRC
+ */
+static int display_crc_ctl_tokenize(char *buf, char *words[], int max_words)
+{
+ int n_words = 0;
+
+ while (*buf) {
+ char *end;
+
+ /* skip leading white space */
+ buf = skip_spaces(buf);
+ if (!*buf)
+ break; /* end of buffer */
+
+ /* find end of word */
+ for (end = buf; *end && !isspace(*end); end++)
+ ;
+
+ if (n_words == max_words) {
+ DRM_DEBUG_DRIVER("too many words, allowed <= %d\n",
+ max_words);
+ return -EINVAL; /* ran out of words[] before bytes */
+ }
+
+ if (*end)
+ *end++ = '\0';
+ words[n_words++] = buf;
+ buf = end;
+ }
+
+ return n_words;
+}
+
+enum intel_pipe_crc_object {
+ PIPE_CRC_OBJECT_PIPE,
+};
+
+static const char * const pipe_crc_objects[] = {
+ "pipe",
+};
+
+static int
+display_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *o)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(pipe_crc_objects); i++)
+ if (!strcmp(buf, pipe_crc_objects[i])) {
+ *o = i;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int display_crc_ctl_parse_pipe(const char *buf, enum pipe *pipe)
+{
+ const char name = buf[0];
+
+ if (name < 'A' || name >= pipe_name(I915_MAX_PIPES))
+ return -EINVAL;
+
+ *pipe = name - 'A';
+
+ return 0;
+}
+
+static int
+display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
+ if (!strcmp(buf, pipe_crc_sources[i])) {
+ *s = i;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int display_crc_ctl_parse(struct drm_device *dev, char *buf, size_t len)
+{
+#define N_WORDS 3
+ int n_words;
+ char *words[N_WORDS];
+ enum pipe pipe;
+ enum intel_pipe_crc_object object;
+ enum intel_pipe_crc_source source;
+
+ n_words = display_crc_ctl_tokenize(buf, words, N_WORDS);
+ if (n_words != N_WORDS) {
+ DRM_DEBUG_DRIVER("tokenize failed, a command is %d words\n",
+ N_WORDS);
+ return -EINVAL;
+ }
+
+ if (display_crc_ctl_parse_object(words[0], &object) < 0) {
+ DRM_DEBUG_DRIVER("unknown object %s\n", words[0]);
+ return -EINVAL;
+ }
+
+ if (display_crc_ctl_parse_pipe(words[1], &pipe) < 0) {
+ DRM_DEBUG_DRIVER("unknown pipe %s\n", words[1]);
+ return -EINVAL;
+ }
+
+ if (display_crc_ctl_parse_source(words[2], &source) < 0) {
+ DRM_DEBUG_DRIVER("unknown source %s\n", words[2]);
+ return -EINVAL;
+ }
+
+ return pipe_crc_set_source(dev, pipe, source);
+}
+
+static ssize_t display_crc_ctl_write(struct file *file, const char __user *ubuf,
+ size_t len, loff_t *offp)
+{
+ struct seq_file *m = file->private_data;
+ struct drm_device *dev = m->private;
+ char *tmpbuf;
+ int ret;
+
+ if (len == 0)
+ return 0;
+
+ if (len > PAGE_SIZE - 1) {
+ DRM_DEBUG_DRIVER("expected <%lu bytes into pipe crc control\n",
+ PAGE_SIZE);
+ return -E2BIG;
+ }
+
+ tmpbuf = kmalloc(len + 1, GFP_KERNEL);
+ if (!tmpbuf)
+ return -ENOMEM;
+
+ if (copy_from_user(tmpbuf, ubuf, len)) {
+ ret = -EFAULT;
+ goto out;
+ }
+ tmpbuf[len] = '\0';
+
+ ret = display_crc_ctl_parse(dev, tmpbuf, len);
+
+out:
+ kfree(tmpbuf);
+ if (ret < 0)
+ return ret;
+
+ *offp += len;
+ return len;
+}
+
+const struct file_operations i915_display_crc_ctl_fops = {
+ .owner = THIS_MODULE,
+ .open = display_crc_ctl_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+ .write = display_crc_ctl_write
+};
+
+void intel_display_crc_init(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum pipe pipe;
+
+ for_each_pipe(dev_priv, pipe) {
+ struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
+
+ pipe_crc->opened = false;
+ spin_lock_init(&pipe_crc->lock);
+ init_waitqueue_head(&pipe_crc->wq);
+ }
+}
+
+int intel_pipe_crc_create(struct drm_minor *minor)
+{
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
+ ret = i915_pipe_crc_create(minor->debugfs_root, minor, i);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+void intel_pipe_crc_cleanup(struct drm_minor *minor)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
+ struct drm_info_list *info_list =
+ (struct drm_info_list *)&i915_pipe_crc_data[i];
+
+ drm_debugfs_remove_files(info_list, 1, minor);
+ }
+}
--
2.5.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-07-22 14:10 [PATCH v3 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
@ 2016-07-22 14:10 ` Tomeu Vizoso
2016-08-03 7:06 ` Ville Syrjälä
2016-08-06 17:04 ` Daniel Stone
2016-07-22 14:10 ` [PATCH v3 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2 siblings, 2 replies; 12+ messages in thread
From: Tomeu Vizoso @ 2016-07-22 14:10 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Thierry Reding, Tomeu Vizoso,
linux-doc, David Airlie, dri-devel, Jonathan Corbet
Adds files and directories to debugfs for controlling and reading frame
CRCs, per CRTC:
dri/0/crtc-0/crc
dri/0/crtc-0/crc/control
dri/0/crtc-0/crc/data
Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
start and stop generating frame CRCs and can add entries to the output
by calling drm_crtc_add_crc_entry.
v2:
- Lots of good fixes suggested by Thierry.
- Added documentation.
- Changed the debugfs layout.
- Moved to allocate the entries circular queue once when frame
generation gets enabled for the first time.
v3:
- Use the control file just to select the source, and start and stop
capture when the data file is opened and closed, respectively.
- Make variable the number of CRC values per entry, per source.
- Allocate entries queue each time we start capturing as now there
isn't a fixed number of CRC values per entry.
- Store the frame counter in the data file as a 8-digit hex number.
- For sources that cannot provide useful frame numbers, place
XXXXXXXX in the frame field.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
Documentation/gpu/drm-uapi.rst | 6 +
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/drm_crtc.c | 12 ++
drivers/gpu/drm/drm_debugfs.c | 36 +++-
drivers/gpu/drm/drm_debugfs_crc.c | 370 ++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_internal.h | 10 ++
include/drm/drmP.h | 5 +
include/drm/drm_crtc.h | 41 +++++
include/drm/drm_debugfs_crc.h | 74 ++++++++
10 files changed, 564 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
create mode 100644 include/drm/drm_debugfs_crc.h
diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 536bf3eaadd4..33f778696ccd 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -109,3 +109,9 @@ interfaces. Especially since all hardware-acceleration interfaces to
userspace are driver specific for efficiency and other reasons these
interfaces can be rather substantial. Hence every driver has its own
chapter.
+
+Testing and validation
+======================
+
+.. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c
+ :doc: CRC ABI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e3dba6f44a79..b53b5aaaeb4d 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,8 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
drm_info.o drm_debugfs.o drm_encoder_slave.o \
drm_trace_points.o drm_global.o drm_prime.o \
drm_rect.o drm_vma_manager.o drm_flip_work.o \
- drm_modeset_lock.o drm_atomic.o drm_bridge.o
+ drm_modeset_lock.o drm_atomic.o drm_bridge.o \
+ drm_debugfs_crc.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 10b73f68c023..087345af96e7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -40,6 +40,7 @@
#include <drm/drm_modeset_lock.h>
#include <drm/drm_atomic.h>
#include <drm/drm_auth.h>
+#include <drm/drm_debugfs_crc.h>
#include "drm_crtc_internal.h"
#include "drm_internal.h"
@@ -738,6 +739,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
if (cursor)
cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
+#ifdef CONFIG_DEBUG_FS
+ spin_lock_init(&crtc->crc.lock);
+ init_waitqueue_head(&crtc->crc.wq);
+ crtc->crc.source = kstrdup("auto", GFP_KERNEL);
+#endif
+
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
drm_object_attach_property(&crtc->base, config->prop_active, 0);
drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
@@ -764,6 +771,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
* the indices on the drm_crtc after us in the crtc_list.
*/
+#ifdef CONFIG_DEBUG_FS
+ drm_debugfs_crtc_remove(crtc);
+ kfree(crtc->crc.source);
+#endif
+
kfree(crtc->gamma_store);
crtc->gamma_store = NULL;
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index fa10cef2ba37..73530cbf1316 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -415,5 +415,39 @@ void drm_debugfs_connector_remove(struct drm_connector *connector)
connector->debugfs_entry = NULL;
}
-#endif /* CONFIG_DEBUG_FS */
+int drm_debugfs_crtc_add(struct drm_crtc *crtc)
+{
+ struct drm_minor *minor = crtc->dev->primary;
+ struct dentry *root;
+ char *name;
+
+ name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
+ if (!name)
+ return -ENOMEM;
+ root = debugfs_create_dir(name, minor->debugfs_root);
+ kfree(name);
+ if (!root)
+ return -ENOMEM;
+
+ crtc->debugfs_entry = root;
+
+ if (drm_debugfs_crtc_crc_add(crtc))
+ goto error;
+
+ return 0;
+
+error:
+ debugfs_remove_recursive(crtc->debugfs_entry);
+ crtc->debugfs_entry = NULL;
+ return -ENOMEM;
+}
+
+void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
+{
+ debugfs_remove_recursive(crtc->debugfs_entry);
+
+ crtc->debugfs_entry = NULL;
+}
+
+#endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
new file mode 100644
index 000000000000..5ef071437952
--- /dev/null
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -0,0 +1,370 @@
+/*
+ * Copyright © 2008 Intel Corporation
+ * Copyright © 2016 Collabora Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Eric Anholt <eric@anholt.net>
+ * Keith Packard <keithp@keithp.com>
+ *
+ */
+
+#include <linux/circ_buf.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
+#include <drm/drmP.h>
+
+/**
+ * DOC: CRC ABI
+ *
+ * DRM device drivers can provide to userspace CRC information of each frame as
+ * it reached a given hardware component (a "source").
+ *
+ * Userspace can control generation of CRCs in a given CRTC by writing to the
+ * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
+ * Accepted values are source names (which are driver-specific) and the "none"
+ * and "auto" keywords. "none" will disable CRC generation and "auto" will let
+ * the driver select a default source of frame CRCs for this CRTC.
+ *
+ * Once frame CRC generation is enabled, userspace can capture them by reading
+ * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
+ * number in the first field and then a number of unsigned integer fields
+ * containing the CRC data. Fields are separated by a single space and the number
+ * of CRC fields is source-specific.
+ *
+ * Note that though in some cases the CRC is computed in a specified way and on
+ * the frame contents as supplied by userspace (eDP 1.3), in general the CRC
+ * computation is performed in an unspecified way and on frame contents that have
+ * been already processed in also an unspecified way and thus userspace cannot
+ * rely on being able to generate matching CRC values for the frame contents that
+ * it submits. In this general case, the maximum userspace can do is to compare
+ * the reported CRCs of frames that should have the same contents.
+ */
+#if defined(CONFIG_DEBUG_FS)
+
+static int crc_control_show(struct seq_file *m, void *data)
+{
+ struct drm_crtc *crtc = m->private;
+
+ seq_printf(m, "%s\n", crtc->crc.source);
+
+ return 0;
+}
+
+static int crc_control_open(struct inode *inode, struct file *file)
+{
+ struct drm_crtc *crtc = inode->i_private;
+
+ return single_open(file, crc_control_show, crtc);
+}
+
+static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
+ size_t len, loff_t *offp)
+{
+ struct seq_file *m = file->private_data;
+ struct drm_crtc *crtc = m->private;
+ struct drm_crtc_crc *crc = &crtc->crc;
+ char *source;
+
+ if (len == 0)
+ return 0;
+
+ if (len > PAGE_SIZE - 1) {
+ DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
+ PAGE_SIZE);
+ return -E2BIG;
+ }
+
+ source = kmalloc(len + 1, GFP_KERNEL);
+ if (!source)
+ return -ENOMEM;
+
+ if (copy_from_user(source, ubuf, len)) {
+ kfree(source);
+ return -EFAULT;
+ }
+
+ if (source[len - 1] == '\n')
+ source[len - 1] = '\0';
+ else
+ source[len] = '\0';
+
+ spin_lock_irq(&crc->lock);
+
+ if (crc->opened) {
+ kfree(source);
+ return -EBUSY;
+ }
+
+ kfree(crc->source);
+ crc->source = source;
+
+ spin_unlock_irq(&crc->lock);
+
+ *offp += len;
+ return len;
+}
+
+const struct file_operations drm_crtc_crc_control_fops = {
+ .owner = THIS_MODULE,
+ .open = crc_control_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+ .write = crc_control_write
+};
+
+static int crtc_crc_open(struct inode *inode, struct file *filep)
+{
+ struct drm_crtc *crtc = inode->i_private;
+ struct drm_crtc_crc *crc = &crtc->crc;
+ struct drm_crtc_crc_entry *entries = NULL;
+ size_t entry_size, values_cnt;
+ int ret;
+
+ if (crc->opened)
+ return -EBUSY;
+
+ ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
+ if (ret)
+ return ret;
+
+ if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
+ ret = -EINVAL;
+ goto err_disable;
+ }
+
+ if (WARN_ON(values_cnt == 0)) {
+ ret = -EINVAL;
+ goto err_disable;
+ }
+
+ entry_size = sizeof(*crc->entries) + sizeof(uint32_t) * values_cnt;
+ entries = kcalloc(DRM_CRC_ENTRIES_NR, entry_size, GFP_KERNEL);
+ if (!entries) {
+ ret = -ENOMEM;
+ goto err_disable;
+ }
+
+ spin_lock_irq(&crc->lock);
+ crc->entries = entries;
+ crc->values_cnt = values_cnt;
+ crc->opened = true;
+ spin_unlock_irq(&crc->lock);
+
+ return 0;
+
+err_disable:
+ crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+ return ret;
+}
+
+static int crtc_crc_release(struct inode *inode, struct file *filep)
+{
+ struct drm_crtc *crtc = filep->f_inode->i_private;
+ struct drm_crtc_crc *crc = &crtc->crc;
+ size_t values_cnt;
+
+ crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+
+ spin_lock_irq(&crc->lock);
+ kfree(crc->entries);
+ crc->entries = NULL;
+ crc->head = 0;
+ crc->tail = 0;
+ crc->values_cnt = 0;
+ crc->opened = false;
+ spin_unlock_irq(&crc->lock);
+
+ 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);
+}
+
+static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
+ int index)
+{
+ void *p = crc->entries;
+ size_t entry_size = (sizeof(*crc->entries) +
+ sizeof(*crc->entries[0].crcs) * crc->values_cnt);
+
+ return p + entry_size * index;
+}
+
+#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
+
+static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
+ size_t count, loff_t *pos)
+{
+ struct drm_crtc *crtc = filep->f_inode->i_private;
+ struct drm_crtc_crc *crc = &crtc->crc;
+ struct drm_crtc_crc_entry *entry;
+ char buf[MAX_LINE_LEN];
+ int ret, i;
+
+ spin_lock_irq(&crc->lock);
+
+ if (!crc->source) {
+ spin_unlock_irq(&crc->lock);
+ return 0;
+ }
+
+ /* Nothing to read? */
+ while (crtc_crc_data_count(crc) == 0) {
+ if (filep->f_flags & O_NONBLOCK) {
+ spin_unlock_irq(&crc->lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_lock_irq(crc->wq,
+ crtc_crc_data_count(crc),
+ crc->lock);
+ if (ret) {
+ spin_unlock_irq(&crc->lock);
+ return ret;
+ }
+ }
+
+ /* We know we have an entry to be read */
+ entry = crtc_get_crc_entry(crc, crc->tail);
+
+ /*
+ * 1 frame field of 8 chars plus a number of CRC fields of 8
+ * chars each, space separated and with a newline at the end.
+ */
+ if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
+ spin_unlock_irq(&crc->lock);
+ return -EINVAL;
+ }
+
+ BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
+ crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
+
+ spin_unlock_irq(&crc->lock);
+
+ if (entry->has_frame_counter)
+ snprintf(buf, 9, "%08x", entry->frame);
+ else
+ snprintf(buf, 9, "XXXXXXXX");
+
+ for (i = 0; i < crc->values_cnt; i++)
+ snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
+ snprintf(buf + strlen(buf), 2, "\n");
+
+ if (copy_to_user(user_buf, buf, strlen(buf) + 1))
+ return -EFAULT;
+
+ return strlen(buf) + 1;
+}
+
+const struct file_operations drm_crtc_crc_data_fops = {
+ .owner = THIS_MODULE,
+ .open = crtc_crc_open,
+ .read = crtc_crc_read,
+ .release = crtc_crc_release,
+};
+
+/**
+ * drm_debugfs_crtc_crc_add - Add files to debugfs for capture of frame CRCs
+ * @crtc: CRTC to whom the frames will belong
+ *
+ * Adds files to debugfs directory that allows userspace to control the
+ * generation of frame CRCs and to read them.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
+{
+ struct dentry *crc_ent, *ent;
+
+ if (!crtc->funcs->set_crc_source)
+ return 0;
+
+ crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
+ if (!crc_ent)
+ return -ENOMEM;
+
+ ent = debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
+ &drm_crtc_crc_control_fops);
+ if (!ent)
+ goto error;
+
+ ent = debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
+ &drm_crtc_crc_data_fops);
+ if (!ent)
+ goto error;
+
+ return 0;
+
+error:
+ debugfs_remove_recursive(crc_ent);
+
+ return -ENOMEM;
+}
+
+/**
+ * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
+ * @crtc: CRTC to which the frame belongs
+ * @has_frame: whether this entry has a frame number to go with
+ * @frame: number of the frame these CRCs are about
+ * @crcs: array of CRC values, with length matching #drm_crtc_crc.values_cnt
+ *
+ * For each frame, the driver polls the source of CRCs for new data and calls
+ * this function to add them to the buffer from where userspace reads.
+ */
+int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
+ uint32_t frame, uint32_t *crcs)
+{
+ struct drm_crtc_crc *crc = &crtc->crc;
+ struct drm_crtc_crc_entry *entry;
+ int head, tail;
+
+ assert_spin_locked(&crc->lock);
+
+ /* Caller may not have noticed yet that userspace has stopped reading */
+ if (!crc->opened)
+ return -EINVAL;
+
+ head = crc->head;
+ tail = crc->tail;
+
+ if (CIRC_SPACE(head, tail, DRM_CRC_ENTRIES_NR) < 1) {
+ DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
+ return -ENOBUFS;
+ }
+
+ entry = crtc_get_crc_entry(crc, head);
+ entry->frame = frame;
+ entry->has_frame_counter = has_frame;
+ memcpy(&entry->crcs, crcs, sizeof(*crcs) * crc->values_cnt);
+
+ head = (head + 1) & (DRM_CRC_ENTRIES_NR - 1);
+ crc->head = head;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(drm_crtc_add_crc_entry);
+#endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index aead9ffcbe29..a02406f51d98 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -192,6 +192,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
static int drm_minor_register(struct drm_device *dev, unsigned int type)
{
struct drm_minor *minor;
+ struct drm_crtc *crtc;
unsigned long flags;
int ret;
@@ -207,6 +208,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
return ret;
}
+ if (type == DRM_MINOR_LEGACY) {
+ drm_for_each_crtc(crtc, dev) {
+ ret = drm_debugfs_crtc_add(crtc);
+ if (ret)
+ goto err_debugfs;
+ }
+ }
+
ret = device_add(minor->kdev);
if (ret)
goto err_debugfs;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b86dc9b921a5..99ce6d4f2916 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -97,6 +97,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
int drm_debugfs_cleanup(struct drm_minor *minor);
int drm_debugfs_connector_add(struct drm_connector *connector);
void drm_debugfs_connector_remove(struct drm_connector *connector);
+int drm_debugfs_crtc_add(struct drm_crtc *crtc);
+void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
#else
static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
struct dentry *root)
@@ -116,4 +118,12 @@ static inline int drm_debugfs_connector_add(struct drm_connector *connector)
static inline void drm_debugfs_connector_remove(struct drm_connector *connector)
{
}
+
+static inline int drm_debugfs_crtc_add(struct drm_crtc *crtc)
+{
+ return 0;
+}
+static inline void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
+{
+}
#endif
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cf918e3e6afb..8a0b235ccc39 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1111,6 +1111,11 @@ static __inline__ bool drm_can_sleep(void)
return true;
}
+#if defined(CONFIG_DEBUG_FS)
+extern const struct file_operations drm_crc_control_fops;
+extern const struct file_operations drm_crtc_crc_fops;
+#endif
+
/* helper for handling conditionals in various for_each macros */
#define for_each_if(condition) if (!(condition)) {} else
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 781695c74528..fcc632940dce 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -35,6 +35,7 @@
#include <uapi/drm/drm_mode.h>
#include <uapi/drm/drm_fourcc.h>
#include <drm/drm_modeset_lock.h>
+#include <drm/drm_debugfs_crc.h>
struct drm_device;
struct drm_mode_set;
@@ -731,6 +732,30 @@ struct drm_crtc_funcs {
* before data structures are torndown.
*/
void (*early_unregister)(struct drm_crtc *crtc);
+
+ /**
+ * @set_crc_source:
+ *
+ * Changes the source of CRC checksums of frames at the request of
+ * userspace, typically for testing purposes. The sources available are
+ * specific of each driver and a %NULL value indicates that CRC
+ * generation is to be switched off.
+ *
+ * When CRC generation is enabled, the driver should call
+ * drm_crtc_add_crc_entry() at each frame, providing any information
+ * that characterizes the frame contents in the crcN arguments, as
+ * provided from the configured source. Drivers should accept a "auto"
+ * source name that will select a default source for this CRTC.
+ *
+ * This callback is optional if the driver does not support any CRC
+ * generation functionality.
+ *
+ * RETURNS:
+ *
+ * 0 on success or a negative error code on failure.
+ */
+ int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
+ size_t *values_cnt);
};
/**
@@ -844,6 +869,22 @@ struct drm_crtc {
* context.
*/
struct drm_modeset_acquire_ctx *acquire_ctx;
+
+#ifdef CONFIG_DEBUG_FS
+ /**
+ * @debugfs_entry:
+ *
+ * Debugfs directory for this CRTC.
+ */
+ struct dentry *debugfs_entry;
+
+ /**
+ * @crc:
+ *
+ * Configuration settings of CRC capture.
+ */
+ struct drm_crtc_crc crc;
+#endif
};
/**
diff --git a/include/drm/drm_debugfs_crc.h b/include/drm/drm_debugfs_crc.h
new file mode 100644
index 000000000000..a341fc9becad
--- /dev/null
+++ b/include/drm/drm_debugfs_crc.h
@@ -0,0 +1,74 @@
+/*
+ * Copyright © 2016 Collabora Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+#ifndef __DRM_DEBUGFS_CRC_H__
+#define __DRM_DEBUGFS_CRC_H__
+
+/**
+ * struct drm_crtc_crc_entry - entry describing a frame's content
+ * @frame: number of the frame this CRC is about
+ * @crc: array of values that characterize the frame
+ */
+struct drm_crtc_crc_entry {
+ bool has_frame_counter;
+ uint32_t frame;
+ uint32_t crcs[0];
+};
+
+#define DRM_MAX_CRC_NR 10
+#define DRM_CRC_ENTRIES_NR 128
+/**
+ * struct drm_crtc_crc - data supporting CRC capture on a given CRTC
+ * @lock: protects the fields in this struct
+ * @source: name of the currently configured source of CRCs
+ * @opened: whether userspace has opened the data file for reading
+ * @entries: array of entries, with size of %DRM_CRTC_CRC_ENTRIES_NR
+ * @head: head of circular queue
+ * @tail: tail of circular queue
+ * @wq: workqueue used to synchronize reading and writing
+ */
+struct drm_crtc_crc {
+ spinlock_t lock;
+ const char *source;
+ bool opened;
+ struct drm_crtc_crc_entry *entries;
+ int head, tail;
+ size_t values_cnt;
+ wait_queue_head_t wq;
+};
+
+#if defined(CONFIG_DEBUG_FS)
+int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
+int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
+ uint32_t frame, uint32_t *crcs);
+#else
+static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
+{
+ return 0;
+}
+static inline int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
+ uint32_t frame, uint32_t *crcs)
+{
+ return -EINVAL;
+}
+#endif /* defined(CONFIG_DEBUG_FS) */
+
+#endif /* __DRM_DEBUGFS_CRC_H__ */
--
2.5.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/3] drm/i915: Use new CRC debugfs API
2016-07-22 14:10 [PATCH v3 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
@ 2016-07-22 14:10 ` Tomeu Vizoso
2016-08-03 8:01 ` Daniel Vetter
2 siblings, 1 reply; 12+ messages in thread
From: Tomeu Vizoso @ 2016-07-22 14:10 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Thierry Reding, Tomeu Vizoso,
Jani Nikula, 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.
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.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++-------
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 124 ++++++++++++++++++++++++----------
4 files changed, 133 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b77d808b71cd..f2726171f7c8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1492,41 +1492,58 @@ 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;
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 {
+ 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;
+ ret = drm_crtc_add_crc_entry(crtc, true,
+ drm_accurate_vblank_count(crtc),
+ 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 111b350d1d7e..d91a2f779fb1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14019,6 +14019,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 5d06cb2425a1..4302cf2cfb04 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1782,6 +1782,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 4e6c28ce8ecf..d2f06a1a919a 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 == INTEL_PIPE_CRC_SOURCE_NONE) {
+ 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,10 +718,11 @@ 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) {
@@ -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,31 @@ 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;
+ 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);
+
+ *values_cnt = 5;
+
+ return ret;
+}
--
2.5.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-07-22 14:10 ` [PATCH v3 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
@ 2016-08-03 7:06 ` Ville Syrjälä
2016-08-03 7:57 ` Daniel Vetter
2016-08-05 10:46 ` Tomeu Vizoso
2016-08-06 17:04 ` Daniel Stone
1 sibling, 2 replies; 12+ messages in thread
From: Ville Syrjälä @ 2016-08-03 7:06 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, linux-doc, Jonathan Corbet, dri-devel,
Daniel Vetter, Emil Velikov
On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:
> Adds files and directories to debugfs for controlling and reading frame
> CRCs, per CRTC:
>
> dri/0/crtc-0/crc
> dri/0/crtc-0/crc/control
> dri/0/crtc-0/crc/data
>
> Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
> start and stop generating frame CRCs and can add entries to the output
> by calling drm_crtc_add_crc_entry.
>
> v2:
> - Lots of good fixes suggested by Thierry.
> - Added documentation.
> - Changed the debugfs layout.
> - Moved to allocate the entries circular queue once when frame
> generation gets enabled for the first time.
> v3:
> - Use the control file just to select the source, and start and stop
> capture when the data file is opened and closed, respectively.
> - Make variable the number of CRC values per entry, per source.
> - Allocate entries queue each time we start capturing as now there
> isn't a fixed number of CRC values per entry.
> - Store the frame counter in the data file as a 8-digit hex number.
> - For sources that cannot provide useful frame numbers, place
> XXXXXXXX in the frame field.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Documentation/gpu/drm-uapi.rst | 6 +
> drivers/gpu/drm/Makefile | 3 +-
> drivers/gpu/drm/drm_crtc.c | 12 ++
> drivers/gpu/drm/drm_debugfs.c | 36 +++-
> drivers/gpu/drm/drm_debugfs_crc.c | 370 ++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_drv.c | 9 +
> drivers/gpu/drm/drm_internal.h | 10 ++
> include/drm/drmP.h | 5 +
> include/drm/drm_crtc.h | 41 +++++
> include/drm/drm_debugfs_crc.h | 74 ++++++++
> 10 files changed, 564 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
> create mode 100644 include/drm/drm_debugfs_crc.h
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 536bf3eaadd4..33f778696ccd 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -109,3 +109,9 @@ interfaces. Especially since all hardware-acceleration interfaces to
> userspace are driver specific for efficiency and other reasons these
> interfaces can be rather substantial. Hence every driver has its own
> chapter.
> +
> +Testing and validation
> +======================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c
> + :doc: CRC ABI
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e3dba6f44a79..b53b5aaaeb4d 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -12,7 +12,8 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> drm_info.o drm_debugfs.o drm_encoder_slave.o \
> drm_trace_points.o drm_global.o drm_prime.o \
> drm_rect.o drm_vma_manager.o drm_flip_work.o \
> - drm_modeset_lock.o drm_atomic.o drm_bridge.o
> + drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> + drm_debugfs_crc.o
>
> drm-$(CONFIG_COMPAT) += drm_ioc32.o
> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 10b73f68c023..087345af96e7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -40,6 +40,7 @@
> #include <drm/drm_modeset_lock.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_auth.h>
> +#include <drm/drm_debugfs_crc.h>
>
> #include "drm_crtc_internal.h"
> #include "drm_internal.h"
> @@ -738,6 +739,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> if (cursor)
> cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>
> +#ifdef CONFIG_DEBUG_FS
> + spin_lock_init(&crtc->crc.lock);
> + init_waitqueue_head(&crtc->crc.wq);
> + crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> +#endif
> +
> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> drm_object_attach_property(&crtc->base, config->prop_active, 0);
> drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> @@ -764,6 +771,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
> * the indices on the drm_crtc after us in the crtc_list.
> */
>
> +#ifdef CONFIG_DEBUG_FS
> + drm_debugfs_crtc_remove(crtc);
> + kfree(crtc->crc.source);
> +#endif
> +
> kfree(crtc->gamma_store);
> crtc->gamma_store = NULL;
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index fa10cef2ba37..73530cbf1316 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -415,5 +415,39 @@ void drm_debugfs_connector_remove(struct drm_connector *connector)
> connector->debugfs_entry = NULL;
> }
>
> -#endif /* CONFIG_DEBUG_FS */
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> + struct drm_minor *minor = crtc->dev->primary;
> + struct dentry *root;
> + char *name;
> +
> + name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
> + if (!name)
> + return -ENOMEM;
>
> + root = debugfs_create_dir(name, minor->debugfs_root);
> + kfree(name);
> + if (!root)
> + return -ENOMEM;
> +
> + crtc->debugfs_entry = root;
> +
> + if (drm_debugfs_crtc_crc_add(crtc))
> + goto error;
> +
> + return 0;
> +
> +error:
> + debugfs_remove_recursive(crtc->debugfs_entry);
> + crtc->debugfs_entry = NULL;
> + return -ENOMEM;
> +}
> +
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> + debugfs_remove_recursive(crtc->debugfs_entry);
> +
> + crtc->debugfs_entry = NULL;
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> new file mode 100644
> index 000000000000..5ef071437952
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -0,0 +1,370 @@
> +/*
> + * Copyright © 2008 Intel Corporation
> + * Copyright © 2016 Collabora Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Eric Anholt <eric@anholt.net>
> + * Keith Packard <keithp@keithp.com>
> + *
> + */
> +
> +#include <linux/circ_buf.h>
> +#include <linux/ctype.h>
> +#include <linux/debugfs.h>
> +#include <drm/drmP.h>
> +
> +/**
> + * DOC: CRC ABI
> + *
> + * DRM device drivers can provide to userspace CRC information of each frame as
> + * it reached a given hardware component (a "source").
> + *
> + * Userspace can control generation of CRCs in a given CRTC by writing to the
> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
> + * Accepted values are source names (which are driver-specific) and the "none"
> + * and "auto" keywords. "none" will disable CRC generation and "auto" will let
> + * the driver select a default source of frame CRCs for this CRTC.
> + *
> + * Once frame CRC generation is enabled, userspace can capture them by reading
> + * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
> + * number in the first field and then a number of unsigned integer fields
> + * containing the CRC data. Fields are separated by a single space and the number
> + * of CRC fields is source-specific.
> + *
> + * Note that though in some cases the CRC is computed in a specified way and on
> + * the frame contents as supplied by userspace (eDP 1.3), in general the CRC
> + * computation is performed in an unspecified way and on frame contents that have
> + * been already processed in also an unspecified way and thus userspace cannot
> + * rely on being able to generate matching CRC values for the frame contents that
> + * it submits. In this general case, the maximum userspace can do is to compare
> + * the reported CRCs of frames that should have the same contents.
> + */
> +#if defined(CONFIG_DEBUG_FS)
> +
> +static int crc_control_show(struct seq_file *m, void *data)
> +{
> + struct drm_crtc *crtc = m->private;
> +
> + seq_printf(m, "%s\n", crtc->crc.source);
> +
> + return 0;
> +}
> +
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> + struct drm_crtc *crtc = inode->i_private;
> +
> + return single_open(file, crc_control_show, crtc);
> +}
> +
> +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
> + size_t len, loff_t *offp)
> +{
> + struct seq_file *m = file->private_data;
> + struct drm_crtc *crtc = m->private;
> + struct drm_crtc_crc *crc = &crtc->crc;
> + char *source;
> +
> + if (len == 0)
> + return 0;
> +
> + if (len > PAGE_SIZE - 1) {
> + DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
> + PAGE_SIZE);
> + return -E2BIG;
> + }
> +
> + source = kmalloc(len + 1, GFP_KERNEL);
> + if (!source)
> + return -ENOMEM;
> +
> + if (copy_from_user(source, ubuf, len)) {
> + kfree(source);
> + return -EFAULT;
> + }
memdup_user_nul() ?
> +
> + if (source[len - 1] == '\n')
> + source[len - 1] = '\0';
> + else
> + source[len] = '\0';
> +
> + spin_lock_irq(&crc->lock);
> +
> + if (crc->opened) {
> + kfree(source);
> + return -EBUSY;
> + }
Why not just start the thing here?
> +
> + kfree(crc->source);
> + crc->source = source;
> +
> + spin_unlock_irq(&crc->lock);
> +
> + *offp += len;
> + return len;
> +}
> +
> +const struct file_operations drm_crtc_crc_control_fops = {
> + .owner = THIS_MODULE,
> + .open = crc_control_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .write = crc_control_write
> +};
> +
> +static int crtc_crc_open(struct inode *inode, struct file *filep)
> +{
> + struct drm_crtc *crtc = inode->i_private;
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entries = NULL;
> + size_t entry_size, values_cnt;
> + int ret;
> +
> + if (crc->opened)
> + return -EBUSY;
> +
> + ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> + if (ret)
> + return ret;
> +
> + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> + ret = -EINVAL;
> + goto err_disable;
> + }
> +
> + if (WARN_ON(values_cnt == 0)) {
> + ret = -EINVAL;
> + goto err_disable;
> + }
> +
> + entry_size = sizeof(*crc->entries) + sizeof(uint32_t) * values_cnt;
> + entries = kcalloc(DRM_CRC_ENTRIES_NR, entry_size, GFP_KERNEL);
> + if (!entries) {
> + ret = -ENOMEM;
> + goto err_disable;
> + }
> +
> + spin_lock_irq(&crc->lock);
> + crc->entries = entries;
> + crc->values_cnt = values_cnt;
> + crc->opened = true;
> + spin_unlock_irq(&crc->lock);
> +
> + return 0;
> +
> +err_disable:
> + crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> + return ret;
> +}
> +
> +static int crtc_crc_release(struct inode *inode, struct file *filep)
> +{
> + struct drm_crtc *crtc = filep->f_inode->i_private;
> + struct drm_crtc_crc *crc = &crtc->crc;
> + size_t values_cnt;
> +
> + crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +
> + spin_lock_irq(&crc->lock);
> + kfree(crc->entries);
> + crc->entries = NULL;
> + crc->head = 0;
> + crc->tail = 0;
> + crc->values_cnt = 0;
> + crc->opened = false;
> + spin_unlock_irq(&crc->lock);
> +
> + 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);
> +}
> +
> +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
> + int index)
> +{
> + void *p = crc->entries;
> + size_t entry_size = (sizeof(*crc->entries) +
> + sizeof(*crc->entries[0].crcs) * crc->values_cnt);
This computation is duplicated also in crtc_crc_open(). could use a
common helper to do it.
Shame the language doesn't have a way to deal with arrays of variable
sized arrays in a nice way.
> +
> + return p + entry_size * index;
> +}
> +
> +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
> +
> +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> + size_t count, loff_t *pos)
> +{
> + struct drm_crtc *crtc = filep->f_inode->i_private;
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entry;
> + char buf[MAX_LINE_LEN];
> + int ret, i;
> +
> + spin_lock_irq(&crc->lock);
> +
> + if (!crc->source) {
> + spin_unlock_irq(&crc->lock);
> + return 0;
> + }
> +
> + /* Nothing to read? */
> + while (crtc_crc_data_count(crc) == 0) {
> + if (filep->f_flags & O_NONBLOCK) {
> + spin_unlock_irq(&crc->lock);
> + return -EAGAIN;
> + }
> +
> + ret = wait_event_interruptible_lock_irq(crc->wq,
> + crtc_crc_data_count(crc),
> + crc->lock);
> + if (ret) {
> + spin_unlock_irq(&crc->lock);
> + return ret;
> + }
> + }
> +
> + /* We know we have an entry to be read */
> + entry = crtc_get_crc_entry(crc, crc->tail);
> +
> + /*
> + * 1 frame field of 8 chars plus a number of CRC fields of 8
> + * chars each, space separated and with a newline at the end.
> + */
> + if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
crc->values_cnt or DRM_MAX_CRC_NR as an argument.
> + spin_unlock_irq(&crc->lock);
> + return -EINVAL;
> + }
> +
> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
> + crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> +
> + spin_unlock_irq(&crc->lock);
> +
> + if (entry->has_frame_counter)
> + snprintf(buf, 9, "%08x", entry->frame);
> + else
> + snprintf(buf, 9, "XXXXXXXX");
Should we add "0x" prefix to all these numbers to make it clear that
they're in fact hex?
> +
> + for (i = 0; i < crc->values_cnt; i++)
> + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
The 'n' in snprintf() here seems pointless. As does the strlen().
> + snprintf(buf + strlen(buf), 2, "\n");
> +
> + if (copy_to_user(user_buf, buf, strlen(buf) + 1))
> + return -EFAULT;
> +
> + return strlen(buf) + 1;
More strlen()s that shouldn't be needed.
> +}
> +
> +const struct file_operations drm_crtc_crc_data_fops = {
> + .owner = THIS_MODULE,
> + .open = crtc_crc_open,
> + .read = crtc_crc_read,
> + .release = crtc_crc_release,
> +};
> +
> +/**
> + * drm_debugfs_crtc_crc_add - Add files to debugfs for capture of frame CRCs
> + * @crtc: CRTC to whom the frames will belong
> + *
> + * Adds files to debugfs directory that allows userspace to control the
> + * generation of frame CRCs and to read them.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +{
> + struct dentry *crc_ent, *ent;
> +
> + if (!crtc->funcs->set_crc_source)
> + return 0;
> +
> + crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> + if (!crc_ent)
> + return -ENOMEM;
> +
> + ent = debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
> + &drm_crtc_crc_control_fops);
> + if (!ent)
> + goto error;
> +
> + ent = debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
> + &drm_crtc_crc_data_fops);
> + if (!ent)
> + goto error;
> +
> + return 0;
> +
> +error:
> + debugfs_remove_recursive(crc_ent);
> +
> + return -ENOMEM;
> +}
> +
> +/**
> + * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
> + * @crtc: CRTC to which the frame belongs
> + * @has_frame: whether this entry has a frame number to go with
> + * @frame: number of the frame these CRCs are about
> + * @crcs: array of CRC values, with length matching #drm_crtc_crc.values_cnt
> + *
> + * For each frame, the driver polls the source of CRCs for new data and calls
> + * this function to add them to the buffer from where userspace reads.
> + */
> +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> + uint32_t frame, uint32_t *crcs)
> +{
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entry;
> + int head, tail;
> +
> + assert_spin_locked(&crc->lock);
> +
> + /* Caller may not have noticed yet that userspace has stopped reading */
> + if (!crc->opened)
> + return -EINVAL;
> +
> + head = crc->head;
> + tail = crc->tail;
> +
> + if (CIRC_SPACE(head, tail, DRM_CRC_ENTRIES_NR) < 1) {
> + DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> + return -ENOBUFS;
> + }
> +
> + entry = crtc_get_crc_entry(crc, head);
> + entry->frame = frame;
> + entry->has_frame_counter = has_frame;
> + memcpy(&entry->crcs, crcs, sizeof(*crcs) * crc->values_cnt);
> +
> + head = (head + 1) & (DRM_CRC_ENTRIES_NR - 1);
> + crc->head = head;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_crtc_add_crc_entry);
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index aead9ffcbe29..a02406f51d98 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -192,6 +192,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
> static int drm_minor_register(struct drm_device *dev, unsigned int type)
> {
> struct drm_minor *minor;
> + struct drm_crtc *crtc;
> unsigned long flags;
> int ret;
>
> @@ -207,6 +208,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
> return ret;
> }
>
> + if (type == DRM_MINOR_LEGACY) {
> + drm_for_each_crtc(crtc, dev) {
> + ret = drm_debugfs_crtc_add(crtc);
> + if (ret)
> + goto err_debugfs;
> + }
> + }
> +
> ret = device_add(minor->kdev);
> if (ret)
> goto err_debugfs;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b86dc9b921a5..99ce6d4f2916 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -97,6 +97,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> int drm_debugfs_cleanup(struct drm_minor *minor);
> int drm_debugfs_connector_add(struct drm_connector *connector);
> void drm_debugfs_connector_remove(struct drm_connector *connector);
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc);
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
> #else
> static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> struct dentry *root)
> @@ -116,4 +118,12 @@ static inline int drm_debugfs_connector_add(struct drm_connector *connector)
> static inline void drm_debugfs_connector_remove(struct drm_connector *connector)
> {
> }
> +
> +static inline int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> + return 0;
> +}
> +static inline void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> +}
> #endif
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index cf918e3e6afb..8a0b235ccc39 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1111,6 +1111,11 @@ static __inline__ bool drm_can_sleep(void)
> return true;
> }
>
> +#if defined(CONFIG_DEBUG_FS)
> +extern const struct file_operations drm_crc_control_fops;
> +extern const struct file_operations drm_crtc_crc_fops;
> +#endif
> +
> /* helper for handling conditionals in various for_each macros */
> #define for_each_if(condition) if (!(condition)) {} else
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 781695c74528..fcc632940dce 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -35,6 +35,7 @@
> #include <uapi/drm/drm_mode.h>
> #include <uapi/drm/drm_fourcc.h>
> #include <drm/drm_modeset_lock.h>
> +#include <drm/drm_debugfs_crc.h>
>
> struct drm_device;
> struct drm_mode_set;
> @@ -731,6 +732,30 @@ struct drm_crtc_funcs {
> * before data structures are torndown.
> */
> void (*early_unregister)(struct drm_crtc *crtc);
> +
> + /**
> + * @set_crc_source:
> + *
> + * Changes the source of CRC checksums of frames at the request of
> + * userspace, typically for testing purposes. The sources available are
> + * specific of each driver and a %NULL value indicates that CRC
> + * generation is to be switched off.
> + *
> + * When CRC generation is enabled, the driver should call
> + * drm_crtc_add_crc_entry() at each frame, providing any information
> + * that characterizes the frame contents in the crcN arguments, as
> + * provided from the configured source. Drivers should accept a "auto"
> + * source name that will select a default source for this CRTC.
> + *
> + * This callback is optional if the driver does not support any CRC
> + * generation functionality.
> + *
> + * RETURNS:
> + *
> + * 0 on success or a negative error code on failure.
> + */
> + int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> + size_t *values_cnt);
> };
>
> /**
> @@ -844,6 +869,22 @@ struct drm_crtc {
> * context.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +#ifdef CONFIG_DEBUG_FS
> + /**
> + * @debugfs_entry:
> + *
> + * Debugfs directory for this CRTC.
> + */
> + struct dentry *debugfs_entry;
> +
> + /**
> + * @crc:
> + *
> + * Configuration settings of CRC capture.
> + */
> + struct drm_crtc_crc crc;
> +#endif
> };
>
> /**
> diff --git a/include/drm/drm_debugfs_crc.h b/include/drm/drm_debugfs_crc.h
> new file mode 100644
> index 000000000000..a341fc9becad
> --- /dev/null
> +++ b/include/drm/drm_debugfs_crc.h
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright © 2016 Collabora Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +#ifndef __DRM_DEBUGFS_CRC_H__
> +#define __DRM_DEBUGFS_CRC_H__
> +
> +/**
> + * struct drm_crtc_crc_entry - entry describing a frame's content
> + * @frame: number of the frame this CRC is about
> + * @crc: array of values that characterize the frame
> + */
> +struct drm_crtc_crc_entry {
> + bool has_frame_counter;
> + uint32_t frame;
> + uint32_t crcs[0];
> +};
> +
> +#define DRM_MAX_CRC_NR 10
> +#define DRM_CRC_ENTRIES_NR 128
> +/**
> + * struct drm_crtc_crc - data supporting CRC capture on a given CRTC
> + * @lock: protects the fields in this struct
> + * @source: name of the currently configured source of CRCs
> + * @opened: whether userspace has opened the data file for reading
> + * @entries: array of entries, with size of %DRM_CRTC_CRC_ENTRIES_NR
> + * @head: head of circular queue
> + * @tail: tail of circular queue
> + * @wq: workqueue used to synchronize reading and writing
> + */
> +struct drm_crtc_crc {
> + spinlock_t lock;
> + const char *source;
> + bool opened;
> + struct drm_crtc_crc_entry *entries;
> + int head, tail;
> + size_t values_cnt;
> + wait_queue_head_t wq;
> +};
> +
> +#if defined(CONFIG_DEBUG_FS)
> +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> + uint32_t frame, uint32_t *crcs);
> +#else
> +static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +{
> + return 0;
> +}
> +static inline int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> + uint32_t frame, uint32_t *crcs)
> +{
> + return -EINVAL;
> +}
> +#endif /* defined(CONFIG_DEBUG_FS) */
> +
> +#endif /* __DRM_DEBUGFS_CRC_H__ */
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-08-03 7:06 ` Ville Syrjälä
@ 2016-08-03 7:57 ` Daniel Vetter
2016-08-05 10:46 ` Tomeu Vizoso
1 sibling, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2016-08-03 7:57 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Tomeu Vizoso, Jonathan Corbet, linux-doc, linux-kernel,
dri-devel, Daniel Vetter, Emil Velikov
On Wed, Aug 03, 2016 at 10:06:38AM +0300, Ville Syrjälä wrote:
> On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:
> > Adds files and directories to debugfs for controlling and reading frame
> > CRCs, per CRTC:
> >
> > dri/0/crtc-0/crc
> > dri/0/crtc-0/crc/control
> > dri/0/crtc-0/crc/data
> >
> > Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
> > start and stop generating frame CRCs and can add entries to the output
> > by calling drm_crtc_add_crc_entry.
> >
> > v2:
> > - Lots of good fixes suggested by Thierry.
> > - Added documentation.
> > - Changed the debugfs layout.
> > - Moved to allocate the entries circular queue once when frame
> > generation gets enabled for the first time.
> > v3:
> > - Use the control file just to select the source, and start and stop
> > capture when the data file is opened and closed, respectively.
> > - Make variable the number of CRC values per entry, per source.
> > - Allocate entries queue each time we start capturing as now there
> > isn't a fixed number of CRC values per entry.
> > - Store the frame counter in the data file as a 8-digit hex number.
> > - For sources that cannot provide useful frame numbers, place
> > XXXXXXXX in the frame field.
> >
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > ---
> >
> > Documentation/gpu/drm-uapi.rst | 6 +
> > drivers/gpu/drm/Makefile | 3 +-
> > drivers/gpu/drm/drm_crtc.c | 12 ++
> > drivers/gpu/drm/drm_debugfs.c | 36 +++-
> > drivers/gpu/drm/drm_debugfs_crc.c | 370 ++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_drv.c | 9 +
> > drivers/gpu/drm/drm_internal.h | 10 ++
> > include/drm/drmP.h | 5 +
> > include/drm/drm_crtc.h | 41 +++++
> > include/drm/drm_debugfs_crc.h | 74 ++++++++
> > 10 files changed, 564 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/gpu/drm/drm_debugfs_crc.c
> > create mode 100644 include/drm/drm_debugfs_crc.h
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 536bf3eaadd4..33f778696ccd 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -109,3 +109,9 @@ interfaces. Especially since all hardware-acceleration interfaces to
> > userspace are driver specific for efficiency and other reasons these
> > interfaces can be rather substantial. Hence every driver has its own
> > chapter.
> > +
> > +Testing and validation
> > +======================
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_debugfs_crc.c
> > + :doc: CRC ABI
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index e3dba6f44a79..b53b5aaaeb4d 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -12,7 +12,8 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> > drm_info.o drm_debugfs.o drm_encoder_slave.o \
> > drm_trace_points.o drm_global.o drm_prime.o \
> > drm_rect.o drm_vma_manager.o drm_flip_work.o \
> > - drm_modeset_lock.o drm_atomic.o drm_bridge.o
> > + drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> > + drm_debugfs_crc.o
> >
> > drm-$(CONFIG_COMPAT) += drm_ioc32.o
> > drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 10b73f68c023..087345af96e7 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -40,6 +40,7 @@
> > #include <drm/drm_modeset_lock.h>
> > #include <drm/drm_atomic.h>
> > #include <drm/drm_auth.h>
> > +#include <drm/drm_debugfs_crc.h>
> >
> > #include "drm_crtc_internal.h"
> > #include "drm_internal.h"
> > @@ -738,6 +739,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> > if (cursor)
> > cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > + spin_lock_init(&crtc->crc.lock);
> > + init_waitqueue_head(&crtc->crc.wq);
> > + crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> > +#endif
> > +
> > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > drm_object_attach_property(&crtc->base, config->prop_active, 0);
> > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> > @@ -764,6 +771,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
> > * the indices on the drm_crtc after us in the crtc_list.
> > */
> >
> > +#ifdef CONFIG_DEBUG_FS
> > + drm_debugfs_crtc_remove(crtc);
> > + kfree(crtc->crc.source);
> > +#endif
> > +
> > kfree(crtc->gamma_store);
> > crtc->gamma_store = NULL;
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index fa10cef2ba37..73530cbf1316 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -415,5 +415,39 @@ void drm_debugfs_connector_remove(struct drm_connector *connector)
> > connector->debugfs_entry = NULL;
> > }
> >
> > -#endif /* CONFIG_DEBUG_FS */
> > +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> > +{
> > + struct drm_minor *minor = crtc->dev->primary;
> > + struct dentry *root;
> > + char *name;
> > +
> > + name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
> > + if (!name)
> > + return -ENOMEM;
> >
> > + root = debugfs_create_dir(name, minor->debugfs_root);
> > + kfree(name);
> > + if (!root)
> > + return -ENOMEM;
> > +
> > + crtc->debugfs_entry = root;
> > +
> > + if (drm_debugfs_crtc_crc_add(crtc))
> > + goto error;
> > +
> > + return 0;
> > +
> > +error:
> > + debugfs_remove_recursive(crtc->debugfs_entry);
> > + crtc->debugfs_entry = NULL;
> > + return -ENOMEM;
> > +}
> > +
> > +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> > +{
> > + debugfs_remove_recursive(crtc->debugfs_entry);
> > +
> > + crtc->debugfs_entry = NULL;
> > +}
> > +
> > +#endif /* CONFIG_DEBUG_FS */
> > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > new file mode 100644
> > index 000000000000..5ef071437952
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -0,0 +1,370 @@
> > +/*
> > + * Copyright © 2008 Intel Corporation
> > + * Copyright © 2016 Collabora Ltd
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Eric Anholt <eric@anholt.net>
> > + * Keith Packard <keithp@keithp.com>
> > + *
> > + */
> > +
> > +#include <linux/circ_buf.h>
> > +#include <linux/ctype.h>
> > +#include <linux/debugfs.h>
> > +#include <drm/drmP.h>
> > +
> > +/**
> > + * DOC: CRC ABI
> > + *
> > + * DRM device drivers can provide to userspace CRC information of each frame as
> > + * it reached a given hardware component (a "source").
> > + *
> > + * Userspace can control generation of CRCs in a given CRTC by writing to the
> > + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
> > + * Accepted values are source names (which are driver-specific) and the "none"
> > + * and "auto" keywords. "none" will disable CRC generation and "auto" will let
> > + * the driver select a default source of frame CRCs for this CRTC.
> > + *
> > + * Once frame CRC generation is enabled, userspace can capture them by reading
> > + * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
> > + * number in the first field and then a number of unsigned integer fields
> > + * containing the CRC data. Fields are separated by a single space and the number
> > + * of CRC fields is source-specific.
> > + *
> > + * Note that though in some cases the CRC is computed in a specified way and on
> > + * the frame contents as supplied by userspace (eDP 1.3), in general the CRC
> > + * computation is performed in an unspecified way and on frame contents that have
> > + * been already processed in also an unspecified way and thus userspace cannot
> > + * rely on being able to generate matching CRC values for the frame contents that
> > + * it submits. In this general case, the maximum userspace can do is to compare
> > + * the reported CRCs of frames that should have the same contents.
> > + */
> > +#if defined(CONFIG_DEBUG_FS)
> > +
> > +static int crc_control_show(struct seq_file *m, void *data)
> > +{
> > + struct drm_crtc *crtc = m->private;
> > +
> > + seq_printf(m, "%s\n", crtc->crc.source);
> > +
> > + return 0;
> > +}
> > +
> > +static int crc_control_open(struct inode *inode, struct file *file)
> > +{
> > + struct drm_crtc *crtc = inode->i_private;
> > +
> > + return single_open(file, crc_control_show, crtc);
> > +}
> > +
> > +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
> > + size_t len, loff_t *offp)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct drm_crtc *crtc = m->private;
> > + struct drm_crtc_crc *crc = &crtc->crc;
> > + char *source;
> > +
> > + if (len == 0)
> > + return 0;
> > +
> > + if (len > PAGE_SIZE - 1) {
> > + DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
> > + PAGE_SIZE);
> > + return -E2BIG;
> > + }
> > +
> > + source = kmalloc(len + 1, GFP_KERNEL);
> > + if (!source)
> > + return -ENOMEM;
> > +
> > + if (copy_from_user(source, ubuf, len)) {
> > + kfree(source);
> > + return -EFAULT;
> > + }
>
> memdup_user_nul() ?
>
> > +
> > + if (source[len - 1] == '\n')
> > + source[len - 1] = '\0';
> > + else
> > + source[len] = '\0';
> > +
> > + spin_lock_irq(&crc->lock);
> > +
> > + if (crc->opened) {
> > + kfree(source);
> > + return -EBUSY;
> > + }
>
> Why not just start the thing here?
>
> > +
> > + kfree(crc->source);
> > + crc->source = source;
> > +
> > + spin_unlock_irq(&crc->lock);
> > +
> > + *offp += len;
> > + return len;
> > +}
> > +
> > +const struct file_operations drm_crtc_crc_control_fops = {
> > + .owner = THIS_MODULE,
> > + .open = crc_control_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > + .write = crc_control_write
> > +};
> > +
> > +static int crtc_crc_open(struct inode *inode, struct file *filep)
> > +{
> > + struct drm_crtc *crtc = inode->i_private;
> > + struct drm_crtc_crc *crc = &crtc->crc;
> > + struct drm_crtc_crc_entry *entries = NULL;
> > + size_t entry_size, values_cnt;
> > + int ret;
> > +
> > + if (crc->opened)
> > + return -EBUSY;
> > +
> > + ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> > + if (ret)
> > + return ret;
> > +
> > + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> > + ret = -EINVAL;
> > + goto err_disable;
> > + }
> > +
> > + if (WARN_ON(values_cnt == 0)) {
> > + ret = -EINVAL;
> > + goto err_disable;
> > + }
> > +
> > + entry_size = sizeof(*crc->entries) + sizeof(uint32_t) * values_cnt;
> > + entries = kcalloc(DRM_CRC_ENTRIES_NR, entry_size, GFP_KERNEL);
> > + if (!entries) {
> > + ret = -ENOMEM;
> > + goto err_disable;
> > + }
> > +
> > + spin_lock_irq(&crc->lock);
> > + crc->entries = entries;
> > + crc->values_cnt = values_cnt;
> > + crc->opened = true;
> > + spin_unlock_irq(&crc->lock);
> > +
> > + return 0;
> > +
> > +err_disable:
> > + crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> > + return ret;
> > +}
> > +
> > +static int crtc_crc_release(struct inode *inode, struct file *filep)
> > +{
> > + struct drm_crtc *crtc = filep->f_inode->i_private;
> > + struct drm_crtc_crc *crc = &crtc->crc;
> > + size_t values_cnt;
> > +
> > + crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> > +
> > + spin_lock_irq(&crc->lock);
> > + kfree(crc->entries);
> > + crc->entries = NULL;
> > + crc->head = 0;
> > + crc->tail = 0;
> > + crc->values_cnt = 0;
> > + crc->opened = false;
> > + spin_unlock_irq(&crc->lock);
> > +
> > + 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);
> > +}
> > +
> > +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
> > + int index)
> > +{
> > + void *p = crc->entries;
> > + size_t entry_size = (sizeof(*crc->entries) +
> > + sizeof(*crc->entries[0].crcs) * crc->values_cnt);
>
> This computation is duplicated also in crtc_crc_open(). could use a
> common helper to do it.
>
> Shame the language doesn't have a way to deal with arrays of variable
> sized arrays in a nice way.
>
> > +
> > + return p + entry_size * index;
> > +}
> > +
> > +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
> > +
> > +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> > + size_t count, loff_t *pos)
> > +{
> > + struct drm_crtc *crtc = filep->f_inode->i_private;
> > + struct drm_crtc_crc *crc = &crtc->crc;
> > + struct drm_crtc_crc_entry *entry;
> > + char buf[MAX_LINE_LEN];
> > + int ret, i;
> > +
> > + spin_lock_irq(&crc->lock);
> > +
> > + if (!crc->source) {
> > + spin_unlock_irq(&crc->lock);
> > + return 0;
> > + }
> > +
> > + /* Nothing to read? */
> > + while (crtc_crc_data_count(crc) == 0) {
> > + if (filep->f_flags & O_NONBLOCK) {
> > + spin_unlock_irq(&crc->lock);
> > + return -EAGAIN;
> > + }
> > +
> > + ret = wait_event_interruptible_lock_irq(crc->wq,
> > + crtc_crc_data_count(crc),
> > + crc->lock);
> > + if (ret) {
> > + spin_unlock_irq(&crc->lock);
> > + return ret;
> > + }
> > + }
> > +
> > + /* We know we have an entry to be read */
> > + entry = crtc_get_crc_entry(crc, crc->tail);
> > +
> > + /*
> > + * 1 frame field of 8 chars plus a number of CRC fields of 8
> > + * chars each, space separated and with a newline at the end.
> > + */
> > + if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
>
> Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
> crc->values_cnt or DRM_MAX_CRC_NR as an argument.
>
> > + spin_unlock_irq(&crc->lock);
> > + return -EINVAL;
> > + }
> > +
> > + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
> > + crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> > +
> > + spin_unlock_irq(&crc->lock);
This here is a bit racy, I'd just loop over the ringbuf and emit crcs
one-by-one. That's simpler flow, and we don't care about performance here
at all. But then that's also a bikeshed ;-)
> > +
> > + if (entry->has_frame_counter)
> > + snprintf(buf, 9, "%08x", entry->frame);
> > + else
> > + snprintf(buf, 9, "XXXXXXXX");
>
> Should we add "0x" prefix to all these numbers to make it clear that
> they're in fact hex?
>
> > +
> > + for (i = 0; i < crc->values_cnt; i++)
> > + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
>
> The 'n' in snprintf() here seems pointless. As does the strlen().
>
> > + snprintf(buf + strlen(buf), 2, "\n");
> > +
> > + if (copy_to_user(user_buf, buf, strlen(buf) + 1))
> > + return -EFAULT;
> > +
> > + return strlen(buf) + 1;
>
> More strlen()s that shouldn't be needed.
Since I just reviewed this on the i915 side: I think we should _not_
include the terminating 0 char in what we copy to userspace and report as
the copied lenght. A file already has out-of-band lenght/eof information,
sprinkling 0 chars at random intervals seems pointless. Random because you
only do it at the end of a read, not after each crc.
-Daniel
>
> > +}
> > +
> > +const struct file_operations drm_crtc_crc_data_fops = {
> > + .owner = THIS_MODULE,
> > + .open = crtc_crc_open,
> > + .read = crtc_crc_read,
> > + .release = crtc_crc_release,
> > +};
> > +
> > +/**
> > + * drm_debugfs_crtc_crc_add - Add files to debugfs for capture of frame CRCs
> > + * @crtc: CRTC to whom the frames will belong
> > + *
> > + * Adds files to debugfs directory that allows userspace to control the
> > + * generation of frame CRCs and to read them.
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> > +{
> > + struct dentry *crc_ent, *ent;
> > +
> > + if (!crtc->funcs->set_crc_source)
> > + return 0;
> > +
> > + crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> > + if (!crc_ent)
> > + return -ENOMEM;
> > +
> > + ent = debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
> > + &drm_crtc_crc_control_fops);
> > + if (!ent)
> > + goto error;
> > +
> > + ent = debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
> > + &drm_crtc_crc_data_fops);
> > + if (!ent)
> > + goto error;
> > +
> > + return 0;
> > +
> > +error:
> > + debugfs_remove_recursive(crc_ent);
> > +
> > + return -ENOMEM;
> > +}
> > +
> > +/**
> > + * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
> > + * @crtc: CRTC to which the frame belongs
> > + * @has_frame: whether this entry has a frame number to go with
> > + * @frame: number of the frame these CRCs are about
> > + * @crcs: array of CRC values, with length matching #drm_crtc_crc.values_cnt
> > + *
> > + * For each frame, the driver polls the source of CRCs for new data and calls
> > + * this function to add them to the buffer from where userspace reads.
> > + */
> > +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > + uint32_t frame, uint32_t *crcs)
> > +{
> > + struct drm_crtc_crc *crc = &crtc->crc;
> > + struct drm_crtc_crc_entry *entry;
> > + int head, tail;
> > +
> > + assert_spin_locked(&crc->lock);
> > +
> > + /* Caller may not have noticed yet that userspace has stopped reading */
> > + if (!crc->opened)
> > + return -EINVAL;
> > +
> > + head = crc->head;
> > + tail = crc->tail;
> > +
> > + if (CIRC_SPACE(head, tail, DRM_CRC_ENTRIES_NR) < 1) {
> > + DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> > + return -ENOBUFS;
> > + }
> > +
> > + entry = crtc_get_crc_entry(crc, head);
> > + entry->frame = frame;
> > + entry->has_frame_counter = has_frame;
> > + memcpy(&entry->crcs, crcs, sizeof(*crcs) * crc->values_cnt);
> > +
> > + head = (head + 1) & (DRM_CRC_ENTRIES_NR - 1);
> > + crc->head = head;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_crtc_add_crc_entry);
> > +#endif /* CONFIG_DEBUG_FS */
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index aead9ffcbe29..a02406f51d98 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -192,6 +192,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
> > static int drm_minor_register(struct drm_device *dev, unsigned int type)
> > {
> > struct drm_minor *minor;
> > + struct drm_crtc *crtc;
> > unsigned long flags;
> > int ret;
> >
> > @@ -207,6 +208,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
> > return ret;
> > }
> >
> > + if (type == DRM_MINOR_LEGACY) {
> > + drm_for_each_crtc(crtc, dev) {
> > + ret = drm_debugfs_crtc_add(crtc);
> > + if (ret)
> > + goto err_debugfs;
> > + }
> > + }
> > +
> > ret = device_add(minor->kdev);
> > if (ret)
> > goto err_debugfs;
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index b86dc9b921a5..99ce6d4f2916 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -97,6 +97,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > int drm_debugfs_cleanup(struct drm_minor *minor);
> > int drm_debugfs_connector_add(struct drm_connector *connector);
> > void drm_debugfs_connector_remove(struct drm_connector *connector);
> > +int drm_debugfs_crtc_add(struct drm_crtc *crtc);
> > +void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
> > #else
> > static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > struct dentry *root)
> > @@ -116,4 +118,12 @@ static inline int drm_debugfs_connector_add(struct drm_connector *connector)
> > static inline void drm_debugfs_connector_remove(struct drm_connector *connector)
> > {
> > }
> > +
> > +static inline int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> > +{
> > + return 0;
> > +}
> > +static inline void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> > +{
> > +}
> > #endif
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index cf918e3e6afb..8a0b235ccc39 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1111,6 +1111,11 @@ static __inline__ bool drm_can_sleep(void)
> > return true;
> > }
> >
> > +#if defined(CONFIG_DEBUG_FS)
> > +extern const struct file_operations drm_crc_control_fops;
> > +extern const struct file_operations drm_crtc_crc_fops;
> > +#endif
> > +
> > /* helper for handling conditionals in various for_each macros */
> > #define for_each_if(condition) if (!(condition)) {} else
> >
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 781695c74528..fcc632940dce 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -35,6 +35,7 @@
> > #include <uapi/drm/drm_mode.h>
> > #include <uapi/drm/drm_fourcc.h>
> > #include <drm/drm_modeset_lock.h>
> > +#include <drm/drm_debugfs_crc.h>
> >
> > struct drm_device;
> > struct drm_mode_set;
> > @@ -731,6 +732,30 @@ struct drm_crtc_funcs {
> > * before data structures are torndown.
> > */
> > void (*early_unregister)(struct drm_crtc *crtc);
> > +
> > + /**
> > + * @set_crc_source:
> > + *
> > + * Changes the source of CRC checksums of frames at the request of
> > + * userspace, typically for testing purposes. The sources available are
> > + * specific of each driver and a %NULL value indicates that CRC
> > + * generation is to be switched off.
> > + *
> > + * When CRC generation is enabled, the driver should call
> > + * drm_crtc_add_crc_entry() at each frame, providing any information
> > + * that characterizes the frame contents in the crcN arguments, as
> > + * provided from the configured source. Drivers should accept a "auto"
> > + * source name that will select a default source for this CRTC.
> > + *
> > + * This callback is optional if the driver does not support any CRC
> > + * generation functionality.
> > + *
> > + * RETURNS:
> > + *
> > + * 0 on success or a negative error code on failure.
> > + */
> > + int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> > + size_t *values_cnt);
> > };
> >
> > /**
> > @@ -844,6 +869,22 @@ struct drm_crtc {
> > * context.
> > */
> > struct drm_modeset_acquire_ctx *acquire_ctx;
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > + /**
> > + * @debugfs_entry:
> > + *
> > + * Debugfs directory for this CRTC.
> > + */
> > + struct dentry *debugfs_entry;
> > +
> > + /**
> > + * @crc:
> > + *
> > + * Configuration settings of CRC capture.
> > + */
> > + struct drm_crtc_crc crc;
> > +#endif
> > };
> >
> > /**
> > diff --git a/include/drm/drm_debugfs_crc.h b/include/drm/drm_debugfs_crc.h
> > new file mode 100644
> > index 000000000000..a341fc9becad
> > --- /dev/null
> > +++ b/include/drm/drm_debugfs_crc.h
> > @@ -0,0 +1,74 @@
> > +/*
> > + * Copyright © 2016 Collabora Ltd.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +#ifndef __DRM_DEBUGFS_CRC_H__
> > +#define __DRM_DEBUGFS_CRC_H__
> > +
> > +/**
> > + * struct drm_crtc_crc_entry - entry describing a frame's content
> > + * @frame: number of the frame this CRC is about
> > + * @crc: array of values that characterize the frame
> > + */
> > +struct drm_crtc_crc_entry {
> > + bool has_frame_counter;
> > + uint32_t frame;
> > + uint32_t crcs[0];
> > +};
> > +
> > +#define DRM_MAX_CRC_NR 10
> > +#define DRM_CRC_ENTRIES_NR 128
> > +/**
> > + * struct drm_crtc_crc - data supporting CRC capture on a given CRTC
> > + * @lock: protects the fields in this struct
> > + * @source: name of the currently configured source of CRCs
> > + * @opened: whether userspace has opened the data file for reading
> > + * @entries: array of entries, with size of %DRM_CRTC_CRC_ENTRIES_NR
> > + * @head: head of circular queue
> > + * @tail: tail of circular queue
> > + * @wq: workqueue used to synchronize reading and writing
> > + */
> > +struct drm_crtc_crc {
> > + spinlock_t lock;
> > + const char *source;
> > + bool opened;
> > + struct drm_crtc_crc_entry *entries;
> > + int head, tail;
> > + size_t values_cnt;
> > + wait_queue_head_t wq;
> > +};
> > +
> > +#if defined(CONFIG_DEBUG_FS)
> > +int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> > +int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > + uint32_t frame, uint32_t *crcs);
> > +#else
> > +static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> > +{
> > + return 0;
> > +}
> > +static inline int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
> > + uint32_t frame, uint32_t *crcs)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif /* defined(CONFIG_DEBUG_FS) */
> > +
> > +#endif /* __DRM_DEBUGFS_CRC_H__ */
> > --
> > 2.5.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Use new CRC debugfs API
2016-07-22 14:10 ` [PATCH v3 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
@ 2016-08-03 8:01 ` Daniel Vetter
2016-08-04 8:00 ` [Intel-gfx] " Tomeu Vizoso
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2016-08-03 8:01 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, intel-gfx, dri-devel, Daniel Vetter, Emil Velikov
On Fri, Jul 22, 2016 at 04:10:45PM +0200, Tomeu Vizoso wrote:
> 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.
This should be a separate patch.
> - Make sure we don't mess with the state of the legacy CRC capture
> ABI implementation.
Hm, as for keeping the legacy debugfs interface alive I think it'd be nice
if we can implement the old one internal with the new one. I.e. call the
set_crc_sourc function (after minimal parsing), use the same ringbuffer
and rework the read function to use the same ringbuffer. Or maybe that's
not worth it since the plan is to nuke all that code anyway after a few
months?
-Daniel
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++-------
> drivers/gpu/drm/i915/intel_display.c | 1 +
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 124 ++++++++++++++++++++++++----------
> 4 files changed, 133 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b77d808b71cd..f2726171f7c8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1492,41 +1492,58 @@ 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;
>
> 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 {
> + 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;
> + ret = drm_crtc_add_crc_entry(crtc, true,
> + drm_accurate_vblank_count(crtc),
> + 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 111b350d1d7e..d91a2f779fb1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14019,6 +14019,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 5d06cb2425a1..4302cf2cfb04 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1782,6 +1782,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 4e6c28ce8ecf..d2f06a1a919a 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 == INTEL_PIPE_CRC_SOURCE_NONE) {
> + 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,10 +718,11 @@ 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) {
> @@ -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,31 @@ 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;
> + 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);
> +
> + *values_cnt = 5;
> +
> + return ret;
> +}
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: Use new CRC debugfs API
2016-08-03 8:01 ` Daniel Vetter
@ 2016-08-04 8:00 ` Tomeu Vizoso
0 siblings, 0 replies; 12+ messages in thread
From: Tomeu Vizoso @ 2016-08-04 8:00 UTC (permalink / raw)
To: Tomeu Vizoso, linux-kernel, Intel Graphics Development,
dri-devel, Daniel Vetter, Emil Velikov
On 3 August 2016 at 10:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jul 22, 2016 at 04:10:45PM +0200, Tomeu Vizoso wrote:
>> 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.
>
> This should be a separate patch.
Ok.
>> - Make sure we don't mess with the state of the legacy CRC capture
>> ABI implementation.
>
> Hm, as for keeping the legacy debugfs interface alive I think it'd be nice
> if we can implement the old one internal with the new one. I.e. call the
> set_crc_sourc function (after minimal parsing), use the same ringbuffer
> and rework the read function to use the same ringbuffer. Or maybe that's
> not worth it since the plan is to nuke all that code anyway after a few
> months?
I think that if the old code isn't to stay, there's not much point in
changing it to reuse parts of the new one.
Regards,
Tomeu
> -Daniel
>
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
>> ---
>>
>> drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++-------
>> drivers/gpu/drm/i915/intel_display.c | 1 +
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> drivers/gpu/drm/i915/intel_pipe_crc.c | 124 ++++++++++++++++++++++++----------
>> 4 files changed, 133 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index b77d808b71cd..f2726171f7c8 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1492,41 +1492,58 @@ 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;
>>
>> 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 {
>> + 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;
>> + ret = drm_crtc_add_crc_entry(crtc, true,
>> + drm_accurate_vblank_count(crtc),
>> + 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 111b350d1d7e..d91a2f779fb1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14019,6 +14019,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 5d06cb2425a1..4302cf2cfb04 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1782,6 +1782,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 4e6c28ce8ecf..d2f06a1a919a 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 == INTEL_PIPE_CRC_SOURCE_NONE) {
>> + 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,10 +718,11 @@ 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) {
>> @@ -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,31 @@ 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;
>> + 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);
>> +
>> + *values_cnt = 5;
>> +
>> + return ret;
>> +}
>> --
>> 2.5.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-08-03 7:06 ` Ville Syrjälä
2016-08-03 7:57 ` Daniel Vetter
@ 2016-08-05 10:46 ` Tomeu Vizoso
2016-08-05 11:42 ` Ville Syrjälä
1 sibling, 1 reply; 12+ messages in thread
From: Tomeu Vizoso @ 2016-08-05 10:46 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Jonathan Corbet, linux-doc, linux-kernel, dri-devel,
Daniel Vetter, Emil Velikov
On 3 August 2016 at 09:06, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:
>> Adds files and directories to debugfs for controlling and reading frame
>> CRCs, per CRTC:
>>
>> dri/0/crtc-0/crc
>> dri/0/crtc-0/crc/control
>> dri/0/crtc-0/crc/data
>>
>> Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
>> start and stop generating frame CRCs and can add entries to the output
>> by calling drm_crtc_add_crc_entry.
>>
>> v2:
>> - Lots of good fixes suggested by Thierry.
>> - Added documentation.
>> - Changed the debugfs layout.
>> - Moved to allocate the entries circular queue once when frame
>> generation gets enabled for the first time.
>> v3:
>> - Use the control file just to select the source, and start and stop
>> capture when the data file is opened and closed, respectively.
>> - Make variable the number of CRC values per entry, per source.
>> - Allocate entries queue each time we start capturing as now there
>> isn't a fixed number of CRC values per entry.
>> - Store the frame counter in the data file as a 8-digit hex number.
>> - For sources that cannot provide useful frame numbers, place
>> XXXXXXXX in the frame field.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
...
>> +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>> + size_t len, loff_t *offp)
>> +{
>> + struct seq_file *m = file->private_data;
>> + struct drm_crtc *crtc = m->private;
>> + struct drm_crtc_crc *crc = &crtc->crc;
>> + char *source;
>> +
>> + if (len == 0)
>> + return 0;
>> +
>> + if (len > PAGE_SIZE - 1) {
>> + DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
>> + PAGE_SIZE);
>> + return -E2BIG;
>> + }
>> +
>> + source = kmalloc(len + 1, GFP_KERNEL);
>> + if (!source)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(source, ubuf, len)) {
>> + kfree(source);
>> + return -EFAULT;
>> + }
>
> memdup_user_nul() ?
Good call.
>> +
>> + if (source[len - 1] == '\n')
>> + source[len - 1] = '\0';
>> + else
>> + source[len] = '\0';
>> +
>> + spin_lock_irq(&crc->lock);
>> +
>> + if (crc->opened) {
>> + kfree(source);
>> + return -EBUSY;
>> + }
>
> Why not just start the thing here?
For the sake of symmetry, as we are stopping when the data file is closed.
>> +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
>> + int index)
>> +{
>> + void *p = crc->entries;
>> + size_t entry_size = (sizeof(*crc->entries) +
>> + sizeof(*crc->entries[0].crcs) * crc->values_cnt);
>
> This computation is duplicated also in crtc_crc_open(). could use a
> common helper to do it.
>
> Shame the language doesn't have a way to deal with arrays of variable
> sized arrays in a nice way.
Ok.
>> +
>> + return p + entry_size * index;
>> +}
>> +
>> +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
>> +
>> +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>> + size_t count, loff_t *pos)
>> +{
>> + struct drm_crtc *crtc = filep->f_inode->i_private;
>> + struct drm_crtc_crc *crc = &crtc->crc;
>> + struct drm_crtc_crc_entry *entry;
>> + char buf[MAX_LINE_LEN];
>> + int ret, i;
>> +
>> + spin_lock_irq(&crc->lock);
>> +
>> + if (!crc->source) {
>> + spin_unlock_irq(&crc->lock);
>> + return 0;
>> + }
>> +
>> + /* Nothing to read? */
>> + while (crtc_crc_data_count(crc) == 0) {
>> + if (filep->f_flags & O_NONBLOCK) {
>> + spin_unlock_irq(&crc->lock);
>> + return -EAGAIN;
>> + }
>> +
>> + ret = wait_event_interruptible_lock_irq(crc->wq,
>> + crtc_crc_data_count(crc),
>> + crc->lock);
>> + if (ret) {
>> + spin_unlock_irq(&crc->lock);
>> + return ret;
>> + }
>> + }
>> +
>> + /* We know we have an entry to be read */
>> + entry = crtc_get_crc_entry(crc, crc->tail);
>> +
>> + /*
>> + * 1 frame field of 8 chars plus a number of CRC fields of 8
>> + * chars each, space separated and with a newline at the end.
>> + */
>> + if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
>
> Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
> crc->values_cnt or DRM_MAX_CRC_NR as an argument.
Sounds good, went with a macro.
>> + spin_unlock_irq(&crc->lock);
>> + return -EINVAL;
>> + }
>> +
>> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
>> + crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
>> +
>> + spin_unlock_irq(&crc->lock);
>> +
>> + if (entry->has_frame_counter)
>> + snprintf(buf, 9, "%08x", entry->frame);
>> + else
>> + snprintf(buf, 9, "XXXXXXXX");
>
> Should we add "0x" prefix to all these numbers to make it clear that
> they're in fact hex?
Sounds like a good idea to me.
>> +
>> + for (i = 0; i < crc->values_cnt; i++)
>> + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
>
> The 'n' in snprintf() here seems pointless. As does the strlen().
Good.
>> + snprintf(buf + strlen(buf), 2, "\n");
>> +
>> + if (copy_to_user(user_buf, buf, strlen(buf) + 1))
>> + return -EFAULT;
>> +
>> + return strlen(buf) + 1;
>
> More strlen()s that shouldn't be needed.
Ok.
Thanks!
Tomeu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-08-05 10:46 ` Tomeu Vizoso
@ 2016-08-05 11:42 ` Ville Syrjälä
0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2016-08-05 11:42 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Jonathan Corbet, linux-doc, linux-kernel, dri-devel,
Daniel Vetter, Emil Velikov
On Fri, Aug 05, 2016 at 12:46:29PM +0200, Tomeu Vizoso wrote:
> On 3 August 2016 at 09:06, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:
> >> Adds files and directories to debugfs for controlling and reading frame
> >> CRCs, per CRTC:
> >>
> >> dri/0/crtc-0/crc
> >> dri/0/crtc-0/crc/control
> >> dri/0/crtc-0/crc/data
> >>
> >> Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
> >> start and stop generating frame CRCs and can add entries to the output
> >> by calling drm_crtc_add_crc_entry.
> >>
> >> v2:
> >> - Lots of good fixes suggested by Thierry.
> >> - Added documentation.
> >> - Changed the debugfs layout.
> >> - Moved to allocate the entries circular queue once when frame
> >> generation gets enabled for the first time.
> >> v3:
> >> - Use the control file just to select the source, and start and stop
> >> capture when the data file is opened and closed, respectively.
> >> - Make variable the number of CRC values per entry, per source.
> >> - Allocate entries queue each time we start capturing as now there
> >> isn't a fixed number of CRC values per entry.
> >> - Store the frame counter in the data file as a 8-digit hex number.
> >> - For sources that cannot provide useful frame numbers, place
> >> XXXXXXXX in the frame field.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> ---
> ...
> >> +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
> >> + size_t len, loff_t *offp)
> >> +{
> >> + struct seq_file *m = file->private_data;
> >> + struct drm_crtc *crtc = m->private;
> >> + struct drm_crtc_crc *crc = &crtc->crc;
> >> + char *source;
> >> +
> >> + if (len == 0)
> >> + return 0;
> >> +
> >> + if (len > PAGE_SIZE - 1) {
> >> + DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
> >> + PAGE_SIZE);
> >> + return -E2BIG;
> >> + }
> >> +
> >> + source = kmalloc(len + 1, GFP_KERNEL);
> >> + if (!source)
> >> + return -ENOMEM;
> >> +
> >> + if (copy_from_user(source, ubuf, len)) {
> >> + kfree(source);
> >> + return -EFAULT;
> >> + }
> >
> > memdup_user_nul() ?
>
> Good call.
>
> >> +
> >> + if (source[len - 1] == '\n')
> >> + source[len - 1] = '\0';
> >> + else
> >> + source[len] = '\0';
> >> +
> >> + spin_lock_irq(&crc->lock);
> >> +
> >> + if (crc->opened) {
> >> + kfree(source);
> >> + return -EBUSY;
> >> + }
> >
> > Why not just start the thing here?
>
> For the sake of symmetry, as we are stopping when the data file is closed.
Yes, but if the data file is already open, we should start as soon as
the source is configured. Or are you redusing to open the data file w/o
a source selected?
>
> >> +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
> >> + int index)
> >> +{
> >> + void *p = crc->entries;
> >> + size_t entry_size = (sizeof(*crc->entries) +
> >> + sizeof(*crc->entries[0].crcs) * crc->values_cnt);
> >
> > This computation is duplicated also in crtc_crc_open(). could use a
> > common helper to do it.
> >
> > Shame the language doesn't have a way to deal with arrays of variable
> > sized arrays in a nice way.
>
> Ok.
>
> >> +
> >> + return p + entry_size * index;
> >> +}
> >> +
> >> +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
> >> +
> >> +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> >> + size_t count, loff_t *pos)
> >> +{
> >> + struct drm_crtc *crtc = filep->f_inode->i_private;
> >> + struct drm_crtc_crc *crc = &crtc->crc;
> >> + struct drm_crtc_crc_entry *entry;
> >> + char buf[MAX_LINE_LEN];
> >> + int ret, i;
> >> +
> >> + spin_lock_irq(&crc->lock);
> >> +
> >> + if (!crc->source) {
> >> + spin_unlock_irq(&crc->lock);
> >> + return 0;
> >> + }
> >> +
> >> + /* Nothing to read? */
> >> + while (crtc_crc_data_count(crc) == 0) {
> >> + if (filep->f_flags & O_NONBLOCK) {
> >> + spin_unlock_irq(&crc->lock);
> >> + return -EAGAIN;
> >> + }
> >> +
> >> + ret = wait_event_interruptible_lock_irq(crc->wq,
> >> + crtc_crc_data_count(crc),
> >> + crc->lock);
> >> + if (ret) {
> >> + spin_unlock_irq(&crc->lock);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + /* We know we have an entry to be read */
> >> + entry = crtc_get_crc_entry(crc, crc->tail);
> >> +
> >> + /*
> >> + * 1 frame field of 8 chars plus a number of CRC fields of 8
> >> + * chars each, space separated and with a newline at the end.
> >> + */
> >> + if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
> >
> > Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
> > crc->values_cnt or DRM_MAX_CRC_NR as an argument.
>
> Sounds good, went with a macro.
>
> >> + spin_unlock_irq(&crc->lock);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
> >> + crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> >> +
> >> + spin_unlock_irq(&crc->lock);
> >> +
> >> + if (entry->has_frame_counter)
> >> + snprintf(buf, 9, "%08x", entry->frame);
> >> + else
> >> + snprintf(buf, 9, "XXXXXXXX");
> >
> > Should we add "0x" prefix to all these numbers to make it clear that
> > they're in fact hex?
>
> Sounds like a good idea to me.
>
> >> +
> >> + for (i = 0; i < crc->values_cnt; i++)
> >> + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
> >
> > The 'n' in snprintf() here seems pointless. As does the strlen().
>
> Good.
>
> >> + snprintf(buf + strlen(buf), 2, "\n");
> >> +
> >> + if (copy_to_user(user_buf, buf, strlen(buf) + 1))
> >> + return -EFAULT;
> >> +
> >> + return strlen(buf) + 1;
> >
> > More strlen()s that shouldn't be needed.
>
> Ok.
>
> Thanks!
>
> Tomeu
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-07-22 14:10 ` [PATCH v3 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-08-03 7:06 ` Ville Syrjälä
@ 2016-08-06 17:04 ` Daniel Stone
2016-08-08 9:09 ` Tomeu Vizoso
1 sibling, 1 reply; 12+ messages in thread
From: Daniel Stone @ 2016-08-06 17:04 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Linux Kernel Mailing List, linux-doc, Jonathan Corbet, dri-devel,
Daniel Vetter, Emil Velikov
Hi Tomeu,
On 22 July 2016 at 15:10, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> +/**
> + * DOC: CRC ABI
> + *
> + * DRM device drivers can provide to userspace CRC information of each frame as
> + * it reached a given hardware component (a "source").
> + *
> + * Userspace can control generation of CRCs in a given CRTC by writing to the
s/can/must/
Is it worth having 'auto' as a default source perhaps?
> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
> + * Accepted values are source names (which are driver-specific) and the "none"
> + * and "auto" keywords. "none" will disable CRC generation and "auto" will let
> + * the driver select a default source of frame CRCs for this CRTC.
Is it also worth having 'connector-%s' (named as per sysfs, e.g.
connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which
have CRC control on the connector rather than the CRTC?
Cheers,
Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
2016-08-06 17:04 ` Daniel Stone
@ 2016-08-08 9:09 ` Tomeu Vizoso
0 siblings, 0 replies; 12+ messages in thread
From: Tomeu Vizoso @ 2016-08-08 9:09 UTC (permalink / raw)
To: Daniel Stone
Cc: Jonathan Corbet, linux-doc, Linux Kernel Mailing List, dri-devel,
Daniel Vetter, Emil Velikov
On 6 August 2016 at 19:04, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi Tomeu,
>
> On 22 July 2016 at 15:10, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> +/**
>> + * DOC: CRC ABI
>> + *
>> + * DRM device drivers can provide to userspace CRC information of each frame as
>> + * it reached a given hardware component (a "source").
>> + *
>> + * Userspace can control generation of CRCs in a given CRTC by writing to the
>
> s/can/must/
>
> Is it worth having 'auto' as a default source perhaps?
Yup, it's the case in v4, so you just cat the data file and start getting CRCs.
>> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the CRTC.
>> + * Accepted values are source names (which are driver-specific) and the "none"
>> + * and "auto" keywords. "none" will disable CRC generation and "auto" will let
>> + * the driver select a default source of frame CRCs for this CRTC.
>
> Is it also worth having 'connector-%s' (named as per sysfs, e.g.
> connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which
> have CRC control on the connector rather than the CRTC?
My impression right now is that only "auto" makes sense as a
standardised entry, as any explicit sources are pretty much
hw-dependent so the tests will need knowledge about the hw anyway.
The IGT tests already try each connector in each CRTC when looking for
a setup that supports CRC capture (with the auto source).
Regards,
Tomeu
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-08-08 9:10 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-22 14:10 [PATCH v3 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-08-03 7:06 ` Ville Syrjälä
2016-08-03 7:57 ` Daniel Vetter
2016-08-05 10:46 ` Tomeu Vizoso
2016-08-05 11:42 ` Ville Syrjälä
2016-08-06 17:04 ` Daniel Stone
2016-08-08 9:09 ` Tomeu Vizoso
2016-07-22 14:10 ` [PATCH v3 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2016-08-03 8:01 ` Daniel Vetter
2016-08-04 8:00 ` [Intel-gfx] " Tomeu Vizoso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).