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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi tomeu,

This patch does not seem to apply cleanly on upstream/master.


Rob.

> 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>
> 
> ---
> 
> Have just rebased this, and made sure we fallback to the old ABI when
> accessing the debugfs files directly from kms_pipe_crc_basic.
> 
> Thanks,
> 
> Tomeu
> ---
>  lib/igt_debugfs.c          | 190 +++++++++++++++++++++++++++++++++
> ------------
>  lib/igt_debugfs.h          |   4 +-
>  tests/kms_pipe_crc_basic.c |  30 ++++++-
>  3 files changed, 171 insertions(+), 53 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 9142e3f78868..3d92c6a10a41 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -25,6 +25,7 @@
>  #include <inttypes.h>
>  #include <sys/stat.h>
>  #include <sys/mount.h>
> +#include <dirent.h>
>  #include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -291,25 +292,23 @@ char *igt_crc_to_string(igt_crc_t *crc)
>  {
>  	char buf[128];
>  
> -	igt_assert_eq(crc->n_words, 5);
> -
>  	sprintf(buf, "%08x %08x %08x %08x %08x", crc->crc[0],
>  		crc->crc[1], crc->crc[2], crc->crc[3], crc->crc[4]);
>  
>  	return strdup(buf);
>  }
>  
> +#define MAX_CRC_ENTRIES 10
> +#define MAX_LINE_LEN (10 + 11 * MAX_CRC_ENTRIES + 1)
> +
>  /* (6 fields, 8 chars each, space separated (5) + '\n') */
> -#define PIPE_CRC_LINE_LEN       (6 * 8 + 5 + 1)
> -/* account for \'0' */
> -#define PIPE_CRC_BUFFER_LEN     (PIPE_CRC_LINE_LEN + 1)
> +#define LEGACY_LINE_LEN       (6 * 8 + 5 + 1)
>  
>  struct _igt_pipe_crc {
>  	int ctl_fd;
>  	int crc_fd;
> -	int line_len;
> -	int buffer_len;
>  	int flags;
> +	bool is_legacy;
>  
>  	enum pipe pipe;
>  	enum intel_pipe_crc_source source;
> @@ -340,13 +339,26 @@ static bool
> igt_pipe_crc_do_start(igt_pipe_crc_t *pipe_crc)
>  	/* Stop first just to make sure we don't have lingering
> state left. */
>  	igt_pipe_crc_stop(pipe_crc);
>  
> -	sprintf(buf, "pipe %s %s", kmstest_pipe_name(pipe_crc-
> >pipe),
> -		pipe_crc_source_name(pipe_crc->source));
> +	if (pipe_crc->is_legacy)
> +		sprintf(buf, "pipe %s %s",
> kmstest_pipe_name(pipe_crc->pipe),
> +			pipe_crc_source_name(pipe_crc->source));
> +	else
> +		sprintf(buf, "%s", pipe_crc_source_name(pipe_crc-
> >source));
> +
>  	errno = 0;
>  	igt_assert_eq(write(pipe_crc->ctl_fd, buf, strlen(buf)),
> strlen(buf));
>  	if (errno != 0)
>  		return false;
>  
> +	if (!pipe_crc->is_legacy) {
> +		sprintf(buf, "crtc-%d/crc/data", pipe_crc->pipe);
> +		errno = 0;
> +		pipe_crc->crc_fd = igt_debugfs_open(buf, pipe_crc-
> >flags);
> +		if (pipe_crc->crc_fd == -1 && errno == EINVAL)
> +			return false;
> +		igt_assert_eq(errno, 0);
> +	}
> +
>  	return true;
>  }
>  
> @@ -360,15 +372,45 @@ static void igt_pipe_crc_pipe_off(int fd, enum
> pipe pipe)
>  
>  static void igt_pipe_crc_reset(void)
>  {
> +	igt_debugfs_t *debugfs = __igt_debugfs_singleton();
>  	int fd;
> +	struct dirent *dirent;
> +	char buf[128];
> +	const char *cmd = "none";
> +	bool done = false;
> +	DIR *dir;
> +
> +	dir = opendir(debugfs->dri_path);
> +	if (dir) {
> +		while ((dirent = readdir(dir))) {
> +			if (strcmp(dirent->d_name, "crtc-") != 0)
> +				continue;
> +
> +			sprintf(buf, "%s/%s/crc/control", debugfs-
> >dri_path,
> +				dirent->d_name);
> +			fd = open(buf, O_WRONLY);
> +			if (fd == -1)
> +				continue;
> +
> +			igt_assert_eq(write(fd, cmd, strlen(cmd)),
> strlen(cmd));
> +			done = true;
> +
> +			close(fd);
> +		}
> +		closedir(dir);
> +	}
>  
> -	fd = igt_debugfs_open("i915_display_crc_ctl", O_WRONLY);
> +	if (done)
> +		return;
>  
> -	igt_pipe_crc_pipe_off(fd, PIPE_A);
> -	igt_pipe_crc_pipe_off(fd, PIPE_B);
> -	igt_pipe_crc_pipe_off(fd, PIPE_C);
> +	fd = igt_debugfs_open("i915_display_crc_ctl", O_WRONLY);
> +	if (fd != -1) {
> +		igt_pipe_crc_pipe_off(fd, PIPE_A);
> +		igt_pipe_crc_pipe_off(fd, PIPE_B);
> +		igt_pipe_crc_pipe_off(fd, PIPE_C);
>  
> -	close(fd);
> +		close(fd);
> +	}
>  }
>  
>  static void pipe_crc_exit_handler(int sig)
> @@ -390,13 +432,16 @@ void igt_require_pipe_crc(void)
>  	size_t written;
>  	int ret;
>  
> -	ctl = igt_debugfs_fopen("i915_display_crc_ctl", "r+");
> -	igt_require_f(ctl,
> -		      "No display_crc_ctl found, kernel too old\n");
> -	written = fwrite(cmd, 1, strlen(cmd), ctl);
> -	ret = fflush(ctl);
> -	igt_require_f((written == strlen(cmd) && ret == 0) || errno
> != ENODEV,
> -		      "CRCs not supported on this platform\n");
> +	ctl = igt_debugfs_fopen("crtc-0/crc/control", "r+");
> +	if (!ctl) {
> +		ctl = igt_debugfs_fopen("i915_display_crc_ctl",
> "r+");
> +		igt_require_f(ctl,
> +			      "No display_crc_ctl found, kernel too
> old\n");
> +		written = fwrite(cmd, 1, strlen(cmd), ctl);
> +		ret = fflush(ctl);
> +		igt_require_f((written == strlen(cmd) && ret == 0)
> || errno != ENODEV,
> +			      "CRCs not supported on this
> platform\n");
> +	}
>  
>  	fclose(ctl);
>  }
> @@ -411,15 +456,25 @@ pipe_crc_new(enum pipe pipe, enum
> intel_pipe_crc_source source, int flags)
>  
>  	pipe_crc = calloc(1, sizeof(struct _igt_pipe_crc));
>  
> -	pipe_crc->ctl_fd = igt_debugfs_open("i915_display_crc_ctl",
> O_WRONLY);
> -	igt_assert(pipe_crc->ctl_fd != -1);
> +	sprintf(buf, "crtc-%d/crc/control", pipe);
> +	pipe_crc->ctl_fd = igt_debugfs_open(buf, O_WRONLY);
> +	if (pipe_crc->ctl_fd == -1) {
> +		pipe_crc->ctl_fd =
> igt_debugfs_open("i915_display_crc_ctl",
> +						    O_WRONLY);
> +		igt_assert(pipe_crc->ctl_fd != -1);
> +		pipe_crc->is_legacy = true;
> +	}
>  
> -	sprintf(buf, "i915_pipe_%s_crc", kmstest_pipe_name(pipe));
> -	pipe_crc->crc_fd = igt_debugfs_open(buf, flags);
> -	igt_assert(pipe_crc->crc_fd != -1);
> +	if (pipe_crc->is_legacy) {
> +		sprintf(buf, "i915_pipe_%s_crc",
> kmstest_pipe_name(pipe));
> +		pipe_crc->crc_fd = igt_debugfs_open(buf, flags);
> +		igt_assert(pipe_crc->crc_fd != -1);
> +		igt_debug("Using legacy frame CRC ABI\n");
> +	} else {
> +		pipe_crc->crc_fd = -1;
> +		igt_debug("Using generic frame CRC ABI\n");
> +	}
>  
> -	pipe_crc->line_len = PIPE_CRC_LINE_LEN;
> -	pipe_crc->buffer_len = PIPE_CRC_BUFFER_LEN;
>  	pipe_crc->pipe = pipe;
>  	pipe_crc->source = source;
>  	pipe_crc->flags = flags;
> @@ -479,34 +534,59 @@ void igt_pipe_crc_free(igt_pipe_crc_t
> *pipe_crc)
>  	free(pipe_crc);
>  }
>  
> -static bool pipe_crc_init_from_string(igt_crc_t *crc, const char
> *line)
> +static bool pipe_crc_init_from_string(igt_pipe_crc_t *pipe_crc,
> igt_crc_t *crc,
> +				      const char *line)
>  {
> -	int n;
> +	int n, i;
> +	const char *buf;
>  
> -	crc->n_words = 5;
> -	n = sscanf(line, "%8u %8x %8x %8x %8x %8x", &crc->frame,
> &crc->crc[0],
> -		   &crc->crc[1], &crc->crc[2], &crc->crc[3], &crc-
> >crc[4]);
> -	return n == 6;
> +	if (pipe_crc->is_legacy) {
> +		crc->has_valid_frame = true;
> +		crc->n_words = 5;
> +		n = sscanf(line, "%8u %8x %8x %8x %8x %8x", &crc-
> >frame,
> +			   &crc->crc[0], &crc->crc[1], &crc->crc[2],
> +			   &crc->crc[3], &crc->crc[4]);
> +		return n == 6;
> +	}
> +
> +	if (strncmp(line, "XXXXXXXXXX", 10) == 0)
> +		crc->has_valid_frame = false;
> +	else {
> +		crc->has_valid_frame = true;
> +		crc->frame = strtoul(line, NULL, 16);
> +	}
> +
> +	buf = line + 10;
> +	for (i = 0; *buf != '\n'; i++, buf += 11)
> +		crc->crc[i] = strtoul(buf, NULL, 16);
> +
> +	crc->n_words = i;
> +
> +	return true;
>  }
>  
>  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);
>  	}
>  	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");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux