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

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:33 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.

Changes since V1:

• refactor code to use single spin lock

Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Leo Li sunpeng.li@amd.com

For core code and the R-Car DU driver,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.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 | 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 | 5 +- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +-- include/drm/drm_crtc.h | 3 +- 8 files changed, 37 insertions(+), 52 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 940b76a1ab71..7386391636e3 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 29b054de9e13..814f6f3e1595 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2155,8 +2155,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 7124918372f8..72db1834dd50 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -808,8 +808,7 @@ static int rcar_du_crtc_verify_crc_source(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 drm_modeset_acquire_ctx ctx; struct drm_crtc_state *crtc_state; @@ -837,8 +836,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc, return -EINVAL; }

• *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 77e91b15ddb4..657372306623 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1117,7 +1117,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; @@ -1127,8 +1127,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)

@@ -1152,7 +1150,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/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 69886025e628..1013d6596408 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -682,8 +682,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: