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

[Kumar, Mahesh]

Hi,


On 7/10/2018 5:40 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote:
>> On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
>>> 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.
> I don't disagree with you, but I don't think this issue is new. As I got a bit
> confused as to why the call is needed in open() (there's no explanation in
> this patch), I think fixing the problem in 08/10 would be better.
ok sure :)
-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:
>