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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org, Mahesh Kumar <mahesh1.kumar@intel.com>
Subject: Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
Date: Tue, 10 Jul 2018 14:49:11 +0300	[thread overview]
Message-ID: <18451995.rnOpagHzdN@avalon> (raw)
In-Reply-To: <20180702110729.8171-9-mahesh1.kumar@intel.com>

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]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel