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

[Leo Li <sunpeng.li@xxxxxxx>]

On 2018-07-02 07:07 AM, 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>

Acked-by: Leo Li <sunpeng.li@xxxxxxx>

> ---
>   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(-)
>
> 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 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;
>   	}
>   
>   	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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 99e3b6477d39..7fe0341bcc27 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2154,8 +2154,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
>   /* intel_pipe_crc.c */
>   int intel_pipe_crc_create(struct drm_minor *minor);
>   #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 1dffc346f1bc..27d560f7a817 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -1028,8 +1028,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];
> @@ -1068,7 +1067,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 24eeaa7e14d7..829c644addba 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -796,8 +796,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 rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>   	struct drm_modeset_acquire_ctx ctx;
> @@ -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 ffaec138aeee..81780325f36f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -659,8 +659,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:
>   	 *
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx