Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

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@intel.com Cc: dri-devel@lists.freedesktop.org Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

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.

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]