On Mon, 05 Dec 2016, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
...
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
