[Intel-gfx] [PATCH V2 1/3] drm/vkms/crc: Implement verify_crc_source callback

[Kumar, Mahesh]

Hi Haneen,


On 8/21/2018 4:09 AM, Haneen Mohammed wrote:
> On Tue, Aug 14, 2018 at 08:31:03AM +0530, Mahesh Kumar wrote:
>> This patch implements "verify_crc_source" callback function for
>> Virtual KMS drm driver.
>>
>> Changes Since V1:
>> - update values_cnt in verify_crc_source
>>
>> Cc: Haneen Mohammed <hamohammed.sa at gmail.com>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/vkms/vkms_crc.c  | 38 ++++++++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/vkms/vkms_crtc.c |  1 +
>>   drivers/gpu/drm/vkms/vkms_drv.h  |  2 ++
>>   3 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
>> index 37d717f38e3c..b2a484b4e2ad 100644
>> --- a/drivers/gpu/drm/vkms/vkms_crc.c
>> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
>> @@ -70,6 +70,37 @@ void vkms_crc_work_handle(struct work_struct *work)
>>   	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
>>   }
>>   
>> +static int vkms_crc_parse_source(const char *src_name, bool *enabled)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!src_name) {
>> +		*enabled = false;
>> +	} else if (strcmp(src_name, "auto") == 0) {
>> +		*enabled = true;
>> +	} else {
>> +		*enabled = false;
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
>> +			   size_t *values_cnt)
>> +{
>> +	bool enabled;
>> +
>> +	if (vkms_crc_parse_source(src_name, &enabled) < 0) {
>> +		DRM_DEBUG_DRIVER("unknown source %s\n", src_name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	*values_cnt = 1;
>> +
>> +	return 0;
>> +}
>> +
>>   int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>>   			size_t *values_cnt)
>>   {
>> @@ -78,10 +109,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>>   	unsigned long flags;
>>   	int ret = 0;
>>   
>> -	if (src_name && strcmp(src_name, "auto") == 0)
>> -		enabled = true;
>> -	else if (src_name)
>> -		ret = -EINVAL;
>> +	ret = vkms_crc_parse_source(src_name, &enabled);
>> +	if (ret)
>> +		return ret;
> I think the return value after vkms_crc_parse_source should be called
> once instead at the end of vkms_set_crc_source otherwise crc_enabled
> won't be updated?
Here I'm assuming if src_name is wrong we don't need to proceed further, 
let the CRC generation to continue whatever it was doing (enabled or 
disabled).
But anyway that is changing the original design, will remove above 
return to keep the behavior same.

-Mahesh
>
> - Haneen
>
>>   
>>   	*values_cnt = 1;
>>   
>> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
>> index bfe6e0312cc4..9d0b1a325a78 100644
>> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
>> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>> @@ -140,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
>>   	.enable_vblank		= vkms_enable_vblank,
>>   	.disable_vblank		= vkms_disable_vblank,
>>   	.set_crc_source		= vkms_set_crc_source,
>> +	.verify_crc_source	= vkms_verify_crc_source,
>>   };
>>   
>>   static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> index f156c930366a..090c5e4f5544 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> @@ -125,6 +125,8 @@ void vkms_gem_vunmap(struct drm_gem_object *obj);
>>   /* CRC Support */
>>   int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>>   			size_t *values_cnt);
>> +int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>> +			   size_t *values_cnt);
>>   void vkms_crc_work_handle(struct work_struct *work);
>>   
>>   #endif /* _VKMS_DRV_H_ */
>> -- 
>> 2.16.2
>>