[igt-dev] [PATCH i-g-t] lib/igt_debugfs: Unify crc collection

[Kahola, Mika]

On Wed, 2019-01-16 at 17:43 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2019-01-14 at 14:14 +0200, Mika Kahola wrote:
> > For one shot crc collection, let's use the same helper function as
> > we
> > use for continuous crc collection. With this patch, we first drain
> > the
> > pipe from queued crc values and read the fresh crc.
> > 
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> >  lib/igt_debugfs.c | 59 +++++++++++++++++++++++--------------------
> > ----
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index a3aca846..12e8ad78 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -901,36 +901,6 @@ static void crc_sanity_checks(igt_crc_t *crc)
> >  	igt_warn_on_f(all_zero, "Suspicious CRC: All values are 0.\n");
> >  }
> >  
> > -/**
> > - * igt_pipe_crc_collect_crc:
> > - * @pipe_crc: pipe CRC object
> > - * @out_crc: buffer for the captured CRC values
> > - *
> > - * Read a single CRC from @pipe_crc. This function blocks until
> > the
> > CRC is
> > - * retrieved, irrespective of whether @pipe_crc has been opened
> > with
> > - * igt_pipe_crc_new() or igt_pipe_crc_new_nonblock().  @out_crc
> > must
> > be
> > - * allocated by the caller.
> > - *
> > - * This function takes care of the pipe_crc book-keeping, it will
> > start/stop
> > - * the collection of the CRC.
> > - *
> > - * This function also calls the interactive debug with the "crc"
> > domain, so you
> > - * can make use of this feature to actually see the screen that is
> > being CRC'd.
> > - *
> > - * For continuous CRC collection look at igt_pipe_crc_start(),
> > - * igt_pipe_crc_get_crcs() and igt_pipe_crc_stop().
> > - */
> > -void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t
> > *out_crc)
> > -{
> > -	igt_debug_wait_for_keypress("crc");
> > -
> > -	igt_pipe_crc_start(pipe_crc);
> > -	read_one_crc(pipe_crc, out_crc);
> > -	igt_pipe_crc_stop(pipe_crc);
> > -
> > -	crc_sanity_checks(out_crc);
> > -}
> > -
> >  /**
> >   * igt_pipe_crc_drain:
> >   * @pipe_crc: pipe CRC object
> > @@ -1004,6 +974,35 @@ igt_pipe_crc_get_current(int drm_fd,
> > igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >  	crc_sanity_checks(crc);
> >  }
> >  
> > +/**
> > + * igt_pipe_crc_collect_crc:
> > + * @pipe_crc: pipe CRC object
> > + * @out_crc: buffer for the captured CRC values
> > + *
> > + * Read a single CRC from @pipe_crc. This function blocks until
> > the
> > CRC is
> > + * retrieved, irrespective of whether @pipe_crc has been opened
> > with
> > + * igt_pipe_crc_new() or igt_pipe_crc_new_nonblock().  @out_crc
> > must
> > be
> > + * allocated by the caller.
> > + *
> > + * This function takes care of the pipe_crc book-keeping, it will
> > start/stop
> > + * the collection of the CRC.
> > + *
> > + * This function also calls the interactive debug with the "crc"
> > domain, so you
> > + * can make use of this feature to actually see the screen that is
> > being CRC'd.
> > + *
> > + * For continuous CRC collection look at igt_pipe_crc_start(),
> > + * igt_pipe_crc_get_crcs() and igt_pipe_crc_stop().
> > + */
> > +void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t
> > *out_crc)
> > +{
> > +	igt_debug_wait_for_keypress("crc");
> > +
> > +	igt_pipe_crc_start(pipe_crc);
> > +	igt_pipe_crc_drain(pipe_crc);
> 
> Since CRC collection just started in the previous line, won't this
> dro
> the first CRC?
You're right. Since we start collecting crc's, we don't need to drain
the pipe (There shouldn't be any crc's left).

I'll drop this in the next version.

Thanks for the review! 
> 
> > +	igt_pipe_crc_get_single(pipe_crc, out_crc);
> > +	igt_pipe_crc_stop(pipe_crc);
> > +}
> > +
> >  /*
> >   * Drop caches
> >   */
> 
>