Hi,
Thanks for the review.
On 7/10/2018 5:19 PM, Laurent Pinchart wrote:
Hi Mahesh,
Thank you for the patch.
On Monday, 2 July 2018 14:07:27 EEST Mahesh Kumar wrote:
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.
Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
---
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 | 52 ++++++++-----------
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 | 5 +--
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +--
include/drm/drm_crtc.h | 3 +-
8 files changed, 30 insertions(+), 50 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
b/drivers/gpu/drm/drm_debugfs_crc.c index f4d76528d24c..739a813b4b09 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -124,11 +124,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);
@@ -193,12 +191,15 @@ static int crtc_crc_open(struct inode *inode, struct
file *filep) return ret;
}
- if (crtc->funcs->verify_crc_source) {
- ret = crtc->funcs->verify_crc_source(crtc, crc->source,
- &values_cnt);
- if (ret)
- 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;
spin_lock_irq(&crc->lock);
if (!crc->opened)
@@ -210,30 +211,22 @@ static int crtc_crc_open(struct inode *inode, struct
file *filep) if (ret)
return ret;
- ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
- 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;
+ goto err;
}
If you moved allocation before the !crc->opened check, you could group the two
code blocks protected by the crc->lock.
agree, will update in next version.
-Mahesh
spin_lock_irq(&crc->lock);
crc->entries = entries;
crc->values_cnt = values_cnt;
+ spin_unlock_irq(&crc->lock);
+ ret = crtc->funcs->set_crc_source(crtc, crc->source);
+ 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
@@ -250,7 +243,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);
@@ -262,9 +255,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);
@@ -370,7 +362,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);
[snip]
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
