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

[Laurent Pinchart]

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.

> >> 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:


-- 
Regards,

Laurent Pinchart