* [PATCH v1 0/3] New debugfs API for capturing CRC of frames
@ 2016-06-21 11:06 Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-21 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Tomeu Vizoso, Jani Nikula,
intel-gfx, dri-devel, 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 expected 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
drivers/gpu/drm/drm_crtc.c | 28 +-
drivers/gpu/drm/drm_debugfs.c | 506 ++++++++++++++++++-
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_dma.c | 2 -
drivers/gpu/drm/i915/i915_drv.h | 21 -
drivers/gpu/drm/i915/i915_irq.c | 39 +-
drivers/gpu/drm/i915/intel_display.c | 3 +
drivers/gpu/drm/i915/intel_drv.h | 5 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 556 +++++++++++++++++++++
include/drm/drmP.h | 5 +
include/drm/drm_crtc.h | 72 +++
13 files changed, 1181 insertions(+), 960 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_pipe_crc.c
--
2.5.5
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 1/3] drm/i915/debugfs: Move out pipe CRC code
2016-06-21 11:06 [PATCH v1 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
@ 2016-06-21 11:06 ` Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2 siblings, 0 replies; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-21 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, 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.
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 | 953 ++++++++++++++++++++++++++++++++++
4 files changed, 963 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 276abf1cac2b..2c14612dbdb5 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -17,7 +17,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 5b7526697838..ece88a1966b2 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,
@@ -3480,12 +3470,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;
@@ -3509,853 +3493,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 = 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;
-
- 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 = dev->dev_private;
- 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 = 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;
-}
-
-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)
@@ -5493,20 +4630,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 = 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 i915_debugfs_init(struct drm_minor *minor)
{
int ret, i;
@@ -5515,11 +4638,9 @@ int i915_debugfs_init(struct drm_minor *minor)
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,
@@ -5544,12 +4665,7 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
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 089a42577ea3..5fdc426640f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1743,4 +1743,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..590e909bcf1b
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -0,0 +1,953 @@
+/*
+ * Copyright © 2008 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.
+ *
+ * Authors:
+ * Eric Anholt <eric@anholt.net>
+ * Keith Packard <keithp@keithp.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] 16+ messages in thread
* [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-21 11:06 [PATCH v1 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
@ 2016-06-21 11:06 ` Tomeu Vizoso
2016-06-21 15:07 ` Thierry Reding
2016-06-22 14:20 ` Daniel Vetter
2016-06-21 11:06 ` [PATCH v1 3/3] drm/i915: Use new CRC debugfs API Tomeu Vizoso
2 siblings, 2 replies; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-21 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Tomeu Vizoso, David Airlie, dri-devel
Adds a per-device debugfile "drm_crc_control" that allows selecting a
source for frame checksums in each CRTC that supports them.
The checksums for each subsequent frame can be read from the per-CRTC
file "drm_crtc_N_crc".
The code is taken from the i915 driver and other drivers can now provide
frame CRCs by implementing the set_crc_source callback in
drm_crtc_funcs.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/drm_crtc.c | 28 ++-
drivers/gpu/drm/drm_debugfs.c | 506 ++++++++++++++++++++++++++++++++++++++++-
drivers/gpu/drm/drm_internal.h | 10 +
include/drm/drmP.h | 5 +
include/drm/drm_crtc.h | 72 ++++++
5 files changed, 611 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7c862bd2f19..4dae42b122d9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -657,8 +657,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
drm_num_crtcs(dev));
}
if (!crtc->name) {
- drm_mode_object_unregister(dev, &crtc->base);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_unregister;
}
crtc->base.properties = &crtc->properties;
@@ -673,12 +673,30 @@ 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.debugfs_entries = kmalloc_array(DRM_MINOR_CNT,
+ sizeof(*crtc->crc.debugfs_entries),
+ GFP_KERNEL);
+
+ ret = drm_debugfs_crtc_add(crtc);
+ if (ret)
+ goto err_free_name;
+#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);
}
return 0;
+
+err_free_name:
+ kfree(crtc->name);
+err_unregister:
+ drm_mode_object_unregister(dev, &crtc->base);
+ return ret;
}
EXPORT_SYMBOL(drm_crtc_init_with_planes);
@@ -699,6 +717,12 @@ 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.debugfs_entries);
+ 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..cdc8836bc22a 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -30,6 +30,8 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
+#include <linux/circ_buf.h>
+#include <linux/ctype.h>
#include <linux/debugfs.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
@@ -127,6 +129,259 @@ fail:
}
EXPORT_SYMBOL(drm_debugfs_create_files);
+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 crc_control_show(struct seq_file *m, void *data)
+{
+ struct drm_device *dev = m->private;
+ struct drm_crtc *crtc;
+
+ drm_for_each_crtc(crtc, dev)
+ seq_printf(m, "crtc %d %s\n", crtc->index,
+ crtc->crc.source ? crtc->crc.source : "none");
+
+ return 0;
+}
+
+static int crc_control_open(struct inode *inode, struct file *file)
+{
+ struct drm_device *dev = inode->i_private;
+
+ return single_open(file, crc_control_show, dev);
+}
+
+static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
+{
+ struct drm_crtc_crc *crc = &crtc->crc;
+ struct drm_crtc_crc_entry *entries;
+ int ret;
+
+ if (!strcmp(source, "none"))
+ source = NULL;
+
+ if (!crc->source && !source)
+ return 0;
+
+ if (crc->source && source && !strcmp(crc->source, source))
+ return 0;
+
+ /* Forbid changing the source without going back to "none". */
+ if (crc->source && source)
+ return -EINVAL;
+
+ if (!crtc->funcs->set_crc_source)
+ return -ENOTSUPP;
+
+ if (source) {
+ entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
+ sizeof(crc->entries[0]),
+ GFP_KERNEL);
+ if (!entries)
+ return -ENOMEM;
+
+ spin_lock_irq(&crc->lock);
+ kfree(crc->entries);
+ crc->entries = entries;
+ crc->head = 0;
+ crc->tail = 0;
+ spin_unlock_irq(&crc->lock);
+ }
+
+ ret = crtc->funcs->set_crc_source(crtc, source);
+ if (ret)
+ return ret;
+
+ kfree(crc->source);
+ crc->source = source ? kstrdup(source, GFP_KERNEL) : NULL;
+
+ if (!source) {
+ spin_lock_irq(&crc->lock);
+ entries = crc->entries;
+ crc->entries = NULL;
+ crc->head = 0;
+ crc->tail = 0;
+ spin_unlock_irq(&crc->lock);
+
+ kfree(entries);
+ }
+
+ return 0;
+}
+
+static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)
+{
+ struct drm_crtc *crtc;
+ int i = 0;
+
+ drm_for_each_crtc(crtc, dev)
+ if (i++ == index)
+ return crtc;
+
+ return NULL;
+}
+
+/*
+ * Parse CRC command strings:
+ * command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
+ * object: ('crtc' | 'pipe')
+ * crtc: (0 | 1 | 2 | ...)
+ * pipe: (A | B | C)
+ * source: (none | plane1 | plane2 | ...)
+ * wsp: (#0x20 | #0x9 | #0xA)+
+ *
+ * eg.:
+ * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
+ * "crtc 0 none" -> Stop CRC
+ */
+static int crc_control_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_KMS("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;
+}
+
+static int crc_control_parse_crtc(const char *buf, unsigned int *crtc_index)
+{
+ const char letter = buf[0];
+
+ if (!kstrtouint(buf, 10, crtc_index))
+ return 0;
+
+ /* Backwards compatibility for Intel-style pipe letters */
+ if (letter < 'A' || letter > 'Z')
+ return -EINVAL;
+
+ *crtc_index = letter - 'A';
+
+ return 0;
+}
+
+static int crc_control_parse(struct drm_device *dev, char *buf, size_t len)
+{
+#define N_WORDS 3
+ int n_words;
+ char *words[N_WORDS];
+ unsigned int crtc_index;
+ struct drm_crtc *crtc;
+
+ n_words = crc_control_tokenize(buf, words, N_WORDS);
+ if (n_words != N_WORDS) {
+ DRM_DEBUG_KMS("tokenize failed, a command is %d words\n",
+ N_WORDS);
+ return -EINVAL;
+ }
+
+ if (strcmp(words[0], "crtc") && strcmp(words[0], "pipe")) {
+ DRM_DEBUG_KMS("Invalid command %s\n", words[0]);
+ return -EINVAL;
+ }
+
+ if (crc_control_parse_crtc(words[1], &crtc_index) < 0) {
+ DRM_DEBUG_KMS("Invalid CRTC index: %s\n", words[1]);
+ return -EINVAL;
+ }
+
+ crtc = crtc_from_index(dev, crtc_index);
+ if (!crtc) {
+ DRM_DEBUG_KMS("Unknown CRTC index: %d\n", crtc_index);
+ return -EINVAL;
+ }
+
+ return crc_control_update_crtc(crtc, words[2]);
+}
+
+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_device *dev = m->private;
+ char *tmpbuf;
+ int ret;
+
+ 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;
+ }
+
+ 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 = crc_control_parse(dev, tmpbuf, len);
+out:
+ kfree(tmpbuf);
+ if (ret < 0)
+ return ret;
+
+ *offp += len;
+ return len;
+}
+
+const struct file_operations drm_crc_control_fops = {
+ .owner = THIS_MODULE,
+ .open = crc_control_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+ .write = crc_control_write
+};
+
/**
* Initialize the DRI debugfs filesystem for a device
*
@@ -142,8 +397,9 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
struct dentry *root)
{
struct drm_device *dev = minor->dev;
+ struct dentry *ent;
char name[64];
- int ret;
+ int ret = 0;
INIT_LIST_HEAD(&minor->debugfs_list);
mutex_init(&minor->debugfs_lock);
@@ -157,10 +413,23 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
minor->debugfs_root, minor);
if (ret) {
- debugfs_remove(minor->debugfs_root);
- minor->debugfs_root = NULL;
DRM_ERROR("Failed to create core drm debugfs files\n");
- return ret;
+ goto error_remove_dir;
+ }
+
+ ent = debugfs_create_file("drm_crc_control", S_IRUGO | S_IWUSR,
+ minor->debugfs_root, dev,
+ &drm_crc_control_fops);
+ if (!ent) {
+ DRM_ERROR("Failed to create CRC control debugfs files\n");
+ ret = -ENOMEM;
+ goto error_remove_files;
+ }
+
+ ret = drm_add_fake_info_node(minor, ent, &drm_crc_control_fops);
+ if (ret) {
+ DRM_ERROR("Failed to create CRC control debugfs files\n");
+ goto error_remove_crc;
}
if (dev->driver->debugfs_init) {
@@ -171,7 +440,18 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
return ret;
}
}
+
return 0;
+
+error_remove_crc:
+ debugfs_remove(ent);
+error_remove_files:
+ drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor);
+error_remove_dir:
+ debugfs_remove(minor->debugfs_root);
+ minor->debugfs_root = NULL;
+
+ return ret;
}
@@ -407,13 +687,223 @@ error:
void drm_debugfs_connector_remove(struct drm_connector *connector)
{
- if (!connector->debugfs_entry)
- return;
-
debugfs_remove_recursive(connector->debugfs_entry);
connector->debugfs_entry = NULL;
}
-#endif /* CONFIG_DEBUG_FS */
+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;
+
+ spin_lock_irq(&crc->lock);
+
+ if (crc->opened) {
+ spin_unlock_irq(&crc->lock);
+ return -EBUSY;
+ }
+
+ crc->opened = true;
+
+ spin_unlock_irq(&crc->lock);
+
+ return 0;
+}
+
+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;
+
+ spin_lock_irq(&crc->lock);
+ crc->opened = false;
+ spin_unlock_irq(&crc->lock);
+
+ return 0;
+}
+
+/* (6 fields, 8 chars each, space separated (5) + '\n') */
+#define CRC_LINE_LEN (6 * 8 + 5 + 1)
+/* account for \'0' */
+#define CRC_BUFFER_LEN (CRC_LINE_LEN + 1)
+
+static int crtc_crc_data_count(struct drm_crtc_crc *crc)
+{
+ assert_spin_locked(&crc->lock);
+ return CIRC_CNT(crc->head, crc->tail,
+ DRM_CRTC_CRC_ENTRIES_NR);
+}
+
+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;
+ char buf[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 < CRC_LINE_LEN)
+ return -EINVAL;
+
+ if (!crc->source)
+ return 0;
+
+ /* Nothing to read? */
+ spin_lock_irq(&crc->lock);
+ while (crtc_crc_data_count(crc) == 0) {
+ int ret;
+
+ 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 now have one or more entries to read */
+ n_entries = count / CRC_LINE_LEN;
+
+ bytes_read = 0;
+ while (n_entries > 0) {
+ struct drm_crtc_crc_entry *entry =
+ &crc->entries[crc->tail];
+ int ret;
+
+ if (CIRC_CNT(crc->head, crc->tail, DRM_CRTC_CRC_ENTRIES_NR) < 1)
+ break;
+
+ BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRTC_CRC_ENTRIES_NR);
+ crc->tail = (crc->tail + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
+
+ bytes_read += snprintf(buf, 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(&crc->lock);
+
+ ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
+ if (ret == CRC_LINE_LEN)
+ return -EFAULT;
+ user_buf += CRC_LINE_LEN;
+ n_entries--;
+
+ spin_lock_irq(&crc->lock);
+ }
+
+ spin_unlock_irq(&crc->lock);
+
+ return bytes_read;
+}
+
+const struct file_operations drm_crtc_crc_fops = {
+ .owner = THIS_MODULE,
+ .open = crtc_crc_open,
+ .read = crtc_crc_read,
+ .release = crtc_crc_release,
+};
+
+static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
+ struct drm_minor *minor)
+{
+ struct dentry *ent;
+ char *name;
+
+ if (!minor->debugfs_root)
+ return -1;
+
+ name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
+ if (!name)
+ return -ENOMEM;
+
+ ent = debugfs_create_file(name, S_IRUGO, minor->debugfs_root, crtc,
+ &drm_crtc_crc_fops);
+ kfree(name);
+ if (!ent)
+ return PTR_ERR(ent);
+
+ crtc->crc.debugfs_entries[minor->type] = ent;
+
+ return 0;
+}
+
+int drm_debugfs_crtc_add(struct drm_crtc *crtc)
+{
+ int ret;
+
+ ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
+ if (ret)
+ return ret;
+
+ ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
+ if (ret)
+ return ret;
+
+ ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
+{
+ int i;
+
+ for (i = 0; i < DRM_MINOR_CNT; i++) {
+ debugfs_remove_recursive(crtc->crc.debugfs_entries[i]);
+ crtc->crc.debugfs_entries[i] = NULL;
+ }
+}
+
+void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
+ uint32_t crc0, uint32_t crc1, uint32_t crc2,
+ uint32_t crc3, uint32_t crc4)
+{
+ struct drm_crtc_crc *crc = &crtc->crc;
+ struct drm_crtc_crc_entry *entry;
+ int head, tail;
+
+ spin_lock(&crc->lock);
+
+ head = crc->head;
+ tail = crc->tail;
+
+ if (CIRC_SPACE(head, tail, DRM_CRTC_CRC_ENTRIES_NR) < 1) {
+ spin_unlock(&crc->lock);
+ DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
+ return;
+ }
+
+ entry = &crc->entries[head];
+
+ entry->frame = frame;
+ entry->crc[0] = crc0;
+ entry->crc[1] = crc1;
+ entry->crc[2] = crc2;
+ entry->crc[3] = crc3;
+ entry->crc[4] = crc4;
+
+ head = (head + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
+ crc->head = head;
+
+ spin_unlock(&crc->lock);
+
+ wake_up_interruptible(&crc->wq);
+}
+
+#endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 38401d406532..e5b124d937f5 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -100,6 +100,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)
@@ -119,4 +121,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 084fd141e8bf..ec2f91c8b7cd 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1142,6 +1142,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 c2734979f164..141335a3c647 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -376,6 +376,22 @@ struct drm_crtc_state {
struct drm_atomic_state *state;
};
+struct drm_crtc_crc_entry {
+ uint32_t frame;
+ uint32_t crc[5];
+};
+
+#define DRM_CRTC_CRC_ENTRIES_NR 128
+struct drm_crtc_crc {
+ spinlock_t lock;
+ const char *source;
+ bool opened; /* exclusive access to the result file */
+ struct drm_crtc_crc_entry *entries;
+ int head, tail;
+ wait_queue_head_t wq;
+ struct dentry **debugfs_entries;
+};
+
/**
* struct drm_crtc_funcs - control CRTCs for a given device
*
@@ -704,6 +720,29 @@ struct drm_crtc_funcs {
const struct drm_crtc_state *state,
struct drm_property *property,
uint64_t *val);
+
+ /**
+ * @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);
};
/**
@@ -817,6 +856,15 @@ struct drm_crtc {
* context.
*/
struct drm_modeset_acquire_ctx *acquire_ctx;
+
+#ifdef CONFIG_DEBUG_FS
+ /**
+ * @drm_crtc_crc:
+ *
+ * Configuration settings of CRC capture.
+ */
+ struct drm_crtc_crc crc;
+#endif
};
/**
@@ -2496,6 +2544,30 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
return 1 << drm_crtc_index(crtc);
}
+/**
+ * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
+ * @crtc: CRTC to which the frame belongs
+ * @frame: number of the frame characterized by the CRC data
+ * @crc0: piece of data about frame
+ * @crc1: piece of data about frame
+ * @crc2: piece of data about frame
+ * @crc3: piece of data about frame
+ * @crc4: piece of data about frame
+ *
+ * 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.
+ */
+#if defined(CONFIG_DEBUG_FS)
+void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
+ uint32_t crc0, uint32_t crc1, uint32_t crc2,
+ uint32_t crc3, uint32_t crc4);
+#else
+static inline void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
+ uint32_t crc0, uint32_t crc1,
+ uint32_t crc2, uint32_t crc3,
+ uint32_t crc4) {}
+#endif
+
extern void drm_connector_ida_init(void);
extern void drm_connector_ida_destroy(void);
extern int drm_connector_init(struct drm_device *dev,
--
2.5.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 3/3] drm/i915: Use new CRC debugfs API
2016-06-21 11:06 [PATCH v1 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
@ 2016-06-21 11:06 ` Tomeu Vizoso
2 siblings, 0 replies; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-21 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Emil Velikov, Tomeu Vizoso, Jani Nikula,
intel-gfx, dri-devel, David Airlie
The core provides now an ABI to userspace for generation of CRCs that is
compatible with the one currently in i915. So remove the code that is
now duplicated and implement instead the ->set_crc_source() callback to
start and end CRC generation.
We still register files in the old debugfs paths so the current
testsuite still passes.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 +-
drivers/gpu/drm/i915/i915_dma.c | 2 -
drivers/gpu/drm/i915/i915_drv.h | 21 --
drivers/gpu/drm/i915/i915_irq.c | 39 +--
drivers/gpu/drm/i915/intel_display.c | 3 +
drivers/gpu/drm/i915/intel_drv.h | 4 +-
drivers/gpu/drm/i915/intel_pipe_crc.c | 511 ++++------------------------------
7 files changed, 67 insertions(+), 521 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ece88a1966b2..f82e66609ace 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4620,7 +4620,7 @@ static const struct i915_debugfs_files {
{"i915_gem_drop_caches", &i915_drop_caches_fops},
{"i915_error_state", &i915_error_state_fops},
{"i915_next_seqno", &i915_next_seqno_fops},
- {"i915_display_crc_ctl", &i915_display_crc_ctl_fops},
+ {"i915_display_crc_ctl", &drm_crc_control_fops},
{"i915_pri_wm_latency", &i915_pri_wm_latency_fops},
{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
@@ -4638,10 +4638,6 @@ int i915_debugfs_init(struct drm_minor *minor)
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,
i915_debugfs_files[i].name,
@@ -4665,8 +4661,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
1, minor);
- intel_pipe_crc_cleanup(minor);
-
for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
struct drm_info_list *info_list =
(struct drm_info_list *) i915_debugfs_files[i].fops;
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d15a461fa84a..bf8fc274f067 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1136,8 +1136,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
intel_init_audio_hooks(dev_priv);
i915_gem_load_init(dev);
- intel_display_crc_init(dev);
-
i915_dump_device_info(dev_priv);
/* Not all pre-production machines fall into this category, only the
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7c81757f3a3f..a2d8e4eca502 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1661,21 +1661,6 @@ enum intel_pipe_crc_source {
INTEL_PIPE_CRC_SOURCE_MAX,
};
-struct intel_pipe_crc_entry {
- uint32_t frame;
- uint32_t crc[5];
-};
-
-#define INTEL_PIPE_CRC_ENTRIES_NR 128
-struct intel_pipe_crc {
- spinlock_t lock;
- bool opened; /* exclusive access to the result file */
- struct intel_pipe_crc_entry *entries;
- enum intel_pipe_crc_source source;
- int head, tail;
- wait_queue_head_t wq;
-};
-
struct i915_frontbuffer_tracking {
struct mutex lock;
@@ -1881,10 +1866,6 @@ struct drm_i915_private {
struct drm_crtc *pipe_to_crtc_mapping[I915_MAX_PIPES];
wait_queue_head_t pending_flip_queue;
-#ifdef CONFIG_DEBUG_FS
- struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
-#endif
-
/* dpll and cdclk state is protected by connection_mutex */
int num_shared_dpll;
struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
@@ -3600,11 +3581,9 @@ int i915_debugfs_init(struct drm_minor *minor);
void i915_debugfs_cleanup(struct drm_minor *minor);
#ifdef CONFIG_DEBUG_FS
int i915_debugfs_connector_add(struct drm_connector *connector);
-void intel_display_crc_init(struct drm_device *dev);
#else
static inline int i915_debugfs_connector_add(struct drm_connector *connector)
{ return 0; }
-static inline void intel_display_crc_init(struct drm_device *dev) {}
#endif
/* i915_gpu_error.c */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4378a659d962..7eb57b9658e4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1503,43 +1503,12 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
uint32_t crc2, uint32_t crc3,
uint32_t crc4)
{
- 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->dev, pipe);
+ uint32_t frame;
- spin_lock(&pipe_crc->lock);
+ frame = dev_priv->dev->driver->get_vblank_counter(dev_priv->dev, pipe);
- if (!pipe_crc->entries) {
- spin_unlock(&pipe_crc->lock);
- DRM_DEBUG_KMS("spurious interrupt\n");
- return;
- }
-
- 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;
- }
-
- entry = &pipe_crc->entries[head];
-
- entry->frame = dev_priv->dev->driver->get_vblank_counter(dev_priv->dev,
- 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;
-
- spin_unlock(&pipe_crc->lock);
-
- wake_up_interruptible(&pipe_crc->wq);
+ drm_crtc_add_crc_entry(crtc, frame, crc0, crc1, crc2, crc3, crc4);
}
#else
static inline void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b2cd669ac05..05816b998f80 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14011,6 +14011,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
.page_flip = drm_atomic_helper_page_flip,
.atomic_duplicate_state = intel_crtc_duplicate_state,
.atomic_destroy_state = intel_crtc_destroy_state,
+ .set_crc_source = intel_crtc_set_crc_source,
};
/**
@@ -14603,6 +14604,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
intel_color_init(&intel_crtc->base);
WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+
+ WARN_ON(intel_pipe_crc_create(&intel_crtc->base));
return;
fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5fdc426640f3..64d02baf6d4f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1744,8 +1744,8 @@ 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);
+int intel_pipe_crc_create(struct drm_crtc *crtc);
extern const struct file_operations i915_display_crc_ctl_fops;
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
#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 590e909bcf1b..933d33273468 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -32,12 +32,6 @@
#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.
*/
@@ -63,171 +57,43 @@ static int drm_add_fake_info_node(struct drm_minor *minor,
return 0;
}
-static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
+int intel_pipe_crc_create_for_minor(struct drm_crtc *crtc,
+ struct drm_minor *minor)
{
- 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);
+ struct dentry *ent;
+ char *name;
- return 0;
-}
+ name = kasprintf(GFP_KERNEL, "i915_pipe_%c_crc",
+ pipe_name(crtc->index));
+ if (!name)
+ return -ENOMEM;
-/* (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)
+ ent = debugfs_create_file(name, S_IRUGO, minor->debugfs_root, crtc,
+ &drm_crtc_crc_fops);
+ kfree(name);
+ if (!ent)
+ return PTR_ERR(ent);
-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);
+ return drm_add_fake_info_node(minor, ent, crtc);
}
-static ssize_t
-i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
- loff_t *pos)
+int intel_pipe_crc_create(struct drm_crtc *crtc)
{
- 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,
-};
+ int ret;
-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,
- },
-};
+ ret = intel_pipe_crc_create_for_minor(crtc, crtc->dev->control);
+ if (ret)
+ return ret;
-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];
+ ret = intel_pipe_crc_create_for_minor(crtc, crtc->dev->primary);
+ if (ret)
+ return ret;
- info->dev = dev;
- ent = debugfs_create_file(info->name, S_IRUGO, root, info,
- &i915_pipe_crc_fops);
- if (!ent)
- return -ENOMEM;
+ ret = intel_pipe_crc_create_for_minor(crtc, crtc->dev->render);
+ if (ret)
+ return ret;
- return drm_add_fake_info_node(minor, ent, info);
+ return 0;
}
static const char * const pipe_crc_sources[] = {
@@ -243,30 +109,19 @@ static const char * const pipe_crc_sources[] = {
"auto",
};
-static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
+static enum intel_pipe_crc_source pipe_crc_source_from_name(const char *name)
{
- 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));
+ unsigned int i;
- return 0;
-}
+ if (!name)
+ return INTEL_PIPE_CRC_SOURCE_NONE;
-static int display_crc_ctl_open(struct inode *inode, struct file *file)
-{
- struct drm_device *dev = inode->i_private;
+ for (i = 0; i < INTEL_PIPE_CRC_SOURCE_MAX; i++) {
+ if (!strcmp(name, pipe_crc_sources[i]))
+ return i;
+ }
- return single_open(file, display_crc_ctl_show, dev);
+ return -1;
}
static int i8xx_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
@@ -626,25 +481,21 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
return 0;
}
-static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
- enum intel_pipe_crc_source source)
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
{
+ struct drm_device *dev = crtc->dev;
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));
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum intel_pipe_crc_source source;
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)
+ source = pipe_crc_source_from_name(source_name);
+ if (source == -1)
return -EINVAL;
- power_domain = POWER_DOMAIN_PIPE(pipe);
+ 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;
@@ -653,84 +504,47 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
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);
+ ret = i9xx_pipe_crc_ctl_reg(dev, crtc->index, &source, &val);
else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
- ret = vlv_pipe_crc_ctl_reg(dev, pipe, &source, &val);
+ ret = vlv_pipe_crc_ctl_reg(dev, crtc->index, &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);
+ ret = ivb_pipe_crc_ctl_reg(dev, crtc->index, &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);
+ hsw_disable_ips(intel_crtc);
}
- pipe_crc->source = source;
-
- I915_WRITE(PIPE_CRC_CTL(pipe), val);
- POSTING_READ(PIPE_CRC_CTL(pipe));
+ I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+ POSTING_READ(PIPE_CRC_CTL(crtc->index));
- /* 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);
+ pipe_name(crtc->index));
- kfree(entries);
+ drm_modeset_lock(&crtc->mutex, NULL);
+ if (crtc->state->active)
+ intel_wait_for_vblank(dev, crtc->index);
+ drm_modeset_unlock(&crtc->mutex);
if (IS_G4X(dev))
- g4x_undo_pipe_scramble_reset(dev, pipe);
+ g4x_undo_pipe_scramble_reset(dev, crtc->index);
else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
- vlv_undo_pipe_scramble_reset(dev, pipe);
- else if (IS_HASWELL(dev) && pipe == PIPE_A)
+ vlv_undo_pipe_scramble_reset(dev, crtc->index);
+ else if (IS_HASWELL(dev) && crtc->index == PIPE_A)
hsw_trans_edp_pipe_A_crc_wa(dev, false);
- hsw_enable_ips(crtc);
+ hsw_enable_ips(intel_crtc);
}
ret = 0;
@@ -740,214 +554,3 @@ out:
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] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-21 11:06 ` [PATCH v1 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
@ 2016-06-21 15:07 ` Thierry Reding
2016-06-22 8:26 ` Tomeu Vizoso
2016-06-22 14:20 ` Daniel Vetter
1 sibling, 1 reply; 16+ messages in thread
From: Thierry Reding @ 2016-06-21 15:07 UTC (permalink / raw)
To: Tomeu Vizoso; +Cc: linux-kernel, Daniel Vetter, dri-devel, Emil Velikov
[-- Attachment #1: Type: text/plain, Size: 17660 bytes --]
On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
> +static int
> +drm_add_fake_info_node(struct drm_minor *minor,
> + struct dentry *ent,
> + const void *key)
Nit: this fits on two lines instead of four.
> +{
> + struct drm_info_node *node;
> +
> + node = kmalloc(sizeof(*node), GFP_KERNEL);
> + if (node == NULL) {
> + debugfs_remove(ent);
You already remove this in the caller upon returning an error, no need
to do it twice. The caller is where it should be removed.
> + 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;
> +}
Is there a specific reason why you use the drm_info_node infrastructure
here? Seems like it'd be simpler just doing plain debugfs.
> +
> +static int crc_control_show(struct seq_file *m, void *data)
> +{
> + struct drm_device *dev = m->private;
> + struct drm_crtc *crtc;
> +
> + drm_for_each_crtc(crtc, dev)
> + seq_printf(m, "crtc %d %s\n", crtc->index,
> + crtc->crc.source ? crtc->crc.source : "none");
> +
> + return 0;
> +}
Why are these control files not per-CRTC? I'd imagine you could do
something like control the CRC generation on writes and provide the
sampled CRCs on reads.
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, crc_control_show, dev);
> +}
> +
> +static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
> +{
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entries;
> + int ret;
> +
> + if (!strcmp(source, "none"))
Nit: I think it's more idiomatic to write the == 0 explicitly for
strcmp() for readability. My brain always interprets !strcmp() as
"strings are not equal", whereas == 0 is more explicit as in "the
difference between strings is 0". But perhaps that's just personal
taste.
> + source = NULL;
> +
> + if (!crc->source && !source)
> + return 0;
> +
> + if (crc->source && source && !strcmp(crc->source, source))
> + return 0;
> +
> + /* Forbid changing the source without going back to "none". */
> + if (crc->source && source)
> + return -EINVAL;
Why? It seems to me that if a driver doesn't support switching from one
source to another directly, then it should internally handle that. After
all the source parameter is already driver-specific, so it seems odd to
impose this kind of policy on it at this level.
> + if (!crtc->funcs->set_crc_source)
> + return -ENOTSUPP;
> +
> + if (source) {
> + entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
> + sizeof(crc->entries[0]),
> + GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> + spin_lock_irq(&crc->lock);
> + kfree(crc->entries);
> + crc->entries = entries;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(&crc->lock);
> + }
Why are we reallocating? Couldn't we simply allocate once and keep
reusing the existing array?
> +
> + ret = crtc->funcs->set_crc_source(crtc, source);
> + if (ret)
> + return ret;
> +
> + kfree(crc->source);
> + crc->source = source ? kstrdup(source, GFP_KERNEL) : NULL;
> +
> + if (!source) {
> + spin_lock_irq(&crc->lock);
> + entries = crc->entries;
> + crc->entries = NULL;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(&crc->lock);
> +
> + kfree(entries);
> + }
This frees the entries array after resetting source to "none", but what
if we never do that? Aren't we going to leak that data at CRTC removal?
> +static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)
unsigned int index?
> +{
> + struct drm_crtc *crtc;
> + int i = 0;
unsigned int?
> +
> + drm_for_each_crtc(crtc, dev)
> + if (i++ == index)
> + return crtc;
> +
> + return NULL;
> +}
This looks like a candidate for the core. I know that at least Tegra
implements a variant of this, and I think i915 does, too.
> +/*
> + * Parse CRC command strings:
> + * command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
Should the "(crtc | pipe)" in the above be "object"?
> + * object: ('crtc' | 'pipe')
Because you define that here?
> + * crtc: (0 | 1 | 2 | ...)
> + * pipe: (A | B | C)
> + * source: (none | plane1 | plane2 | ...)
I wouldn't provide "plane1 | plane2 |" here, since the parameter is
passed as-is to drivers, which may or may not support plane1 or plane2.
> + * wsp: (#0x20 | #0x9 | #0xA)+
> + *
> + * eg.:
> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
> + * "crtc 0 none" -> Stop CRC
I've said this above, but again, it seems odd to me that you'd have to
configure the CRC per-CRTC in one per-device file and read out the CRC
from per-CRTC files.
> + return n_words;
> +}
> +
> +static int crc_control_parse_crtc(const char *buf, unsigned int *crtc_index)
> +{
> + const char letter = buf[0];
> +
> + if (!kstrtouint(buf, 10, crtc_index))
Nit: Same comment as for !strcmp().
> + return 0;
> +
> + /* Backwards compatibility for Intel-style pipe letters */
> + if (letter < 'A' || letter > 'Z')
> + return -EINVAL;
> +
> + *crtc_index = letter - 'A';
> +
> + return 0;
> +}
Perhaps it would be more readable to write the above as:
err = kstrtouint(buf, 10, crtc_index);
if (err < 0) {
if (letter < 'A' || letter > 'Z') {
*crtc_index = letter - 'A';
err = 0;
}
}
return err;
> +static int crc_control_parse(struct drm_device *dev, char *buf, size_t len)
> +{
> +#define N_WORDS 3
> + int n_words;
unsigned int?
> + char *words[N_WORDS];
> + unsigned int crtc_index;
> + struct drm_crtc *crtc;
> +
> + n_words = crc_control_tokenize(buf, words, N_WORDS);
> + if (n_words != N_WORDS) {
> + DRM_DEBUG_KMS("tokenize failed, a command is %d words\n",
%u for unsigned int.
> + N_WORDS);
n_words
> + return -EINVAL;
> + }
> +
> + if (strcmp(words[0], "crtc") && strcmp(words[0], "pipe")) {
> + DRM_DEBUG_KMS("Invalid command %s\n", words[0]);
> + return -EINVAL;
> + }
> +
> + if (crc_control_parse_crtc(words[1], &crtc_index) < 0) {
> + DRM_DEBUG_KMS("Invalid CRTC index: %s\n", words[1]);
> + return -EINVAL;
> + }
Propagate the error from crc_control_parse_crtc()?
> +
> + crtc = crtc_from_index(dev, crtc_index);
> + if (!crtc) {
> + DRM_DEBUG_KMS("Unknown CRTC index: %d\n", crtc_index);
%u for unsigned int.
> + return -EINVAL;
> + }
> +
> + return crc_control_update_crtc(crtc, words[2]);
> +}
> +
> +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_device *dev = m->private;
> + char *tmpbuf;
> + int ret;
> +
> + if (len == 0)
> + return 0;
> +
> + if (len > PAGE_SIZE - 1) {
> + DRM_DEBUG_KMS("expected <%lu bytes into crtc crc control\n",
"CRTC" and "CRC", and perhaps a space after <.
> @@ -157,10 +413,23 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
> minor->debugfs_root, minor);
> if (ret) {
> - debugfs_remove(minor->debugfs_root);
> - minor->debugfs_root = NULL;
> DRM_ERROR("Failed to create core drm debugfs files\n");
> - return ret;
> + goto error_remove_dir;
I think we can do without the error_ prefix on these labels.
> +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;
> + char buf[CRC_BUFFER_LEN];
> + int n_entries;
unsigned int?
> + ssize_t bytes_read;
> +
> + /*
> + * Don't allow user space to provide buffers not big enough to hold
> + * a line of data.
> + */
> + if (count < CRC_LINE_LEN)
> + return -EINVAL;
> +
> + if (!crc->source)
> + return 0;
> +
> + /* Nothing to read? */
> + spin_lock_irq(&crc->lock);
> + while (crtc_crc_data_count(crc) == 0) {
> + int ret;
> +
> + 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 now have one or more entries to read */
> + n_entries = count / CRC_LINE_LEN;
> +
> + bytes_read = 0;
> + while (n_entries > 0) {
> + struct drm_crtc_crc_entry *entry =
> + &crc->entries[crc->tail];
Fits on one line.
> + int ret;
> +
> + if (CIRC_CNT(crc->head, crc->tail, DRM_CRTC_CRC_ENTRIES_NR) < 1)
> + break;
> +
> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRTC_CRC_ENTRIES_NR);
> + crc->tail = (crc->tail + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> +
> + bytes_read += snprintf(buf, CRC_BUFFER_LEN,
> + "%8u %8x %8x %8x %8x %8x\n",
uint32_t can be 9 characters long in decimal representation. Also I
think it'd be safer to go through a separate variable here, to avoid
inadvertantly subtracting from bytes_read. And then you can also make
the bytes_read variable size_t.
> + entry->frame, entry->crc[0],
> + entry->crc[1], entry->crc[2],
> + entry->crc[3], entry->crc[4]);
What about drivers that only support one uint32_t for the CRC? Do they
also need to output all unused uint32_t:s?
> +
> + spin_unlock_irq(&crc->lock);
Is it really safe to unlock here? I thought the whole point of this lock
was to prevent someone from reconfiguring the CRC mid-way, but after the
unlock above that's exactly what could happen.
> +
> + ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
> + if (ret == CRC_LINE_LEN)
> + return -EFAULT;
>
> + user_buf += CRC_LINE_LEN;
> + n_entries--;
> +
> + spin_lock_irq(&crc->lock);
> + }
> +
> + spin_unlock_irq(&crc->lock);
> +
> + return bytes_read;
> +}
> +
> +const struct file_operations drm_crtc_crc_fops = {
> + .owner = THIS_MODULE,
> + .open = crtc_crc_open,
> + .read = crtc_crc_read,
> + .release = crtc_crc_release,
> +};
Do we want to support poll?
> +
> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
> + struct drm_minor *minor)
> +{
> + struct dentry *ent;
> + char *name;
> +
> + if (!minor->debugfs_root)
> + return -1;
Can this ever happen? Perhaps turn this into a symbolic name if you
really need it.
> +
> + name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
> + if (!name)
> + return -ENOMEM;
I think it might be preferable to move this all into per-CRTC debugfs
directories, perhaps even collapse the "crc" and "control" files. But in
any case I think the drm_ prefix is redundant here and should be
dropped.
> + ent = debugfs_create_file(name, S_IRUGO, minor->debugfs_root, crtc,
> + &drm_crtc_crc_fops);
> + kfree(name);
> + if (!ent)
> + return PTR_ERR(ent);
> +
> + crtc->crc.debugfs_entries[minor->type] = ent;
> +
> + return 0;
> +}
> +
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> + int ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
> + if (ret)
> + return ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
> + if (ret)
> + return ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
Do we really want these for all minors? Is ->primary not enough? It
certainly seems completely misplaced in ->render, and I don't think
anything really uses ->control anymore.
Also I think you need to export this symbol.
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> + int i;
unsigned int
> +
> + for (i = 0; i < DRM_MINOR_CNT; i++) {
> + debugfs_remove_recursive(crtc->crc.debugfs_entries[i]);
> + crtc->crc.debugfs_entries[i] = NULL;
> + }
> +}
This also needs an export, I think.
> +
> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> + uint32_t crc0, uint32_t crc1, uint32_t crc2,
> + uint32_t crc3, uint32_t crc4)
Perhaps allow passing the CRC as an array with a count parameter? I can
imagine that a lot of hardware will only give you a single uint32_t for
the CRC, in which case you could do:
drm_crtc_add_crc_entry(crtc, frame, &crc, 1);
instead of:
drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);
It would probably save poor users of the interface, such as myself, a
lot of headaches because they can't remember how many uint32_t:s the
function needs.
> +{
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entry;
> + int head, tail;
unsigned int
> +
> + spin_lock(&crc->lock);
> +
> + head = crc->head;
> + tail = crc->tail;
> +
> + if (CIRC_SPACE(head, tail, DRM_CRTC_CRC_ENTRIES_NR) < 1) {
Perhaps "== 0"?
> + spin_unlock(&crc->lock);
> + DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> + return;
> + }
Maybe return an error here? And perhaps use some sort of printk rate
limiting here to avoid this from spamming logs?
> +
> + entry = &crc->entries[head];
> +
> + entry->frame = frame;
> + entry->crc[0] = crc0;
> + entry->crc[1] = crc1;
> + entry->crc[2] = crc2;
> + entry->crc[3] = crc3;
> + entry->crc[4] = crc4;
> +
> + head = (head + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> + crc->head = head;
> +
> + spin_unlock(&crc->lock);
> +
> + wake_up_interruptible(&crc->wq);
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 38401d406532..e5b124d937f5 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -100,6 +100,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);
Oh... this isn't something that drivers are supposed to call?
> #else
> static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> struct dentry *root)
> @@ -119,4 +121,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 084fd141e8bf..ec2f91c8b7cd 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1142,6 +1142,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 c2734979f164..141335a3c647 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -376,6 +376,22 @@ struct drm_crtc_state {
> struct drm_atomic_state *state;
> };
>
> +struct drm_crtc_crc_entry {
> + uint32_t frame;
> + uint32_t crc[5];
> +};
> +
> +#define DRM_CRTC_CRC_ENTRIES_NR 128
> +struct drm_crtc_crc {
> + spinlock_t lock;
> + const char *source;
> + bool opened; /* exclusive access to the result file */
You could probably have done this with an atomic and avoid the spin lock
for exclusive access, but it's probably not worth it.
> + struct drm_crtc_crc_entry *entries;
> + int head, tail;
unsigned int?
> + wait_queue_head_t wq;
> + struct dentry **debugfs_entries;
> +};
> +
> /**
> * struct drm_crtc_funcs - control CRTCs for a given device
> *
> @@ -704,6 +720,29 @@ struct drm_crtc_funcs {
> const struct drm_crtc_state *state,
> struct drm_property *property,
> uint64_t *val);
> +
> + /**
> + * @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.
Perhaps also mention that "none" is an alias for NULL?
> + *
> + * 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.
Would it be useful to provide some more aliases? "enable" and "on" for
"auto", "disable" and "off" for "none"?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-21 15:07 ` Thierry Reding
@ 2016-06-22 8:26 ` Tomeu Vizoso
2016-06-22 13:32 ` Thierry Reding
0 siblings, 1 reply; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-22 8:26 UTC (permalink / raw)
To: Thierry Reding; +Cc: Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
On 21 June 2016 at 17:07, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> [...]
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> [...]
>
>> +
>> +static int crc_control_show(struct seq_file *m, void *data)
>> +{
>> + struct drm_device *dev = m->private;
>> + struct drm_crtc *crtc;
>> +
>> + drm_for_each_crtc(crtc, dev)
>> + seq_printf(m, "crtc %d %s\n", crtc->index,
>> + crtc->crc.source ? crtc->crc.source : "none");
>> +
>> + return 0;
>> +}
>
> Why are these control files not per-CRTC? I'd imagine you could do
> something like control the CRC generation on writes and provide the
> sampled CRCs on reads.
We just thought there wasn't a strong point for breaking the existing
API in a fundamental way. The current proposal allows us to reuse more
code between the new and legacy CRC implementations in i915 and in
IGT.
>> + source = NULL;
>> +
>> + if (!crc->source && !source)
>> + return 0;
>> +
>> + if (crc->source && source && !strcmp(crc->source, source))
>> + return 0;
>> +
>> + /* Forbid changing the source without going back to "none". */
>> + if (crc->source && source)
>> + return -EINVAL;
>
> Why? It seems to me that if a driver doesn't support switching from one
> source to another directly, then it should internally handle that. After
> all the source parameter is already driver-specific, so it seems odd to
> impose this kind of policy on it at this level.
Hmm, I don't see when that would make sense for userspace. If
userspace has a source configured and changes directly to another, how
does it know what's the last CRC for the old source? I think that if
userspace does that it's shooting in its foot and it's good to give an
error.
>> +
>> + drm_for_each_crtc(crtc, dev)
>> + if (i++ == index)
>> + return crtc;
>> +
>> + return NULL;
>> +}
>
> This looks like a candidate for the core. I know that at least Tegra
> implements a variant of this, and I think i915 does, too.
And a few others. I would go this way but when I pinged danvet on irc
he didn't reply so I just went with the safe option.
>> +/*
>> + * Parse CRC command strings:
>> + * command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
>
> Should the "(crtc | pipe)" in the above be "object"?
In one case they are literals and in the other symbols.
>> + * object: ('crtc' | 'pipe')
>
> Because you define that here?
>
>> + * crtc: (0 | 1 | 2 | ...)
>> + * pipe: (A | B | C)
>> + * source: (none | plane1 | plane2 | ...)
>
> I wouldn't provide "plane1 | plane2 |" here, since the parameter is
> passed as-is to drivers, which may or may not support plane1 or plane2.
Agreed, feels more confusing than clarifying.
>> + * wsp: (#0x20 | #0x9 | #0xA)+
>> + *
>> + * eg.:
>> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
>> + * "crtc 0 none" -> Stop CRC
>
> I've said this above, but again, it seems odd to me that you'd have to
> configure the CRC per-CRTC in one per-device file and read out the CRC
> from per-CRTC files.
Not sure, I like that the per-crtc files just provide CRC data, and
that there's a separate control file that can be queried for the
current state.
>
>> + entry->frame, entry->crc[0],
>> + entry->crc[1], entry->crc[2],
>> + entry->crc[3], entry->crc[4]);
>
> What about drivers that only support one uint32_t for the CRC? Do they
> also need to output all unused uint32_t:s?
Yeah, do you think that could be a problem?
>> +
>> + ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
>> + if (ret == CRC_LINE_LEN)
>> + return -EFAULT;
>>
>> + user_buf += CRC_LINE_LEN;
>> + n_entries--;
>> +
>> + spin_lock_irq(&crc->lock);
>> + }
>> +
>> + spin_unlock_irq(&crc->lock);
>> +
>> + return bytes_read;
>> +}
>> +
>> +const struct file_operations drm_crtc_crc_fops = {
>> + .owner = THIS_MODULE,
>> + .open = crtc_crc_open,
>> + .read = crtc_crc_read,
>> + .release = crtc_crc_release,
>> +};
>
> Do we want to support poll?
Don't see the point of it, TBH.
>> +
>> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
>> + struct drm_minor *minor)
>> +{
>> + struct dentry *ent;
>> + char *name;
>> +
>> + if (!minor->debugfs_root)
>> + return -1;
>
> Can this ever happen? Perhaps turn this into a symbolic name if you
> really need it.
Sorry, can you explain what you mean by that?
>> +
>> + name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
>> + if (!name)
>> + return -ENOMEM;
>
> I think it might be preferable to move this all into per-CRTC debugfs
> directories, perhaps even collapse the "crc" and "control" files. But in
> any case I think the drm_ prefix is redundant here and should be
> dropped.
Yeah, I kind of like the redundancy as in the client code you will
only sometimes find the file name next to the directory name, but I
don't particularly care myself.
>> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
>> +{
>> + int ret;
>> +
>> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>
> Do we really want these for all minors? Is ->primary not enough? It
> certainly seems completely misplaced in ->render, and I don't think
> anything really uses ->control anymore.
Agreed.
>> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
>> + uint32_t crc0, uint32_t crc1, uint32_t crc2,
>> + uint32_t crc3, uint32_t crc4)
>
> Perhaps allow passing the CRC as an array with a count parameter? I can
> imagine that a lot of hardware will only give you a single uint32_t for
> the CRC, in which case you could do:
>
> drm_crtc_add_crc_entry(crtc, frame, &crc, 1);
>
> instead of:
>
> drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);
>
> It would probably save poor users of the interface, such as myself, a
> lot of headaches because they can't remember how many uint32_t:s the
> function needs.
Sounds good to me, I don't really know how common multiple sources of
complex CRC data will be in the future.
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 38401d406532..e5b124d937f5 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -100,6 +100,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);
>
> Oh... this isn't something that drivers are supposed to call?
Right, it's analogous to drm_debugfs_connector_add.
>> + *
>> + * 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.
>
> Would it be useful to provide some more aliases? "enable" and "on" for
> "auto", "disable" and "off" for "none"?
Not sure, TBH, i like to keep my interfaces simple. Do you think this
could be helpful?
Thanks for the great review!
Tomeu
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 8:26 ` Tomeu Vizoso
@ 2016-06-22 13:32 ` Thierry Reding
2016-06-22 14:08 ` Daniel Vetter
2016-06-22 14:12 ` Daniel Vetter
0 siblings, 2 replies; 16+ messages in thread
From: Thierry Reding @ 2016-06-22 13:32 UTC (permalink / raw)
To: Tomeu Vizoso; +Cc: Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
[-- Attachment #1: Type: text/plain, Size: 11426 bytes --]
On Wed, Jun 22, 2016 at 10:26:36AM +0200, Tomeu Vizoso wrote:
> On 21 June 2016 at 17:07, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> > [...]
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > [...]
> >
> >> +
> >> +static int crc_control_show(struct seq_file *m, void *data)
> >> +{
> >> + struct drm_device *dev = m->private;
> >> + struct drm_crtc *crtc;
> >> +
> >> + drm_for_each_crtc(crtc, dev)
> >> + seq_printf(m, "crtc %d %s\n", crtc->index,
> >> + crtc->crc.source ? crtc->crc.source : "none");
> >> +
> >> + return 0;
> >> +}
> >
> > Why are these control files not per-CRTC? I'd imagine you could do
> > something like control the CRC generation on writes and provide the
> > sampled CRCs on reads.
>
> We just thought there wasn't a strong point for breaking the existing
> API in a fundamental way. The current proposal allows us to reuse more
> code between the new and legacy CRC implementations in i915 and in
> IGT.
This is pretty much the last chance to make changes to this interface,
so I think it'd be good to spend at least some time thinking about it
and if there's anything that could be improved.
> >> + source = NULL;
> >> +
> >> + if (!crc->source && !source)
> >> + return 0;
> >> +
> >> + if (crc->source && source && !strcmp(crc->source, source))
> >> + return 0;
> >> +
> >> + /* Forbid changing the source without going back to "none". */
> >> + if (crc->source && source)
> >> + return -EINVAL;
> >
> > Why? It seems to me that if a driver doesn't support switching from one
> > source to another directly, then it should internally handle that. After
> > all the source parameter is already driver-specific, so it seems odd to
> > impose this kind of policy on it at this level.
>
> Hmm, I don't see when that would make sense for userspace. If
> userspace has a source configured and changes directly to another, how
> does it know what's the last CRC for the old source? I think that if
> userspace does that it's shooting in its foot and it's good to give an
> error.
You do clear the ring buffer when the configured source is changed,
right? If so, then userspace would automatically get the new CRC upon
the next read. How is that different than switching via the "none"
source?
> >> +
> >> + drm_for_each_crtc(crtc, dev)
> >> + if (i++ == index)
> >> + return crtc;
> >> +
> >> + return NULL;
> >> +}
> >
> > This looks like a candidate for the core. I know that at least Tegra
> > implements a variant of this, and I think i915 does, too.
>
> And a few others. I would go this way but when I pinged danvet on irc
> he didn't reply so I just went with the safe option.
It could be a follow up patch, I don't mind much either way.
> >> +/*
> >> + * Parse CRC command strings:
> >> + * command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
> >
> > Should the "(crtc | pipe)" in the above be "object"?
>
> In one case they are literals and in the other symbols.
>
> >> + * object: ('crtc' | 'pipe')
> >
> > Because you define that here?
Right, I see now.
> >> + * wsp: (#0x20 | #0x9 | #0xA)+
> >> + *
> >> + * eg.:
> >> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
> >> + * "crtc 0 none" -> Stop CRC
> >
> > I've said this above, but again, it seems odd to me that you'd have to
> > configure the CRC per-CRTC in one per-device file and read out the CRC
> > from per-CRTC files.
>
> Not sure, I like that the per-crtc files just provide CRC data, and
> that there's a separate control file that can be queried for the
> current state.
In my opinion that makes things needlessly complicated for userspace. If
you want to query the state of a specific CRTC, you have to read out the
entire file and parse each line to find the correct CRTC. On the other
hand, chances are that you already need to know the path to the CRTC
because you want to read the CRC out of the per-CRTC CRC file. In that
case it would be much easier to simply concatenate the CRTC path and the
CRC (or control) filename and read a single line (actually a single
word) out of it to get at the same information.
Furthermore if you have everything per-CRTC you no longer have to worry
about pipe vs. index (that's always confusing because in the DRM core
they're actually synonymous) because the CRTC path is canonical and will
have the correct context.
Per-CRTC directory with a single duplex file, or separate control and
CRC files, is much simpler than the mix proposed here. No tokenization
required when parsing in userspace, and no tokenization required to
parse in the kernel either.
> >> + entry->frame, entry->crc[0],
> >> + entry->crc[1], entry->crc[2],
> >> + entry->crc[3], entry->crc[4]);
> >
> > What about drivers that only support one uint32_t for the CRC? Do they
> > also need to output all unused uint32_t:s?
>
> Yeah, do you think that could be a problem?
I worry slightly about the flexibility of this. On one hand it's
redundant to have to read 5 values if you only get one that's valid.
It's also ambiguous if you receive one value and 4 zeroes, because
zero could technically be a valid CRC.
Furthermore what if some hardware needed to provide more than five
values? One simple solution would be to make the number of values per
entry variable (and perhaps even include the count as first value in
the debugfs file).
On that note: what if there's hardware that doesn't fit into the above
format at all? I can't immediately think of anything, but that's not a
guarantee that it won't happen. I suppose we have to settle on something
for the sake of genericity, so one option would be for eccentric drivers
to encode whatever format they have into uint32_t:s. Also, anything that
doesn't fit into a couple of uint32_t:s could well be considered not a
CRC at all.
> >> +
> >> + ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
> >> + if (ret == CRC_LINE_LEN)
> >> + return -EFAULT;
> >>
> >> + user_buf += CRC_LINE_LEN;
> >> + n_entries--;
> >> +
> >> + spin_lock_irq(&crc->lock);
> >> + }
> >> +
> >> + spin_unlock_irq(&crc->lock);
> >> +
> >> + return bytes_read;
> >> +}
> >> +
> >> +const struct file_operations drm_crtc_crc_fops = {
> >> + .owner = THIS_MODULE,
> >> + .open = crtc_crc_open,
> >> + .read = crtc_crc_read,
> >> + .release = crtc_crc_release,
> >> +};
> >
> > Do we want to support poll?
>
> Don't see the point of it, TBH.
I have difficulty coming up with a concrete use-case, but given that we
have most of the infrastructure to support it already, it might be nice
to have. Could of course be added later on if there's a need.
> >> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
> >> + struct drm_minor *minor)
> >> +{
> >> + struct dentry *ent;
> >> + char *name;
> >> +
> >> + if (!minor->debugfs_root)
> >> + return -1;
> >
> > Can this ever happen? Perhaps turn this into a symbolic name if you
> > really need it.
>
> Sorry, can you explain what you mean by that?
I was referring to -1 not being a proper error code. Although I also
questioned whether the check is even necessary. Under what circumstances
will minor->debugfs_root be NULL at this point?
> >> +
> >> + name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
> >> + if (!name)
> >> + return -ENOMEM;
> >
> > I think it might be preferable to move this all into per-CRTC debugfs
> > directories, perhaps even collapse the "crc" and "control" files. But in
> > any case I think the drm_ prefix is redundant here and should be
> > dropped.
>
> Yeah, I kind of like the redundancy as in the client code you will
> only sometimes find the file name next to the directory name, but I
> don't particularly care myself.
Any client code dealing with display CRCs is going to be fairly DRM/KMS
specific, no? As such the use of the drm_ prefix is pretty redundant no
matter what.
> >> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> >> + uint32_t crc0, uint32_t crc1, uint32_t crc2,
> >> + uint32_t crc3, uint32_t crc4)
> >
> > Perhaps allow passing the CRC as an array with a count parameter? I can
> > imagine that a lot of hardware will only give you a single uint32_t for
> > the CRC, in which case you could do:
> >
> > drm_crtc_add_crc_entry(crtc, frame, &crc, 1);
> >
> > instead of:
> >
> > drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);
> >
> > It would probably save poor users of the interface, such as myself, a
> > lot of headaches because they can't remember how many uint32_t:s the
> > function needs.
>
> Sounds good to me, I don't really know how common multiple sources of
> complex CRC data will be in the future.
When you mention multiple sources, are the additional values meant to be
CRCs for additional sources? Or is it that some hardware generates more
than 32 bits for the CRC and drivers are supposed to split them up into
several values.
>
> >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> >> index 38401d406532..e5b124d937f5 100644
> >> --- a/drivers/gpu/drm/drm_internal.h
> >> +++ b/drivers/gpu/drm/drm_internal.h
> >> @@ -100,6 +100,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);
> >
> > Oh... this isn't something that drivers are supposed to call?
>
> Right, it's analogous to drm_debugfs_connector_add.
I recall seeing calls to this function from within the i915 driver,
wouldn't that cause the build to fail if i915 was built as a loadable
module?
> >> + *
> >> + * 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.
> >
> > Would it be useful to provide some more aliases? "enable" and "on" for
> > "auto", "disable" and "off" for "none"?
>
> Not sure, TBH, i like to keep my interfaces simple. Do you think this
> could be helpful?
I don't know. It's probably okay to not have the aliases, and it should
be trivial to add them later on if we see that people get annoyed by the
fact that they have to write "auto" instead of "on" or "enable".
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 13:32 ` Thierry Reding
@ 2016-06-22 14:08 ` Daniel Vetter
2016-06-22 14:31 ` Thierry Reding
2016-06-22 14:12 ` Daniel Vetter
1 sibling, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2016-06-22 14:08 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomeu Vizoso, Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> >> + * wsp: (#0x20 | #0x9 | #0xA)+
>> >> + *
>> >> + * eg.:
>> >> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
>> >> + * "crtc 0 none" -> Stop CRC
>> >
>> > I've said this above, but again, it seems odd to me that you'd have to
>> > configure the CRC per-CRTC in one per-device file and read out the CRC
>> > from per-CRTC files.
>>
>> Not sure, I like that the per-crtc files just provide CRC data, and
>> that there's a separate control file that can be queried for the
>> current state.
>
> In my opinion that makes things needlessly complicated for userspace. If
> you want to query the state of a specific CRTC, you have to read out the
> entire file and parse each line to find the correct CRTC. On the other
> hand, chances are that you already need to know the path to the CRTC
> because you want to read the CRC out of the per-CRTC CRC file. In that
> case it would be much easier to simply concatenate the CRTC path and the
> CRC (or control) filename and read a single line (actually a single
> word) out of it to get at the same information.
>
> Furthermore if you have everything per-CRTC you no longer have to worry
> about pipe vs. index (that's always confusing because in the DRM core
> they're actually synonymous) because the CRTC path is canonical and will
> have the correct context.
>
> Per-CRTC directory with a single duplex file, or separate control and
> CRC files, is much simpler than the mix proposed here. No tokenization
> required when parsing in userspace, and no tokenization required to
> parse in the kernel either.
Just jumping on this one here. I agree that if we remodel the
interface making the control file per-crtc would make sense. I think
separate control and read files makes sense, that's much less magic.
And by reading the control file you can check what's the currently
selected source easily.
I'm not sure on the canonical CRTC path - right now we don't have that
in debugfs. I think just using index numbers is ok, we use those all
over the place already. Or maybe we could indeed add a new per-crtc
subdir in debugfs for this. Either way is fine with me.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 13:32 ` Thierry Reding
2016-06-22 14:08 ` Daniel Vetter
@ 2016-06-22 14:12 ` Daniel Vetter
1 sibling, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2016-06-22 14:12 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomeu Vizoso, Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> >> +const struct file_operations drm_crtc_crc_fops = {
>> >> + .owner = THIS_MODULE,
>> >> + .open = crtc_crc_open,
>> >> + .read = crtc_crc_read,
>> >> + .release = crtc_crc_release,
>> >> +};
>> >
>> > Do we want to support poll?
>>
>> Don't see the point of it, TBH.
>
> I have difficulty coming up with a concrete use-case, but given that we
> have most of the infrastructure to support it already, it might be nice
> to have. Could of course be added later on if there's a need.
O_NONBLOCK is definitely needed to be able to write testcases, so that
you can schedule flips and collect CRCs for them in parallel (e.g. to
validate atomic flips by making sure there's never an intermediate CRC
with garbage). I haven't seen a need yet for poll, and like you say
it's easy to add if we ever need it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-21 11:06 ` [PATCH v1 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-06-21 15:07 ` Thierry Reding
@ 2016-06-22 14:20 ` Daniel Vetter
1 sibling, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2016-06-22 14:20 UTC (permalink / raw)
To: Tomeu Vizoso; +Cc: linux-kernel, Daniel Vetter, dri-devel, Emil Velikov
On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> Adds a per-device debugfile "drm_crc_control" that allows selecting a
> source for frame checksums in each CRTC that supports them.
>
> The checksums for each subsequent frame can be read from the per-CRTC
> file "drm_crtc_N_crc".
>
> The code is taken from the i915 driver and other drivers can now provide
> frame CRCs by implementing the set_crc_source callback in
> drm_crtc_funcs.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> drivers/gpu/drm/drm_crtc.c | 28 ++-
> drivers/gpu/drm/drm_debugfs.c | 506 ++++++++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/drm_internal.h | 10 +
> include/drm/drmP.h | 5 +
> include/drm/drm_crtc.h | 72 ++++++
> 5 files changed, 611 insertions(+), 10 deletions(-)
I think we should finalize the internal and external api first, but this
needs a bit better documentation. For that I think it'd be good to extract
this to a new file like drm_debugfs_crc.[hc] and pull that into a new
section (maybe under the testing and validation section, next to the igt
howto) in drm-uapi.rst.
More doc bikeshedding below.
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e7c862bd2f19..4dae42b122d9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -657,8 +657,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> drm_num_crtcs(dev));
> }
> if (!crtc->name) {
> - drm_mode_object_unregister(dev, &crtc->base);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto err_unregister;
> }
>
> crtc->base.properties = &crtc->properties;
> @@ -673,12 +673,30 @@ 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.debugfs_entries = kmalloc_array(DRM_MINOR_CNT,
> + sizeof(*crtc->crc.debugfs_entries),
> + GFP_KERNEL);
> +
> + ret = drm_debugfs_crtc_add(crtc);
> + if (ret)
> + goto err_free_name;
> +#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);
> }
>
> return 0;
> +
> +err_free_name:
> + kfree(crtc->name);
> +err_unregister:
> + drm_mode_object_unregister(dev, &crtc->base);
> + return ret;
> }
> EXPORT_SYMBOL(drm_crtc_init_with_planes);
>
> @@ -699,6 +717,12 @@ 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.debugfs_entries);
> + 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..cdc8836bc22a 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -30,6 +30,8 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> */
>
> +#include <linux/circ_buf.h>
> +#include <linux/ctype.h>
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> @@ -127,6 +129,259 @@ fail:
> }
> EXPORT_SYMBOL(drm_debugfs_create_files);
>
> +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 crc_control_show(struct seq_file *m, void *data)
> +{
> + struct drm_device *dev = m->private;
> + struct drm_crtc *crtc;
> +
> + drm_for_each_crtc(crtc, dev)
> + seq_printf(m, "crtc %d %s\n", crtc->index,
> + crtc->crc.source ? crtc->crc.source : "none");
> +
> + return 0;
> +}
> +
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, crc_control_show, dev);
> +}
> +
> +static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
> +{
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entries;
> + int ret;
> +
> + if (!strcmp(source, "none"))
> + source = NULL;
> +
> + if (!crc->source && !source)
> + return 0;
> +
> + if (crc->source && source && !strcmp(crc->source, source))
> + return 0;
> +
> + /* Forbid changing the source without going back to "none". */
> + if (crc->source && source)
> + return -EINVAL;
> +
> + if (!crtc->funcs->set_crc_source)
> + return -ENOTSUPP;
> +
> + if (source) {
> + entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
> + sizeof(crc->entries[0]),
> + GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> + spin_lock_irq(&crc->lock);
> + kfree(crc->entries);
> + crc->entries = entries;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(&crc->lock);
> + }
> +
> + ret = crtc->funcs->set_crc_source(crtc, source);
> + if (ret)
> + return ret;
> +
> + kfree(crc->source);
> + crc->source = source ? kstrdup(source, GFP_KERNEL) : NULL;
> +
> + if (!source) {
> + spin_lock_irq(&crc->lock);
> + entries = crc->entries;
> + crc->entries = NULL;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(&crc->lock);
> +
> + kfree(entries);
> + }
> +
> + return 0;
> +}
> +
> +static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)
> +{
> + struct drm_crtc *crtc;
> + int i = 0;
> +
> + drm_for_each_crtc(crtc, dev)
> + if (i++ == index)
> + return crtc;
> +
> + return NULL;
> +}
> +
> +/*
> + * Parse CRC command strings:
> + * command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
> + * object: ('crtc' | 'pipe')
> + * crtc: (0 | 1 | 2 | ...)
> + * pipe: (A | B | C)
> + * source: (none | plane1 | plane2 | ...)
> + * wsp: (#0x20 | #0x9 | #0xA)+
> + *
> + * eg.:
> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
> + * "crtc 0 none" -> Stop CRC
> + */
> +static int crc_control_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_KMS("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;
> +}
> +
> +static int crc_control_parse_crtc(const char *buf, unsigned int *crtc_index)
> +{
> + const char letter = buf[0];
> +
> + if (!kstrtouint(buf, 10, crtc_index))
> + return 0;
> +
> + /* Backwards compatibility for Intel-style pipe letters */
> + if (letter < 'A' || letter > 'Z')
> + return -EINVAL;
> +
> + *crtc_index = letter - 'A';
> +
> + return 0;
> +}
> +
> +static int crc_control_parse(struct drm_device *dev, char *buf, size_t len)
> +{
> +#define N_WORDS 3
> + int n_words;
> + char *words[N_WORDS];
> + unsigned int crtc_index;
> + struct drm_crtc *crtc;
> +
> + n_words = crc_control_tokenize(buf, words, N_WORDS);
> + if (n_words != N_WORDS) {
> + DRM_DEBUG_KMS("tokenize failed, a command is %d words\n",
> + N_WORDS);
> + return -EINVAL;
> + }
> +
> + if (strcmp(words[0], "crtc") && strcmp(words[0], "pipe")) {
> + DRM_DEBUG_KMS("Invalid command %s\n", words[0]);
> + return -EINVAL;
> + }
> +
> + if (crc_control_parse_crtc(words[1], &crtc_index) < 0) {
> + DRM_DEBUG_KMS("Invalid CRTC index: %s\n", words[1]);
> + return -EINVAL;
> + }
> +
> + crtc = crtc_from_index(dev, crtc_index);
> + if (!crtc) {
> + DRM_DEBUG_KMS("Unknown CRTC index: %d\n", crtc_index);
> + return -EINVAL;
> + }
> +
> + return crc_control_update_crtc(crtc, words[2]);
> +}
> +
> +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_device *dev = m->private;
> + char *tmpbuf;
> + int ret;
> +
> + 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;
> + }
> +
> + 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 = crc_control_parse(dev, tmpbuf, len);
> +out:
> + kfree(tmpbuf);
> + if (ret < 0)
> + return ret;
> +
> + *offp += len;
> + return len;
> +}
> +
> +const struct file_operations drm_crc_control_fops = {
> + .owner = THIS_MODULE,
> + .open = crc_control_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .write = crc_control_write
> +};
> +
> /**
> * Initialize the DRI debugfs filesystem for a device
> *
> @@ -142,8 +397,9 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> struct dentry *root)
> {
> struct drm_device *dev = minor->dev;
> + struct dentry *ent;
> char name[64];
> - int ret;
> + int ret = 0;
>
> INIT_LIST_HEAD(&minor->debugfs_list);
> mutex_init(&minor->debugfs_lock);
> @@ -157,10 +413,23 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
> minor->debugfs_root, minor);
> if (ret) {
> - debugfs_remove(minor->debugfs_root);
> - minor->debugfs_root = NULL;
> DRM_ERROR("Failed to create core drm debugfs files\n");
> - return ret;
> + goto error_remove_dir;
> + }
> +
> + ent = debugfs_create_file("drm_crc_control", S_IRUGO | S_IWUSR,
> + minor->debugfs_root, dev,
> + &drm_crc_control_fops);
> + if (!ent) {
> + DRM_ERROR("Failed to create CRC control debugfs files\n");
> + ret = -ENOMEM;
> + goto error_remove_files;
> + }
> +
> + ret = drm_add_fake_info_node(minor, ent, &drm_crc_control_fops);
> + if (ret) {
> + DRM_ERROR("Failed to create CRC control debugfs files\n");
> + goto error_remove_crc;
> }
>
> if (dev->driver->debugfs_init) {
> @@ -171,7 +440,18 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> return ret;
> }
> }
> +
> return 0;
> +
> +error_remove_crc:
> + debugfs_remove(ent);
> +error_remove_files:
> + drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor);
> +error_remove_dir:
> + debugfs_remove(minor->debugfs_root);
> + minor->debugfs_root = NULL;
> +
> + return ret;
> }
>
>
> @@ -407,13 +687,223 @@ error:
>
> void drm_debugfs_connector_remove(struct drm_connector *connector)
> {
> - if (!connector->debugfs_entry)
> - return;
> -
> debugfs_remove_recursive(connector->debugfs_entry);
>
> connector->debugfs_entry = NULL;
> }
>
> -#endif /* CONFIG_DEBUG_FS */
> +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;
> +
> + spin_lock_irq(&crc->lock);
> +
> + if (crc->opened) {
> + spin_unlock_irq(&crc->lock);
> + return -EBUSY;
> + }
> +
> + crc->opened = true;
> +
> + spin_unlock_irq(&crc->lock);
> +
> + return 0;
> +}
> +
> +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;
> +
> + spin_lock_irq(&crc->lock);
> + crc->opened = false;
> + spin_unlock_irq(&crc->lock);
> +
> + return 0;
> +}
> +
> +/* (6 fields, 8 chars each, space separated (5) + '\n') */
> +#define CRC_LINE_LEN (6 * 8 + 5 + 1)
> +/* account for \'0' */
> +#define CRC_BUFFER_LEN (CRC_LINE_LEN + 1)
> +
> +static int crtc_crc_data_count(struct drm_crtc_crc *crc)
> +{
> + assert_spin_locked(&crc->lock);
> + return CIRC_CNT(crc->head, crc->tail,
> + DRM_CRTC_CRC_ENTRIES_NR);
> +}
> +
> +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;
> + char buf[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 < CRC_LINE_LEN)
> + return -EINVAL;
> +
> + if (!crc->source)
> + return 0;
> +
> + /* Nothing to read? */
> + spin_lock_irq(&crc->lock);
> + while (crtc_crc_data_count(crc) == 0) {
> + int ret;
> +
> + 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 now have one or more entries to read */
> + n_entries = count / CRC_LINE_LEN;
> +
> + bytes_read = 0;
> + while (n_entries > 0) {
> + struct drm_crtc_crc_entry *entry =
> + &crc->entries[crc->tail];
> + int ret;
> +
> + if (CIRC_CNT(crc->head, crc->tail, DRM_CRTC_CRC_ENTRIES_NR) < 1)
> + break;
> +
> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRTC_CRC_ENTRIES_NR);
> + crc->tail = (crc->tail + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> +
> + bytes_read += snprintf(buf, 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(&crc->lock);
> +
> + ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
> + if (ret == CRC_LINE_LEN)
> + return -EFAULT;
>
> + user_buf += CRC_LINE_LEN;
> + n_entries--;
> +
> + spin_lock_irq(&crc->lock);
> + }
> +
> + spin_unlock_irq(&crc->lock);
> +
> + return bytes_read;
> +}
> +
> +const struct file_operations drm_crtc_crc_fops = {
> + .owner = THIS_MODULE,
> + .open = crtc_crc_open,
> + .read = crtc_crc_read,
> + .release = crtc_crc_release,
> +};
> +
> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
> + struct drm_minor *minor)
> +{
> + struct dentry *ent;
> + char *name;
> +
> + if (!minor->debugfs_root)
> + return -1;
> +
> + name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
I'd check for set_crc_source here and just not add the file if it's not
set. Seems a bit silly to expose an interface when it's not there.
-Daniel
> + if (!name)
> + return -ENOMEM;
> +
> + ent = debugfs_create_file(name, S_IRUGO, minor->debugfs_root, crtc,
> + &drm_crtc_crc_fops);
> + kfree(name);
> + if (!ent)
> + return PTR_ERR(ent);
> +
> + crtc->crc.debugfs_entries[minor->type] = ent;
> +
> + return 0;
> +}
> +
> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +{
> + int ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
> + if (ret)
> + return ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
> + if (ret)
> + return ret;
> +
> + ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> +{
> + int i;
> +
> + for (i = 0; i < DRM_MINOR_CNT; i++) {
> + debugfs_remove_recursive(crtc->crc.debugfs_entries[i]);
> + crtc->crc.debugfs_entries[i] = NULL;
> + }
> +}
> +
> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> + uint32_t crc0, uint32_t crc1, uint32_t crc2,
> + uint32_t crc3, uint32_t crc4)
> +{
> + struct drm_crtc_crc *crc = &crtc->crc;
> + struct drm_crtc_crc_entry *entry;
> + int head, tail;
> +
> + spin_lock(&crc->lock);
> +
> + head = crc->head;
> + tail = crc->tail;
> +
> + if (CIRC_SPACE(head, tail, DRM_CRTC_CRC_ENTRIES_NR) < 1) {
> + spin_unlock(&crc->lock);
> + DRM_ERROR("Overflow of CRC buffer, userspace reads too slow.\n");
> + return;
> + }
> +
> + entry = &crc->entries[head];
> +
> + entry->frame = frame;
> + entry->crc[0] = crc0;
> + entry->crc[1] = crc1;
> + entry->crc[2] = crc2;
> + entry->crc[3] = crc3;
> + entry->crc[4] = crc4;
> +
> + head = (head + 1) & (DRM_CRTC_CRC_ENTRIES_NR - 1);
> + crc->head = head;
> +
> + spin_unlock(&crc->lock);
> +
> + wake_up_interruptible(&crc->wq);
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 38401d406532..e5b124d937f5 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -100,6 +100,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)
> @@ -119,4 +121,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 084fd141e8bf..ec2f91c8b7cd 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1142,6 +1142,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 c2734979f164..141335a3c647 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -376,6 +376,22 @@ struct drm_crtc_state {
> struct drm_atomic_state *state;
> };
>
> +struct drm_crtc_crc_entry {
> + uint32_t frame;
> + uint32_t crc[5];
> +};
> +
> +#define DRM_CRTC_CRC_ENTRIES_NR 128
> +struct drm_crtc_crc {
> + spinlock_t lock;
> + const char *source;
> + bool opened; /* exclusive access to the result file */
> + struct drm_crtc_crc_entry *entries;
> + int head, tail;
> + wait_queue_head_t wq;
> + struct dentry **debugfs_entries;
> +};
I think the above two should be moved into drm_debugfs_crc.h and also
documented with kerneldoc.
> +
> /**
> * struct drm_crtc_funcs - control CRTCs for a given device
> *
> @@ -704,6 +720,29 @@ struct drm_crtc_funcs {
> const struct drm_crtc_state *state,
> struct drm_property *property,
> uint64_t *val);
> +
> + /**
> + * @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);
> };
>
> /**
> @@ -817,6 +856,15 @@ struct drm_crtc {
> * context.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> +#ifdef CONFIG_DEBUG_FS
> + /**
> + * @drm_crtc_crc:
> + *
> + * Configuration settings of CRC capture.
> + */
> + struct drm_crtc_crc crc;
> +#endif
> };
>
> /**
> @@ -2496,6 +2544,30 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
> return 1 << drm_crtc_index(crtc);
> }
>
> +/**
> + * drm_crtc_add_crc_entry - Add entry with CRC information for a frame
> + * @crtc: CRTC to which the frame belongs
> + * @frame: number of the frame characterized by the CRC data
> + * @crc0: piece of data about frame
> + * @crc1: piece of data about frame
> + * @crc2: piece of data about frame
> + * @crc3: piece of data about frame
> + * @crc4: piece of data about frame
> + *
> + * 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.
> + */
Generally we put the kerneldoc for functions into the source file. Only
static inlines and structures are documented in the headers.
> +#if defined(CONFIG_DEBUG_FS)
> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> + uint32_t crc0, uint32_t crc1, uint32_t crc2,
> + uint32_t crc3, uint32_t crc4);
> +#else
> +static inline void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
> + uint32_t crc0, uint32_t crc1,
> + uint32_t crc2, uint32_t crc3,
> + uint32_t crc4) {}
> +#endif
> +
> extern void drm_connector_ida_init(void);
> extern void drm_connector_ida_destroy(void);
> extern int drm_connector_init(struct drm_device *dev,
> --
> 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] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 14:08 ` Daniel Vetter
@ 2016-06-22 14:31 ` Thierry Reding
2016-06-22 14:38 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Thierry Reding @ 2016-06-22 14:31 UTC (permalink / raw)
To: Daniel Vetter
Cc: Tomeu Vizoso, Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]
On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote:
> On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >> >> + * wsp: (#0x20 | #0x9 | #0xA)+
> >> >> + *
> >> >> + * eg.:
> >> >> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
> >> >> + * "crtc 0 none" -> Stop CRC
> >> >
> >> > I've said this above, but again, it seems odd to me that you'd have to
> >> > configure the CRC per-CRTC in one per-device file and read out the CRC
> >> > from per-CRTC files.
> >>
> >> Not sure, I like that the per-crtc files just provide CRC data, and
> >> that there's a separate control file that can be queried for the
> >> current state.
> >
> > In my opinion that makes things needlessly complicated for userspace. If
> > you want to query the state of a specific CRTC, you have to read out the
> > entire file and parse each line to find the correct CRTC. On the other
> > hand, chances are that you already need to know the path to the CRTC
> > because you want to read the CRC out of the per-CRTC CRC file. In that
> > case it would be much easier to simply concatenate the CRTC path and the
> > CRC (or control) filename and read a single line (actually a single
> > word) out of it to get at the same information.
> >
> > Furthermore if you have everything per-CRTC you no longer have to worry
> > about pipe vs. index (that's always confusing because in the DRM core
> > they're actually synonymous) because the CRTC path is canonical and will
> > have the correct context.
> >
> > Per-CRTC directory with a single duplex file, or separate control and
> > CRC files, is much simpler than the mix proposed here. No tokenization
> > required when parsing in userspace, and no tokenization required to
> > parse in the kernel either.
>
> Just jumping on this one here. I agree that if we remodel the
> interface making the control file per-crtc would make sense. I think
> separate control and read files makes sense, that's much less magic.
Agreed, separate files would be a little simpler. I must admit that my
proposal is partially motivated by a desire to avoid cumbersome naming
of files. If we have separate files, what do you name them? crc for
reading, crc_control for writing? crc_values for reading and crc for
writing?
Perhaps another way to avoid that would be to put the two files into a
separate directory, as in:
/sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
+-- control
+-- data
That's slightly on the deeply nested side, but on the other hand it
nicely uses the filesystem for namespacing, which is what filesystems
are really good at.
> And by reading the control file you can check what's the currently
> selected source easily.
Is that really a useful feature? If you're going to capture CRCs, you
likely just want to set whatever you expect to receive irrespective of
the current setting.
> I'm not sure on the canonical CRTC path - right now we don't have that
> in debugfs. I think just using index numbers is ok, we use those all
> over the place already. Or maybe we could indeed add a new per-crtc
> subdir in debugfs for this. Either way is fine with me.
I can imagine that we'd like to expose a number of other per-CRTC
properties (name, parts of the state, object ID, one day perhaps VBLANK
counts, ...) this way, so a per-CRTC directory makes a lot of sense in
my opinion.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 14:31 ` Thierry Reding
@ 2016-06-22 14:38 ` Daniel Vetter
2016-06-23 8:21 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2016-06-22 14:38 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomeu Vizoso, Daniel Vetter, linux-kernel, dri-devel, Emil Velikov
On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote:
>> On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> >> >> + * wsp: (#0x20 | #0x9 | #0xA)+
>> >> >> + *
>> >> >> + * eg.:
>> >> >> + * "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
>> >> >> + * "crtc 0 none" -> Stop CRC
>> >> >
>> >> > I've said this above, but again, it seems odd to me that you'd have to
>> >> > configure the CRC per-CRTC in one per-device file and read out the CRC
>> >> > from per-CRTC files.
>> >>
>> >> Not sure, I like that the per-crtc files just provide CRC data, and
>> >> that there's a separate control file that can be queried for the
>> >> current state.
>> >
>> > In my opinion that makes things needlessly complicated for userspace. If
>> > you want to query the state of a specific CRTC, you have to read out the
>> > entire file and parse each line to find the correct CRTC. On the other
>> > hand, chances are that you already need to know the path to the CRTC
>> > because you want to read the CRC out of the per-CRTC CRC file. In that
>> > case it would be much easier to simply concatenate the CRTC path and the
>> > CRC (or control) filename and read a single line (actually a single
>> > word) out of it to get at the same information.
>> >
>> > Furthermore if you have everything per-CRTC you no longer have to worry
>> > about pipe vs. index (that's always confusing because in the DRM core
>> > they're actually synonymous) because the CRTC path is canonical and will
>> > have the correct context.
>> >
>> > Per-CRTC directory with a single duplex file, or separate control and
>> > CRC files, is much simpler than the mix proposed here. No tokenization
>> > required when parsing in userspace, and no tokenization required to
>> > parse in the kernel either.
>>
>> Just jumping on this one here. I agree that if we remodel the
>> interface making the control file per-crtc would make sense. I think
>> separate control and read files makes sense, that's much less magic.
>
> Agreed, separate files would be a little simpler. I must admit that my
> proposal is partially motivated by a desire to avoid cumbersome naming
> of files. If we have separate files, what do you name them? crc for
> reading, crc_control for writing? crc_values for reading and crc for
> writing?
>
> Perhaps another way to avoid that would be to put the two files into a
> separate directory, as in:
>
> /sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
> +-- control
> +-- data
>
> That's slightly on the deeply nested side, but on the other hand it
> nicely uses the filesystem for namespacing, which is what filesystems
> are really good at.
crtc-<index>/crc/(control|data) sounds great.
>> And by reading the control file you can check what's the currently
>> selected source easily.
>
> Is that really a useful feature? If you're going to capture CRCs, you
> likely just want to set whatever you expect to receive irrespective of
> the current setting.
I think it's useful for hacking together your driver support, going
through tests it one more indirection. And I have a really bad
attention span ;-)
>> I'm not sure on the canonical CRTC path - right now we don't have that
>> in debugfs. I think just using index numbers is ok, we use those all
>> over the place already. Or maybe we could indeed add a new per-crtc
>> subdir in debugfs for this. Either way is fine with me.
>
> I can imagine that we'd like to expose a number of other per-CRTC
> properties (name, parts of the state, object ID, one day perhaps VBLANK
> counts, ...) this way, so a per-CRTC directory makes a lot of sense in
> my opinion.
Yeah, we might have room for more ... otoh for read-only debug
information I prefer big files that dump everything. Easier to extend.
But for tests/automation the one-value-per-file idea from sysfs, or at
least going much closer to that idea is a good idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-22 14:38 ` Daniel Vetter
@ 2016-06-23 8:21 ` Jani Nikula
2016-06-23 8:24 ` Tomeu Vizoso
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2016-06-23 8:21 UTC (permalink / raw)
To: Daniel Vetter, Thierry Reding
Cc: Daniel Vetter, dri-devel, linux-kernel, Tomeu Vizoso, Emil Velikov
On Wed, 22 Jun 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> Perhaps another way to avoid that would be to put the two files into a
>> separate directory, as in:
>>
>> /sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
>> +-- control
>> +-- data
>>
>> That's slightly on the deeply nested side, but on the other hand it
>> nicely uses the filesystem for namespacing, which is what filesystems
>> are really good at.
>
> crtc-<index>/crc/(control|data) sounds great.
Side note, we should eventually do the same for sink CRCs, but I guess
under the connectors. i915 currently has a special cased version for eDP
(named "i915_sink_crc_eDP1"...), reading the data from DPCD.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-23 8:21 ` Jani Nikula
@ 2016-06-23 8:24 ` Tomeu Vizoso
2016-06-23 8:43 ` Thierry Reding
0 siblings, 1 reply; 16+ messages in thread
From: Tomeu Vizoso @ 2016-06-23 8:24 UTC (permalink / raw)
To: Jani Nikula
Cc: Daniel Vetter, Thierry Reding, Daniel Vetter, Emil Velikov,
linux-kernel, dri-devel
On 23 June 2016 at 10:21, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 22 Jun 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>> Perhaps another way to avoid that would be to put the two files into a
>>> separate directory, as in:
>>>
>>> /sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
>>> +-- control
>>> +-- data
>>>
>>> That's slightly on the deeply nested side, but on the other hand it
>>> nicely uses the filesystem for namespacing, which is what filesystems
>>> are really good at.
>>
>> crtc-<index>/crc/(control|data) sounds great.
>
> Side note, we should eventually do the same for sink CRCs, but I guess
> under the connectors. i915 currently has a special cased version for eDP
> (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
Was hoping we could just add one more source to this interface to expose those.
Regards,
Tomeu
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-23 8:24 ` Tomeu Vizoso
@ 2016-06-23 8:43 ` Thierry Reding
2016-06-23 10:07 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Thierry Reding @ 2016-06-23 8:43 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Jani Nikula, Daniel Vetter, Daniel Vetter, Emil Velikov,
linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]
On Thu, Jun 23, 2016 at 10:24:46AM +0200, Tomeu Vizoso wrote:
> On 23 June 2016 at 10:21, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Wed, 22 Jun 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >>> Perhaps another way to avoid that would be to put the two files into a
> >>> separate directory, as in:
> >>>
> >>> /sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
> >>> +-- control
> >>> +-- data
> >>>
> >>> That's slightly on the deeply nested side, but on the other hand it
> >>> nicely uses the filesystem for namespacing, which is what filesystems
> >>> are really good at.
> >>
> >> crtc-<index>/crc/(control|data) sounds great.
> >
> > Side note, we should eventually do the same for sink CRCs, but I guess
> > under the connectors. i915 currently has a special cased version for eDP
> > (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
>
> Was hoping we could just add one more source to this interface to expose those.
I don't think that would be easy to achieve. You'd have to determine
that a sink source is connected to the CRTC and then ajdust the list
of possible sources dynamically.
For connectors we already have separate directories in debugfs and a
sink can easily be found from the connector it is attached to.
It's also possible, though I don't know of any that do so currently,
that eventually sinks will support several types (i.e. "sources") of
CRC, which will further complicate to represent this in the CRTC's
list of CRC sources.
And it may also be useful to have both CRCs at the same time. The eDP
specification for example defines a very specific polynomial that is
used to compute the CRC, whereas not all display hardware uses a CRC
algorithm that's documented.
Finally, the data that will be checksummed by the CRTC isn't necessarily
the same as that arriving at the sink.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
2016-06-23 8:43 ` Thierry Reding
@ 2016-06-23 10:07 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2016-06-23 10:07 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomeu Vizoso, Jani Nikula, Daniel Vetter, Emil Velikov,
linux-kernel, dri-devel
On Thu, Jun 23, 2016 at 10:43 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, Jun 23, 2016 at 10:24:46AM +0200, Tomeu Vizoso wrote:
>> On 23 June 2016 at 10:21, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > On Wed, 22 Jun 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>> >> <thierry.reding@gmail.com> wrote:
>> >>> Perhaps another way to avoid that would be to put the two files into a
>> >>> separate directory, as in:
>> >>>
>> >>> /sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/
>> >>> +-- control
>> >>> +-- data
>> >>>
>> >>> That's slightly on the deeply nested side, but on the other hand it
>> >>> nicely uses the filesystem for namespacing, which is what filesystems
>> >>> are really good at.
>> >>
>> >> crtc-<index>/crc/(control|data) sounds great.
>> >
>> > Side note, we should eventually do the same for sink CRCs, but I guess
>> > under the connectors. i915 currently has a special cased version for eDP
>> > (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
>>
>> Was hoping we could just add one more source to this interface to expose those.
>
> I don't think that would be easy to achieve. You'd have to determine
> that a sink source is connected to the CRTC and then ajdust the list
> of possible sources dynamically.
>
> For connectors we already have separate directories in debugfs and a
> sink can easily be found from the connector it is attached to.
>
> It's also possible, though I don't know of any that do so currently,
> that eventually sinks will support several types (i.e. "sources") of
> CRC, which will further complicate to represent this in the CRTC's
> list of CRC sources.
>
> And it may also be useful to have both CRCs at the same time. The eDP
> specification for example defines a very specific polynomial that is
> used to compute the CRC, whereas not all display hardware uses a CRC
> algorithm that's documented.
>
> Finally, the data that will be checksummed by the CRTC isn't necessarily
> the same as that arriving at the sink.
We already do all that for i915. On some platforms the normal
end-of-pipe CRC doesn't work together with DP, instead you need to use
a special port (= connector/encoder) based CRC. The "auto" one
automatically walks the modeset config on those platforms to figure
out which one to pick. It would be kinda hard for userspace to
automatically figure out when the auto CRC doesn't work on the CRTC
and it would instead need to use the connector one. I think it'd
better to hide that in the kernel driver, where it's known.
Wrt specific polynomials: We could just spec that the "eDP-sink"
source is the one with CRC computed per the eDP spec. Similar for
anything else standardize. Heck you could even do the same with vendor
specific CRCs on IP blocks by using special names.
Wrt CRC measuring different data: That's also already the case on
i915. We have CRC for pre-gamma, post-gamma, post panel-fitter, on the
port, with or without audio stream included, .... And this all even
varies between platforms. The only thing that's specifified is that
"auto" should be some CRC after all the blending/color-correction has
been applied, but before any port specific scrambling happens which
might make the CRC change with each frame. E.g. on platforms where
auto aliases the DP ports we need to set a special bit to reset the
scrambler state on each vblank to ensure stable CRCs.
Given all that I think putting sink crc capture into the same
framework makes sense.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-06-23 10:07 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-21 11:06 [PATCH v1 0/3] New debugfs API for capturing CRC of frames Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 1/3] drm/i915/debugfs: Move out pipe CRC code Tomeu Vizoso
2016-06-21 11:06 ` [PATCH v1 2/3] drm: Add API for capturing frame CRCs Tomeu Vizoso
2016-06-21 15:07 ` Thierry Reding
2016-06-22 8:26 ` Tomeu Vizoso
2016-06-22 13:32 ` Thierry Reding
2016-06-22 14:08 ` Daniel Vetter
2016-06-22 14:31 ` Thierry Reding
2016-06-22 14:38 ` Daniel Vetter
2016-06-23 8:21 ` Jani Nikula
2016-06-23 8:24 ` Tomeu Vizoso
2016-06-23 8:43 ` Thierry Reding
2016-06-23 10:07 ` Daniel Vetter
2016-06-22 14:12 ` Daniel Vetter
2016-06-22 14:20 ` Daniel Vetter
2016-06-21 11:06 ` [PATCH v1 3/3] drm/i915: Use new CRC debugfs API 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).