Re: [PATCH i-g-t] lib/igt_debugfs: Remove support for legacy CRC api.

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

 



On Thu, Oct 26, 2017 at 01:38:51PM +0200, Maarten Lankhorst wrote:
> In kernel v4.10 the legacy crc api has been replaced by a generic
> drm crc API. While at it, fix igt_require_pipe_crc, the file cannot be
> opened any more when the crtc is not active after kernel commit 8038e09be5a3
> ("drm/crc: Only open CRC on atomic drivers when the CRTC is active.").
> Statting the file should be enough for testing it's supported.

What's the impact of this change on devices running older kernels - such
as KBL ChromeOS on 4.4?

> 
> Cc: Tomi Sarvela <tomi.p.sarvela@xxxxxxxxx>
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
>  lib/igt_debugfs.c | 231 +++++++-----------------------------------------------
>  1 file changed, 28 insertions(+), 203 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 8b33b478a9a9..63a0989e5ceb 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -416,16 +416,12 @@ char *igt_crc_to_string(igt_crc_t *crc)
>  #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 LEGACY_LINE_LEN       (6 * 8 + 5 + 1)
> -
>  struct _igt_pipe_crc {
>  	int fd;
>  	int dir;
>  	int ctl_fd;
>  	int crc_fd;
>  	int flags;
> -	bool is_legacy;
>  
>  	enum pipe pipe;
>  	enum intel_pipe_crc_source source;
> @@ -449,130 +445,6 @@ static const char *pipe_crc_source_name(enum intel_pipe_crc_source source)
>          return pipe_crc_sources[source];
>  }
>  
> -static bool igt_pipe_crc_do_start(igt_pipe_crc_t *pipe_crc)
> -{
> -	char buf[64];
> -
> -	/* Stop first just to make sure we don't have lingering state left. */
> -	igt_pipe_crc_stop(pipe_crc);
> -
> -	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));
> -
> -	igt_assert_eq(write(pipe_crc->ctl_fd, buf, strlen(buf)), strlen(buf));
> -
> -	if (!pipe_crc->is_legacy) {
> -		int err;
> -
> -		sprintf(buf, "crtc-%d/crc/data", pipe_crc->pipe);
> -		err = 0;
> -
> -		pipe_crc->crc_fd = openat(pipe_crc->dir, buf, pipe_crc->flags);
> -		if (pipe_crc->crc_fd < 0)
> -			err = -errno;
> -
> -		if (err == -EINVAL)
> -			return false;
> -
> -		igt_assert_eq(err, 0);
> -	}
> -
> -	errno = 0;
> -	return true;
> -}
> -
> -static void igt_pipe_crc_pipe_off(int fd, enum pipe pipe)
> -{
> -	char buf[32];
> -
> -	sprintf(buf, "pipe %s none", kmstest_pipe_name(pipe));
> -	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> -}
> -
> -static void igt_pipe_crc_reset(int drm_fd)
> -{
> -	struct dirent *dirent;
> -	const char *cmd = "none";
> -	bool done = false;
> -	DIR *dir;
> -	int fdir;
> -	int fd;
> -
> -	fdir = igt_debugfs_dir(drm_fd);
> -	if (fdir < 0)
> -		return;
> -
> -	dir = fdopendir(fdir);
> -	if (!dir) {
> -		close(fdir);
> -		return;
> -	}
> -
> -	while ((dirent = readdir(dir))) {
> -		char buf[PATH_MAX];
> -
> -		if (strcmp(dirent->d_name, "crtc-") != 0)
> -			continue;
> -
> -		sprintf(buf, "%s/crc/control", dirent->d_name);
> -		fd = openat(fdir, buf, O_WRONLY);
> -		if (fd < 0)
> -			continue;
> -
> -		igt_assert_eq(write(fd, cmd, strlen(cmd)), strlen(cmd));
> -		close(fd);
> -
> -		done = true;
> -	}
> -	closedir(dir);
> -
> -	if (!done) {
> -		fd = openat(fdir, "i915_display_crtc_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(fdir);
> -}
> -
> -static void pipe_crc_exit_handler(int sig)
> -{
> -	struct dirent *dirent;
> -	char buf[PATH_MAX];
> -	DIR *dir;
> -	int fd;
> -
> -	dir = opendir("/dev/dri");
> -	if (!dir)
> -		return;
> -
> -	/*
> -	 * Try to reset CRC capture for all DRM devices, this is only needed
> -	 * for the legacy CRC ABI and can be completely removed once the
> -	 * legacy codepaths are removed.
> -	 */
> -	while ((dirent = readdir(dir))) {
> -		if (strncmp(dirent->d_name, "card", 4) != 0)
> -			continue;
> -
> -		sprintf(buf, "/dev/dri/%s", dirent->d_name);
> -		fd = open(buf, O_WRONLY);
> -
> -		igt_pipe_crc_reset(fd);
> -
> -		close(fd);
> -	}
> -	closedir(dir);
> -}
> -
>  /**
>   * igt_require_pipe_crc:
>   *
> @@ -582,20 +454,15 @@ static void pipe_crc_exit_handler(int sig)
>   */
>  void igt_require_pipe_crc(int fd)
>  {
> -	const char *cmd = "pipe A none";
> -	int ctl, written;
> -
> -	ctl = igt_debugfs_open(fd, "crtc-0/crc/control", O_RDONLY);
> -	if (ctl < 0) {
> -		ctl = igt_debugfs_open(fd, "i915_display_crc_ctl", O_WRONLY);
> -		igt_require_f(ctl,
> -			      "No display_crc_ctl found, kernel too old\n");
> -
> -		written = write(ctl, cmd, strlen(cmd));
> -		igt_require_f(written < 0,
> -			      "CRCs not supported on this platform\n");
> -	}
> -	close(ctl);
> +	int dir;
> +	struct stat stat;
> +
> +	dir = igt_debugfs_dir(fd);
> +	igt_require_f(dir >= 0, "Could not open debugfs directory\n");
> +	igt_require_f(fstatat(dir, "crtc-0/crc/control", &stat, 0) == 0,
> +		      "CRCs not supported on this platform\n");
> +
> +	close(dir);
>  }
>  
>  static void igt_hpd_storm_exit_handler(int sig)
> @@ -729,29 +596,13 @@ pipe_crc_new(int fd, enum pipe pipe, enum intel_pipe_crc_source source, int flag
>  	debugfs = igt_debugfs_dir(fd);
>  	igt_assert(debugfs != -1);
>  
> -	igt_install_exit_handler(pipe_crc_exit_handler);
> -
>  	pipe_crc = calloc(1, sizeof(struct _igt_pipe_crc));
>  
>  	sprintf(buf, "crtc-%d/crc/control", pipe);
>  	pipe_crc->ctl_fd = openat(debugfs, buf, O_WRONLY);
> -	if (pipe_crc->ctl_fd == -1) {
> -		pipe_crc->ctl_fd = openat(debugfs,
> -					  "i915_display_crc_ctl", O_WRONLY);
> -		igt_assert(pipe_crc->ctl_fd != -1);
> -		pipe_crc->is_legacy = true;
> -	}
> -
> -	if (pipe_crc->is_legacy) {
> -		sprintf(buf, "i915_pipe_%s_crc", kmstest_pipe_name(pipe));
> -		pipe_crc->crc_fd = openat(debugfs, 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");
> -	}
> +	igt_assert(pipe_crc->ctl_fd != -1);
>  
> +	pipe_crc->crc_fd = -1;
>  	pipe_crc->fd = fd;
>  	pipe_crc->dir = debugfs;
>  	pipe_crc->pipe = pipe;
> @@ -817,18 +668,9 @@ void igt_pipe_crc_free(igt_pipe_crc_t *pipe_crc)
>  static bool pipe_crc_init_from_string(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc,
>  				      const char *line)
>  {
> -	int n, i;
> +	int i;
>  	const char *buf;
>  
> -	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 {
> @@ -849,15 +691,9 @@ static int read_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out)
>  {
>  	ssize_t bytes_read;
>  	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, read_len);
> +	bytes_read = read(pipe_crc->crc_fd, &buf, MAX_LINE_LEN);
>  	igt_reset_timeout();
>  
>  	if (bytes_read < 0 && errno == EAGAIN)
> @@ -888,22 +724,19 @@ static void read_one_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out)
>   */
>  void igt_pipe_crc_start(igt_pipe_crc_t *pipe_crc)
>  {
> -	igt_crc_t crc;
> -
> -	igt_assert(igt_pipe_crc_do_start(pipe_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);
> -	}
> +	const char *src = pipe_crc_source_name(pipe_crc->source);
> +	char buf[32];
> +
> +	/* Stop first just to make sure we don't have lingering state left. */
> +	igt_pipe_crc_stop(pipe_crc);
> +
> +	igt_assert_eq(write(pipe_crc->ctl_fd, src, strlen(src)), strlen(src));
> +
> +	sprintf(buf, "crtc-%d/crc/data", pipe_crc->pipe);
> +
> +	pipe_crc->crc_fd = openat(pipe_crc->dir, buf, pipe_crc->flags);
> +	igt_assert(pipe_crc->crc_fd != -1);
> +	errno = 0;
>  }
>  
>  /**
> @@ -914,16 +747,8 @@ void igt_pipe_crc_start(igt_pipe_crc_t *pipe_crc)
>   */
>  void igt_pipe_crc_stop(igt_pipe_crc_t *pipe_crc)
>  {
> -	char buf[32];
> -
> -	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;
> -	}
> +	close(pipe_crc->crc_fd);
> +	pipe_crc->crc_fd = -1;
>  }
>  
>  /**
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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