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

["Juha-Pekka Heikkilä" <juhapekka.heikkila@gmail.com>]

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.
>    */
> -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)
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev