Re: [igt-dev] [PATCH i-g-t] lib/debugfs: Use a blocking read in igt_pipe_crc_get_single()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 26, 2018 at 09:56:47PM +0300, Juha-Pekka Heikkilä wrote:
> Looks good. I suspect this will reduce those random crc errors seen in CI.
> 
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> 
> Ville Syrjala kirjoitti 25.9.2018 klo 17.53:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > All users of igt_pipe_crc_get_single() want a blocking read even if
> > the fd was opened in nonblocking mode. Make it so.
> > 
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   lib/igt_debugfs.c           | 35 ++++++++++++-----------------------
> >   lib/igt_debugfs.h           |  2 +-
> >   tests/kms_chv_cursor_fail.c |  2 +-
> >   3 files changed, 14 insertions(+), 25 deletions(-)
> > 
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index baedc2255ac9..d151d9b23ed4 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -757,8 +757,13 @@ static void read_one_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out)
> >   {
> >   	int ret;
> >   
> > -	while ((ret = read_crc(pipe_crc, out)) <= 0 && ret != -EINVAL)
> > -		usleep(1000);
> > +	fcntl(pipe_crc->crc_fd, F_SETFL, pipe_crc->flags & ~O_NONBLOCK);
> > +
> > +	do {
> > +		ret = read_crc(pipe_crc, out);
> > +	} while (ret == -EINTR);
> > +
> > +	fcntl(pipe_crc->crc_fd, F_SETFL, pipe_crc->flags);
> >   }
> >   
> >   /**
> > @@ -926,12 +931,8 @@ void igt_pipe_crc_drain(igt_pipe_crc_t *pipe_crc)
> >    * @pipe_crc: pipe CRC object
> >    * @crc: buffer pointer for the captured CRC value
> >    *
> > - * Read a single @crc from @pipe_crc. This function does not block
> > - * when nonblocking CRC is requested, and will return false if no CRC
> > - * can be captured.
> > - *
> > - * If opened in blocking mode it will always block until a new CRC is read, like
> > - * igt_pipe_crc_collect_crc().
> > + * Read a single @crc from @pipe_crc. This function blocks even
> > + * when nonblocking CRC is requested.
> >    *
> >    * Callers must start and stop the capturing themselves by calling
> >    * igt_pipe_crc_start() and igt_pipe_crc_stop(). For one-shot CRC collecting
> > @@ -943,22 +944,11 @@ void igt_pipe_crc_drain(igt_pipe_crc_t *pipe_crc)
> >    * Returns:
> >    * Whether a crc is captured, only false in non-blocking mode.

I just noticed that I failed to remove this "Returns: ..." stuff here.
Fixed while pushing. Thanks for the review.

> >    */
> > -bool
> > -igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> > +void igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >   {
> > -	bool found = true;
> > +	read_one_crc(pipe_crc, crc);
> >   
> > -	if (pipe_crc->flags & O_NONBLOCK) {
> > -		int ret = read_crc(pipe_crc, crc);
> > -
> > -		found = ret > 0;
> > -	} else
> > -		read_one_crc(pipe_crc, crc);
> > -
> > -	if (found)
> > -		crc_sanity_checks(crc);
> > -
> > -	return found;
> > +	crc_sanity_checks(crc);
> >   }
> >   
> >   /**
> > @@ -976,7 +966,6 @@ igt_pipe_crc_get_current(int drm_fd, igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >   {
> >   	unsigned vblank = kmstest_get_vblank(drm_fd, pipe_crc->pipe, 0);
> >   
> > -	igt_assert(!(pipe_crc->flags & O_NONBLOCK));
> >   	do {
> >   		read_one_crc(pipe_crc, crc);
> >   
> > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> > index 9f81be0a24de..58ca07f5c15d 100644
> > --- a/lib/igt_debugfs.h
> > +++ b/lib/igt_debugfs.h
> > @@ -132,7 +132,7 @@ __attribute__((warn_unused_result))
> >   int igt_pipe_crc_get_crcs(igt_pipe_crc_t *pipe_crc, int n_crcs,
> >   			  igt_crc_t **out_crcs);
> >   void igt_pipe_crc_drain(igt_pipe_crc_t *pipe_crc);
> > -bool igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc);
> > +void igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc);
> >   void igt_pipe_crc_get_current(int drm_fd, igt_pipe_crc_t *pipe_crc, igt_crc_t *crc);
> >   
> >   void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc);
> > diff --git a/tests/kms_chv_cursor_fail.c b/tests/kms_chv_cursor_fail.c
> > index 7138e549aeb7..8e50978d0be1 100644
> > --- a/tests/kms_chv_cursor_fail.c
> > +++ b/tests/kms_chv_cursor_fail.c
> > @@ -258,7 +258,7 @@ static void prepare_crtc(data_t *data)
> >   
> >   	/* get reference crc w/o cursor */
> >   	igt_pipe_crc_start(data->pipe_crc);
> > -	igt_assert(igt_pipe_crc_get_single(data->pipe_crc, &data->ref_crc));
> > +	igt_pipe_crc_get_single(data->pipe_crc, &data->ref_crc);
> >   }
> >   
> >   static void test_crtc(data_t *data, unsigned int edges)
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev