On Mon, Aug 05, 2019 at 04:54:15PM +0000, Brian Starkey wrote:
> On Mon, Aug 05, 2019 at 06:24:17PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote:
> > > CRC generation can be impacted by commits coming from userspace, and
> > > enabling CRC generation may itself trigger a commit. Add notes about
> > > this to the kerneldoc.
> > >
> > > Signed-off-by: Brian Starkey <brian.star...@arm.com>
> > > ---
> > >
> > > I might have got the wrong end of the stick, but this is what I
> > > understood from what you said.
> > >
> > > Cheers,
> > > -Brian
> > >
> > > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++----
> > > include/drm/drm_crtc.h | 3 +++
> > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> > > b/drivers/gpu/drm/drm_debugfs_crc.c
> > > index 7ca486d750e9..1dff956bcc74 100644
> > > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > > @@ -65,10 +65,17 @@
> > > * it submits. In this general case, the maximum userspace can do is to
> > > compare
> > > * the reported CRCs of frames that should have the same contents.
> > > *
> > > - * On the driver side the implementation effort is minimal, drivers only
> > > need to
> > > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> > > automatically
> > > - * set up if that vfunc is set. CRC samples need to be captured in the
> > > driver by
> > > - * calling drm_crtc_add_crc_entry().
> > > + * On the driver side the implementation effort is minimal, drivers only
> > > need
> > > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> > > + * automatically set up if that vfunc is set. CRC samples need to be
> > > captured
> > > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the
> > > driver
> > > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a
> > > commit
> > > + * (even a full modeset).
> > > + *
> > > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC
> > > or the
> > > + * legacy paths may interfere with CRC generation. So, in the general
> > > case,
> > > + * userspace can't rely on the values in crtc-N/crc/data being valid
> > > + * across a commit.
> >
> > It's not just the values, but the generation itself might get disabled
> > (e.g. on i915 if you select "auto" on some chips you get the DP port
> > sampling point, but for HDMI mode you get a different sampling ploint, and
> > if you get the wrong one there won't be any crc for you). Also it's not
> > just any atomic commit, only the ones with ALLOW_MODESET.
>
> Is the meaning of ALLOW_MODESET actually defined somewhere then? I
> thought it was broadly speaking "anything that would cause a flicker
> on the output" - but that needn't be the same set of things that break
> CRC generation.
It's the inverse, I think we should require that crc keeps working for
non-ALLOW_MODESET. Otherwise crc are essentially useless for validating
stuff. And yeah allow_modeset is "could flicker and/or take enormous
amounts of time".
-Daniel
> > Maybe something like the below text:
> >
> > "Please note that an atomic modeset commit with the
> > DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl
> > (which amounts to the same), can destry the CRC setup due to hardware
> > requirements. This results in inconsistent CRC values or not even any CRC
> > values generated. Generic userspace therefore needs to re-setup the CRC
> > after each such modeset."
> >
> > >
> > > static int crc_control_show(struct seq_file *m, void *data)
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 128d8b210621..0f7ea094a900 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -756,6 +756,8 @@ struct drm_crtc_funcs {
> > > * provided from the configured source. Drivers must accept an "auto"
> > > * source name that will select a default source for this CRTC.
> > > *
> > > + * This may trigger a commit if necessary, to enable CRC generation.
> >
> > I'd clarify this as "atomic modeset commit" just to be sure.
>
> Ack.
>
> Thanks,
> -Brian
>
> >
> > With these two comments addressed somehow:
> >
> > Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >
> >
> > > + *
> > > * Note that "auto" can depend upon the current modeset configuration,
> > > * e.g. it could pick an encoder or output specific CRC sampling point.
> > > *
> > > @@ -767,6 +769,7 @@ struct drm_crtc_funcs {
> > > * 0 on success or a negative error code on failure.
> > > */
> > > int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> > > +
> > > /**
> > > * @verify_crc_source:
> > > *
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
