This series improves crc-core <-> driver interface. This series adds following functionality in the crc-core - Now control node will print all the available sources if implemented by driver along with current source. - Setting of sorce will fail if provided source is not supported - cleanup of crtc_crc_open function first allocate memory before enabling CRC generation - Don't block open() call instead wait in crc read call.
Following IGT will fail due to crc-core <-> driver interface change igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail> ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence In nonblocking crc tests we'll get lesser crc's due to skipping crc
AMD/Rockchip/rcar code path is not validated and may need inputs
Changes: - Rebase on top of VKMS driver
Cc: dri-devel@lists.freedesktop.org
Mahesh Kumar (3): drm/vkms/crc: Implement verify_crc_source callback drm/crc: Cleanup crtc_crc_open function Revert "drm: crc: Wait for a frame before returning from open()"
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +- drivers/gpu/drm/drm_debugfs_crc.c | 71 ++++++++-------------- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 4 +- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 +- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +- drivers/gpu/drm/vkms/vkms_crc.c | 41 ++++++++++--- drivers/gpu/drm/vkms/vkms_crtc.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 5 +- include/drm/drm_crtc.h | 3 +- 11 files changed, 71 insertions(+), 74 deletions(-)
This patch implements "verify_crc_source" callback function for Virtual KMS drm driver.
Cc: dri-devel@lists.freedesktop.org Cc: Haneen Mohammed hamohammed.sa@gmail.com Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com --- drivers/gpu/drm/vkms/vkms_crc.c | 36 ++++++++++++++++++++++++++++++++---- drivers/gpu/drm/vkms/vkms_crtc.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 2 ++ 3 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index 37d717f38e3c..ce5ee0461f80 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -70,6 +70,35 @@ void vkms_crc_work_handle(struct work_struct *work) drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32); }
+static int vkms_crc_parse_source(const char *src_name, bool *enabled) +{ + int ret = 0; + + if (!src_name) { + *enabled = false; + } else if (strcmp(src_name, "auto") == 0) { + *enabled = true; + } else { + *enabled = false; + ret = -EINVAL; + } + + return ret; +} + +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name, + size_t *values_cnt) +{ + bool enabled; + + if (vkms_crc_parse_source(src_name, &enabled) < 0) { + DRM_DEBUG_DRIVER("unknown source %s\n", src_name); + return -EINVAL; + } + + return 0; +} + int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, size_t *values_cnt) { @@ -78,10 +107,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, unsigned long flags; int ret = 0;
- if (src_name && strcmp(src_name, "auto") == 0) - enabled = true; - else if (src_name) - ret = -EINVAL; + ret = vkms_crc_parse_source(src_name, &enabled); + if (ret) + return ret;
*values_cnt = 1;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index bfe6e0312cc4..9d0b1a325a78 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -140,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { .enable_vblank = vkms_enable_vblank, .disable_vblank = vkms_disable_vblank, .set_crc_source = vkms_set_crc_source, + .verify_crc_source = vkms_verify_crc_source, };
static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index f156c930366a..090c5e4f5544 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -125,6 +125,8 @@ void vkms_gem_vunmap(struct drm_gem_object *obj); /* CRC Support */ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, size_t *values_cnt); +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name, + size_t *values_cnt); void vkms_crc_work_handle(struct work_struct *work);
#endif /* _VKMS_DRV_H_ */
This patch make changes to allocate crc-entries buffer before enabling CRC generation. It moves all the failure check early in the function before setting the source or memory allocation. Now set_crc_source takes only two variable inputs, values_cnt we already gets as part of verify_crc_source.
Changes since V1: - refactor code to use single spin lock Changes since V2: - rebase Changes since V3: - rebase on top of VKMS driver
Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Haneen Mohammed hamohammed.sa@gmail.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com (V2) Acked-by: Leo Li sunpeng.li@amd.com (V2) Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com (V3) --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +- drivers/gpu/drm/drm_debugfs_crc.c | 61 ++++++++++------------ drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 4 +- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 +- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +-- drivers/gpu/drm/vkms/vkms_crc.c | 5 +- drivers/gpu/drm/vkms/vkms_drv.h | 3 +- include/drm/drm_crtc.h | 3 +- 10 files changed, 39 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index e43ed064dc46..54056d180003 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
/* amdgpu_dm_crc.c */ #ifdef CONFIG_DEBUG_FS -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name, - size_t *values_cnt); +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name); int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name, size_t *values_cnt); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c index dfcca594d52a..e7ad528f5853 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c @@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name, return 0; }
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name, - size_t *values_cnt) +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) { struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state); struct dc_stream_state *stream_state = crtc_state->stream; @@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name, return -EINVAL; }
- *values_cnt = 3; /* Reset crc_skipped on dm state */ crtc_state->crc_skip_count = 0; return 0; diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index d7e626331eca..3e0a2cfaa35c 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -127,11 +127,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf, if (source[len] == '\n') source[len] = '\0';
- if (crtc->funcs->verify_crc_source) { - ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); - if (ret) - return ret; - } + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); + if (ret) + return ret;
spin_lock_irq(&crc->lock);
@@ -197,40 +195,40 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) return ret; }
+ ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt); + if (ret) + return ret; + + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) + return -EINVAL; + + if (WARN_ON(values_cnt == 0)) + return -EINVAL; + + entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL); + if (!entries) + return -ENOMEM; + spin_lock_irq(&crc->lock); - if (!crc->opened) + if (!crc->opened) { crc->opened = true; - else + crc->entries = entries; + crc->values_cnt = values_cnt; + } else { ret = -EBUSY; + } spin_unlock_irq(&crc->lock);
- if (ret) + if (ret) { + kfree(entries); return ret; + }
- ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt); + ret = crtc->funcs->set_crc_source(crtc, crc->source); if (ret) goto err;
- if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) { - ret = -EINVAL; - goto err_disable; - } - - if (WARN_ON(values_cnt == 0)) { - ret = -EINVAL; - goto err_disable; - } - - entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL); - if (!entries) { - ret = -ENOMEM; - goto err_disable; - } - spin_lock_irq(&crc->lock); - crc->entries = entries; - crc->values_cnt = values_cnt; - /* * Only return once we got a first frame, so userspace doesn't have to * guess when this particular piece of HW will be ready to start @@ -247,7 +245,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) return 0;
err_disable: - crtc->funcs->set_crc_source(crtc, NULL, &values_cnt); + crtc->funcs->set_crc_source(crtc, NULL); err: spin_lock_irq(&crc->lock); crtc_crc_cleanup(crc); @@ -259,9 +257,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) { struct drm_crtc *crtc = filep->f_inode->i_private; struct drm_crtc_crc *crc = &crtc->crc; - size_t values_cnt;
- crtc->funcs->set_crc_source(crtc, NULL, &values_cnt); + crtc->funcs->set_crc_source(crtc, NULL);
spin_lock_irq(&crc->lock); crtc_crc_cleanup(crc); @@ -367,7 +364,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc) { struct dentry *crc_ent, *ent;
- if (!crtc->funcs->set_crc_source) + if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source) return 0;
crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7b984aefce98..2cb8d8007a50 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2178,8 +2178,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
/* intel_pipe_crc.c */ #ifdef CONFIG_DEBUG_FS -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name, - size_t *values_cnt); +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name); int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, size_t *values_cnt); const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c index 83f9ade0cd81..f3c9010e332a 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -583,8 +583,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, return -EINVAL; }
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name, - size_t *values_cnt) +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index]; @@ -623,7 +622,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name, }
pipe_crc->skipped = 0; - *values_cnt = 5;
out: intel_display_power_put(dev_priv, power_domain); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 57db868da4fe..8a9e5e6f16b4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -887,8 +887,7 @@ const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc, }
static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc, - const char *source_name, - size_t *values_cnt) + const char *source_name) { struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct drm_modeset_acquire_ctx ctx; @@ -903,7 +902,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc, return ret;
index = ret; - *values_cnt = 1;
/* Perform an atomic commit to set the CRC source. */ drm_modeset_acquire_init(&ctx, 0); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c9a5ea38e86b..38f8cae7ef51 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1111,7 +1111,7 @@ static struct drm_connector *vop_get_edp_connector(struct vop *vop) }
static int vop_crtc_set_crc_source(struct drm_crtc *crtc, - const char *source_name, size_t *values_cnt) + const char *source_name) { struct vop *vop = to_vop(crtc); struct drm_connector *connector; @@ -1121,8 +1121,6 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc, if (!connector) return -EINVAL;
- *values_cnt = 3; - if (source_name && strcmp(source_name, "auto") == 0) ret = analogix_dp_start_crc(connector); else if (!source_name) @@ -1146,7 +1144,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
#else static int vop_crtc_set_crc_source(struct drm_crtc *crtc, - const char *source_name, size_t *values_cnt) + const char *source_name) { return -ENODEV; } diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index ce5ee0461f80..44775434945f 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -99,8 +99,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name, return 0; }
-int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, - size_t *values_cnt) +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) { struct vkms_output *out = drm_crtc_to_vkms_output(crtc); bool enabled = false; @@ -111,8 +110,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, if (ret) return ret;
- *values_cnt = 1; - /* make sure nothing is scheduled on crtc workq */ flush_workqueue(out->crc_workq);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 090c5e4f5544..2017a2ccc43d 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -123,8 +123,7 @@ int vkms_gem_vmap(struct drm_gem_object *obj); void vkms_gem_vunmap(struct drm_gem_object *obj);
/* CRC Support */ -int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name, - size_t *values_cnt); +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name); int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name, size_t *values_cnt); void vkms_crc_work_handle(struct work_struct *work); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f2dd180a867a..b21437bc95bf 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -744,8 +744,7 @@ struct drm_crtc_funcs { * * 0 on success or a negative error code on failure. */ - int (*set_crc_source)(struct drm_crtc *crtc, const char *source, - size_t *values_cnt); + int (*set_crc_source)(struct drm_crtc *crtc, const char *source); /** * @verify_crc_source: *
This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.
Don't wait for first CRC during crtc_crc_open. It avoids one frame wait during open. If application want to wait after read call, it can use poll/read blocking read() call.
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_debugfs_crc.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 3e0a2cfaa35c..00e743153e94 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -228,24 +228,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) if (ret) goto err;
- spin_lock_irq(&crc->lock); - /* - * Only return once we got a first frame, so userspace doesn't have to - * guess when this particular piece of HW will be ready to start - * generating CRCs. - */ - ret = wait_event_interruptible_lock_irq(crc->wq, - crtc_crc_data_count(crc), - crc->lock); - spin_unlock_irq(&crc->lock); - - if (ret) - goto err_disable; - return 0;
-err_disable: - crtc->funcs->set_crc_source(crtc, NULL); err: spin_lock_irq(&crc->lock); crtc_crc_cleanup(crc);
dri-devel@lists.freedesktop.org
-
Mahesh Kumar