[Intel-gfx] [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Kumar, Mahesh]

Hi,

Thanks for the review,
On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
>> This patch adds a new callback function "verify_crc_source" which will
>> be used during setting the crc source in control node and while opening
>> data node for crc reading. This will help in avoiding setting of wrong
>> string for source.
> Why do you need to verify the source in the open() operation ? Isn't it enough
> to verify it in the write() handler ? The answer to this question might lie in
> patch 08/10, in which case I'd add the .verify_crc_source() call in open() in
> that patch, not here.
Yes, final goal is achieved by patch 08/10. But if crc_read is called 
before setting the source, it may have wrong source set in that case I 
wanted to return early at least for the drivers implemented 
verify_crc_source. If you still think otherwise I will modify 
accordingly in next series.

-Mahesh

>
>> 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/drm_debugfs_crc.c | 15 +++++++++++++++
>>   include/drm/drm_crtc.h            | 15 +++++++++++++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const
>> char __user *ubuf, struct drm_crtc *crtc = m->private;
>>   	struct drm_crtc_crc *crc = &crtc->crc;
>>   	char *source;
>> +	size_t values_cnt;
>> +	int ret = 0;
>>
>>   	if (len == 0)
>>   		return 0;
>> @@ -104,6 +106,12 @@ 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;
>> +	}
>> +
>>   	spin_lock_irq(&crc->lock);
>>
>>   	if (crc->opened) {
>> @@ -167,6 +175,13 @@ 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;
>> +	}
>> +
>>   	spin_lock_irq(&crc->lock);
>>   	if (!crc->opened)
>>   		crc->opened = true;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 23eddbccab10..1a6dcbf91744 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>>   	 */
>>   	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>>   			      size_t *values_cnt);
>> +	/**
>> +	 * @verify_crc_source:
>> +	 *
>> +	 * verifies the source of CRC checksums of frames before setting the
>> +	 * source for CRC and during crc open.
>> +	 *
>> +	 * This callback is optional if the driver does not support any CRC
>> +	 * generation functionality.
>> +	 *
>> +	 * RETURNS:
>> +	 *
>> +	 * 0 on success or a negative error code on failure.
>> +	 */
>> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>> +				 size_t *values_cnt);
>>
>>   	/**
>>   	 * @atomic_print_state:
>