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@xxxxxxxxx>
> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
