Re: [igt-dev] [PATCH i-g-t] lib/debugfs: Sanity check even discarded CRCs

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-12-13 at 12:57 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We currently only spot check some of the CRCs. We should check them
> all. 

Sounds like the right thing to do
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> This will probably cause failures on machines with PSR as
> it looks like the CRC captured after PSR exit is bogus. Or at least
> it is on ICL. We should probably just disable PSR whenever CRC
> capturing is active.

The frame counter got updated, but the CRC is junk. Isn't it still
useful to see make sure we get the CRC we expect with PSR, even if the
CRC is delayed by a frame?

> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106974
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  lib/igt_debugfs.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index a3aca8466658..f6ffcbe67b7b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -760,6 +760,22 @@ static bool
> pipe_crc_init_from_string(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc,
>  	return true;
>  }
>  
> +static void crc_sanity_checks(igt_crc_t *crc)
> +{
> +	bool all_zero = true;
> +
> +	for (int i = 0; i < crc->n_words; i++) {
> +		igt_warn_on_f(crc->crc[i] == 0xffffffff,
> +			      "Suspicious CRC: it looks like the CRC "
> +			      "read back was from a register in a
> powered "
> +			      "down well\n");
> +		if (crc->crc[i])
> +			all_zero = false;
> +	}
> +
> +	igt_warn_on_f(all_zero, "Suspicious CRC: All values are 0.\n");
> +}
> +
>  static int read_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out)
>  {
>  	ssize_t bytes_read;
> @@ -777,6 +793,8 @@ static int read_crc(igt_pipe_crc_t *pipe_crc,
> igt_crc_t *out)
>  	if (bytes_read > 0 && !pipe_crc_init_from_string(pipe_crc, out,
> buf))
>  		return -EINVAL;
>  
> +	crc_sanity_checks(out);
> +
>  	return bytes_read;
>  }
>  
> @@ -884,23 +902,6 @@ igt_pipe_crc_get_crcs(igt_pipe_crc_t *pipe_crc,
> int n_crcs,
>  	return n;
>  }
>  
> -static void crc_sanity_checks(igt_crc_t *crc)
> -{
> -	int i;
> -	bool all_zero = true;
> -
> -	for (i = 0; i < crc->n_words; i++) {
> -		igt_warn_on_f(crc->crc[i] == 0xffffffff,
> -			      "Suspicious CRC: it looks like the CRC "
> -			      "read back was from a register in a
> powered "
> -			      "down well\n");
> -		if (crc->crc[i])
> -			all_zero = false;
> -	}
> -
> -	igt_warn_on_f(all_zero, "Suspicious CRC: All values are 0.\n");
> -}
> -
>  /**
>   * igt_pipe_crc_collect_crc:
>   * @pipe_crc: pipe CRC object
> @@ -927,8 +928,6 @@ void igt_pipe_crc_collect_crc(igt_pipe_crc_t
> *pipe_crc, igt_crc_t *out_crc)
>  	igt_pipe_crc_start(pipe_crc);
>  	read_one_crc(pipe_crc, out_crc);
>  	igt_pipe_crc_stop(pipe_crc);
> -
> -	crc_sanity_checks(out_crc);
>  }
>  
>  /**
> @@ -971,8 +970,6 @@ void igt_pipe_crc_drain(igt_pipe_crc_t *pipe_crc)
>  void igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t
> *crc)
>  {
>  	read_one_crc(pipe_crc, crc);
> -
> -	crc_sanity_checks(crc);
>  }
>  
>  /**
> @@ -1000,8 +997,6 @@ igt_pipe_crc_get_current(int drm_fd,
> igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
>  			return;
>  		}
>  	} while (crc->frame <= vblank);
> -
> -	crc_sanity_checks(crc);
>  }
>  
>  /*

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev