[PATCH] drm/i915: Hang counting is now always per-fd, so relax the ioctl for DEFAULT_CONTEXT

[PATCH] drm/i915: Hang counting is now always per-fd, so relax the ioctl for DEFAULT_CONTEXT

[Chris Wilson]

Since we only count hangs towards the owner of the fd issuing the
command, we can allow that fd to inspect its own default context without
leaking global information. We introduced per-fd accounting with

commit 0eea67eb26000657079b7fc41079097942339452
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Fri Dec 6 14:11:19 2013 -0800

    drm/i915: Create a per file_priv default context

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/intel_uncore.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2adf6aa..f8f13ae 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -930,9 +930,6 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
 	if (args->flags || args->pad)
 		return -EINVAL;
 
-	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
-- 
1.7.9.5

Re: [PATCH] drm/i915: Hang counting is now always per-fd, so relax the ioctl for DEFAULT_CONTEXT

[Ben Widawsky]

On Thu, May 01, 2014 at 08:18:44AM +0100, Chris Wilson wrote:
> Since we only count hangs towards the owner of the fd issuing the
> command, we can allow that fd to inspect its own default context without
> leaking global information. We introduced per-fd accounting with
> 
> commit 0eea67eb26000657079b7fc41079097942339452
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Fri Dec 6 14:11:19 2013 -0800
> 
>     drm/i915: Create a per file_priv default context
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>

Acked-by: Ben Widawsky <ben@bwidawsk.net>

I can bump it to a r-b if Mika doesn't

> ---
>  drivers/gpu/drm/i915/intel_uncore.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2adf6aa..f8f13ae 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -930,9 +930,6 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
>  	if (args->flags || args->pad)
>  		return -EINVAL;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);
>  	if (ret)
>  		return ret;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Hang counting is now always per-fd, so relax the ioctl for DEFAULT_CONTEXT

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Since we only count hangs towards the owner of the fd issuing the
> command, we can allow that fd to inspect its own default context without
> leaking global information. We introduced per-fd accounting with
>
> commit 0eea67eb26000657079b7fc41079097942339452
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Fri Dec 6 14:11:19 2013 -0800
>
>     drm/i915: Create a per file_priv default context
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>

I think we need ack from Kenneth too.
I will fix testcase to not expect EPERM for nonroot.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_uncore.c |    3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2adf6aa..f8f13ae 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -930,9 +930,6 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
>  	if (args->flags || args->pad)
>  		return -EINVAL;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);
>  	if (ret)
>  		return ret;
> -- 
> 1.7.9.5

[PATCH 1/1] tests/gem_reset_stats: EPERM check not needed

[Mika Kuoppala]

Without root we used to get EPERM if we tried to aquire
default context statistics. Now we have per fd statistics
so only thing which is hidden from us without root access,
is the global reset count.

Drop EPERM and check that global reset count is always zero
for nonroot.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 tests/gem_reset_stats.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
index c227798..898f696 100644
--- a/tests/gem_reset_stats.c
+++ b/tests/gem_reset_stats.c
@@ -913,11 +913,7 @@ static void test_reset_count(const bool create_ctx)
 		igt_drop_root();
 
 		c2 = get_reset_count(fd, ctx);
-
-		if (ctx == 0)
-			igt_assert(c2 == -EPERM);
-		else
-			igt_assert(c2 == 0);
+		igt_assert(c2 == 0);
 	}
 
 	igt_waitchildren();
@@ -952,45 +948,37 @@ static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad)
 	return 0;
 }
 
-typedef enum { root = 0, user } cap_t;
-
-static void _check_param_ctx(const int fd, const int ctx, const cap_t cap)
+static void _check_param_ctx(const int fd, const int ctx)
 {
 	const uint32_t bad = rand() + 1;
 
-	if (ctx == 0) {
-		if (cap == root)
-			igt_assert(_test_params(fd, ctx, 0, 0) == 0);
-		else
-			igt_assert(_test_params(fd, ctx, 0, 0) == -EPERM);
-	}
-
+	igt_assert(_test_params(fd, ctx, 0, 0) == 0);
 	igt_assert(_test_params(fd, ctx, 0, bad) == -EINVAL);
 	igt_assert(_test_params(fd, ctx, bad, 0) == -EINVAL);
 	igt_assert(_test_params(fd, ctx, bad, bad) == -EINVAL);
 }
 
-static void check_params(const int fd, const int ctx, cap_t cap)
+static void check_params(const int fd, const int ctx)
 {
 	igt_assert(ioctl(fd, GET_RESET_STATS_IOCTL, 0) == -1);
 	igt_assert(_test_params(fd, 0xbadbad, 0, 0) == -ENOENT);
 
-	_check_param_ctx(fd, ctx, cap);
+	_check_param_ctx(fd, ctx);
 }
 
 static void _test_param(const int fd, const int ctx)
 {
-	check_params(fd, ctx, root);
+	check_params(fd, ctx);
 
 	igt_fork(child, 1) {
-		check_params(fd, ctx, root);
+		check_params(fd, ctx);
 
 		igt_drop_root();
 
-		check_params(fd, ctx, user);
+		check_params(fd, ctx);
 	}
 
-	check_params(fd, ctx, root);
+	check_params(fd, ctx);
 
 	igt_waitchildren();
 }
-- 
1.7.9.5