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

[Kumar, Mahesh]

Hi,

Thanks for the review.

On 7/10/2018 5:19 PM, Laurent Pinchart wrote:
> 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 at intel.com>
>> Cc: dri-devel at lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at 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.
agree, will update in next version.
-Mahesh
>>   	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]
>