Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:26 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. This will help
> in avoiding setting of wrong string for source.
>
> Changes since V1:
> - do not yet verify_crc_source during open.
>
> Signed-off-by: Mahesh Kumar <mahesh1.ku...@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> ---
> drivers/gpu/drm/drm_debugfs_crc.c | 8 ++++++++
> include/drm/drm_crtc.h | 15 +++++++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 99961192bf03..1956acb61cc8 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;
No need to initialize ret to 0 here.
>
> 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) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 17f4f93340b8..3ce5d2fe133d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -684,6 +684,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.
Please also document whether the source parameter can be NULL.
It would also be nice to document values_cnt, but as that's not documented for
the .set_crc_source() operation at the moment, I won't block the patch for
this.
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> + *
> + * 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
