Re: [PATCH i-g-t v4] lib/debugfs: Support new generic ABI for CRC capture

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 05 Dec 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> The kernel has now a new debugfs ABI that can also allow capturing frame
> CRCs for drivers other than i915.
>
> Add alternative codepaths so the new ABI is used if the kernel is recent
> enough, and fall back to the legacy ABI if not.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

...

>  static int read_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out)
>  {
>  	ssize_t bytes_read;
> -	char buf[pipe_crc->buffer_len];
> +	char buf[MAX_LINE_LEN + 1];
> +	size_t read_len;
> +
> +	if (pipe_crc->is_legacy)
> +		read_len = LEGACY_LINE_LEN;
> +	else
> +		read_len = MAX_LINE_LEN;
>  
>  	igt_set_timeout(5, "CRC reading");
> -	bytes_read = read(pipe_crc->crc_fd, &buf, pipe_crc->line_len);
> +	bytes_read = read(pipe_crc->crc_fd, &buf, read_len);
>  	igt_reset_timeout();
>  
>  	if (bytes_read < 0 && errno == EAGAIN) {
>  		igt_assert(pipe_crc->flags & O_NONBLOCK);
>  		bytes_read = 0;
> -	} else {
> -		igt_assert_eq(bytes_read, pipe_crc->line_len);
>  	}

Leaving out the else branch leads to

igt_debugfs.c: In function 'read_crc':
igt_debugfs.c:604:5: error: array subscript is below array bounds [-Werror=array-bounds]
  buf[bytes_read] = '\0';
     ^

as bytes_read may end up being < 0.

BR,
Jani.


>  	buf[bytes_read] = '\0';
>  
> -	if (bytes_read && !pipe_crc_init_from_string(out, buf))
> +	if (bytes_read && !pipe_crc_init_from_string(pipe_crc, out, buf))
>  		return -EINVAL;
>  
>  	return bytes_read;
> @@ -530,15 +610,18 @@ void igt_pipe_crc_start(igt_pipe_crc_t *pipe_crc)
>  
>  	igt_assert(igt_pipe_crc_do_start(pipe_crc));
>  
> -	/*
> -	 * For some no yet identified reason, the first CRC is bonkers. So
> -	 * let's just wait for the next vblank and read out the buggy result.
> -	 *
> -	 * On CHV sometimes the second CRC is bonkers as well, so don't trust
> -	 * that one either.
> -	 */
> -	read_one_crc(pipe_crc, &crc);
> -	read_one_crc(pipe_crc, &crc);
> +	if (pipe_crc->is_legacy) {
> +		/*
> +		 * For some no yet identified reason, the first CRC is
> +		 * bonkers. So let's just wait for the next vblank and read
> +		 * out the buggy result.
> +		 *
> +		 * On CHV sometimes the second CRC is bonkers as well, so
> +		 * don't trust that one either.
> +		 */
> +		read_one_crc(pipe_crc, &crc);
> +		read_one_crc(pipe_crc, &crc);
> +	}
>  }
>  
>  /**
> @@ -551,8 +634,15 @@ void igt_pipe_crc_stop(igt_pipe_crc_t *pipe_crc)
>  {
>  	char buf[32];
>  
> -	sprintf(buf, "pipe %s none", kmstest_pipe_name(pipe_crc->pipe));
> -	igt_assert_eq(write(pipe_crc->ctl_fd, buf, strlen(buf)), strlen(buf));
> +	if (pipe_crc->is_legacy) {
> +		sprintf(buf, "pipe %s none",
> +			kmstest_pipe_name(pipe_crc->pipe));
> +		igt_assert_eq(write(pipe_crc->ctl_fd, buf, strlen(buf)),
> +			      strlen(buf));
> +	} else {
> +		close(pipe_crc->crc_fd);
> +		pipe_crc->crc_fd = -1;
> +	}
>  }
>  
>  /**
> diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> index fb189322633f..4c6572cd244c 100644
> --- a/lib/igt_debugfs.h
> +++ b/lib/igt_debugfs.h
> @@ -62,6 +62,7 @@ bool igt_debugfs_search(const char *filename, const char *substring);
>   */
>  typedef struct _igt_pipe_crc igt_pipe_crc_t;
>  
> +#define DRM_MAX_CRC_NR 10
>  /**
>   * igt_crc_t:
>   * @frame: frame number of the capture CRC
> @@ -73,8 +74,9 @@ typedef struct _igt_pipe_crc igt_pipe_crc_t;
>   */
>  typedef struct {
>  	uint32_t frame;
> +	bool has_valid_frame;
>  	int n_words;
> -	uint32_t crc[5];
> +	uint32_t crc[DRM_MAX_CRC_NR];
>  } igt_crc_t;
>  
>  /**
> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> index 04d5a1345760..b106f9bd05ee 100644
> --- a/tests/kms_pipe_crc_basic.c
> +++ b/tests/kms_pipe_crc_basic.c
> @@ -49,6 +49,8 @@ static void test_bad_command(data_t *data, const char *cmd)
>  	size_t written;
>  
>  	ctl = igt_debugfs_fopen("i915_display_crc_ctl", "r+");
> +	igt_require(ctl);
> +
>  	written = fwrite(cmd, 1, strlen(cmd), ctl);
>  	fflush(ctl);
>  	igt_assert_eq(written, strlen(cmd));
> @@ -58,6 +60,30 @@ static void test_bad_command(data_t *data, const char *cmd)
>  	fclose(ctl);
>  }
>  
> +static void test_bad_source(data_t *data)
> +{
> +	FILE *f;
> +	size_t written;
> +	const char *source = "foo";
> +
> +	f = igt_debugfs_fopen("crtc-0/crc/control", "w");
> +	if (!f) {
> +		test_bad_command(data, "pipe A foo");
> +		return;
> +	}
> +
> +	written = fwrite(source, 1, strlen(source), f);
> +	fflush(f);
> +	igt_assert_eq(written, strlen(source));
> +	igt_assert(!ferror(f));
> +	igt_assert(!errno);
> +	fclose(f);
> +
> +	f = igt_debugfs_fopen("crtc-0/crc/data", "w");
> +	igt_assert(!f);
> +	igt_assert_eq(errno, EINVAL);
> +}
> +
>  #define N_CRCS	3
>  
>  #define TEST_SEQUENCE (1<<0)
> @@ -185,7 +211,7 @@ igt_main
>  	igt_skip_on_simulation();
>  
>  	igt_fixture {
> -		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> +		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>  
>  		igt_enable_connectors();
>  
> @@ -200,7 +226,7 @@ igt_main
>  		test_bad_command(&data, "pipe D none");
>  
>  	igt_subtest("bad-source")
> -		test_bad_command(&data, "pipe A foo");
> +		test_bad_source(&data);
>  
>  	igt_subtest("bad-nb-words-1")
>  		test_bad_command(&data, "pipe foo");

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx