Date:   Tue, 6 Aug 2019 11:12:33 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Brian Starkey <Brian.Starkey@....com>
Cc:     Daniel Vetter <daniel@...ll.ch>, Liviu Dudau <Liviu.Dudau@....com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@....com>,
        "Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
        nd <nd@....com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit
 interactions

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.starkey@....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.vetter@...ll.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

Powered by blists - more mailing lists