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

Laurent Pinchart laurent.pinchart at ideasonboard.com
Thu Jul 12 11:39:55 UTC 2018


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 at intel.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Acked-by: Leo Li <sunpeng.li at amd.com>

For core code and the R-Car DU driver,

Reviewed-by: Laurent Pinchart <laurent.pinchart at 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:
>  	 *


-- 
Regards,

Laurent Pinchart





More information about the dri-devel mailing list