Re: [igt-dev] [PATCH i-g-t] i915/i915_pm_rpm: Only check for suspend failures after each debugfs entry

[Martin Peres <martin.peres@linux.intel.com>]

On 2020-02-20 19:41, Chris Wilson wrote:
> Since we check before and then after each debugfs entry, we do not need
> to check before each time as well. We will error out as soon as it does
> fail, at all other times we know the system to be idle.
> 
> No impact on runtime for glk (which apparently is one of the better
> behaving systems).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Martin Peres <martin.peres@linux.intel.com>

I don't like this patch because the first read might not have the gpu
suspended, and there shouldn't be much overhead in checking twice rather
than once.

What's your rationale here?

To me, the issue is that some platforms suspend in milliseconds while
some take seconds, and that might be indicative a real bug in the driver.

Martin

> ---
>  tests/i915/i915_pm_rpm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index 0c2821122..bf412b5cc 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -932,9 +932,6 @@ static int read_entry(const char *filepath,
>  	int fd;
>  	int rc;
>  
> -	igt_assert_f(wait_for_suspended(), "Before opening: %s (%s)\n",
> -		     filepath + pathinfo->base, filepath);
> -
>  	fd = open(filepath, O_RDONLY | O_NONBLOCK);
>  	if (fd < 0) {
>  		igt_debug("Failed to open '%s': %m\n", filepath);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev