[PATCH] drm/i915: fix pm refcounting on fence error in execbuf

[PATCH] drm/i915: fix pm refcounting on fence error in execbuf

[Daniele Ceraolo Spurio]

Fences are creted/checked before the pm ref is taken, so if we jump to
pre_mutex_err we will uncorrectly call intel_runtime_pm_put.

Fixes: fec0445caa27 (drm/i915: Support explicit fencing for execbuf)
Testcase: igt/gem_exec_params
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a40ade6..7fb8bad 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1644,17 +1644,15 @@ static void eb_export_fence(struct drm_i915_gem_object *obj,
 	if (args->flags & I915_EXEC_FENCE_IN) {
 		in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2));
 		if (!in_fence) {
-			ret = -EINVAL;
-			goto pre_mutex_err;
+			return -EINVAL;
 		}
 	}
 
 	if (args->flags & I915_EXEC_FENCE_OUT) {
 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
 		if (out_fence_fd < 0) {
-			ret = out_fence_fd;
-			out_fence_fd = -1;
-			goto pre_mutex_err;
+			dma_fence_put(in_fence);
+			return out_fence_fd;
 		}
 	}
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] tests/gem_exec_params: add test for exec_fence params

[Daniele Ceraolo Spurio]

Added a subtest for invalid FENCE_IN usage, updated invalid-flag subtest
and made the rsvd2 test skip when exec fences are available.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 lib/ioctl_wrappers.c    | 29 +++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h    |  1 +
 tests/gem_exec_fence.c  | 14 --------------
 tests/gem_exec_params.c | 24 +++++++++++++++++++++---
 4 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index ccc5ccf..cd0c24b 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1433,6 +1433,35 @@ bool gem_has_softpin(int fd)
 	return has_softpin;
 }
 
+#define LOCAL_PARAM_HAS_EXEC_FENCE 44
+/**
+ * gem_has_exec_fence:
+ * @fd: open i915 drm file descriptor
+ *
+ * Feature test macro to query whether in/out fence support in execbuffer is
+ * available.
+ *
+ * Returns: Whether fence support is available
+ */
+bool gem_has_exec_fence(int fd)
+{
+	static int has_exec_fence = -1;
+
+	if (has_exec_fence < 0) {
+		struct drm_i915_getparam gp;
+
+		memset(&gp, 0, sizeof(gp));
+		gp.param = LOCAL_PARAM_HAS_EXEC_FENCE;
+		gp.value = &has_exec_fence;
+
+		has_exec_fence = 0;
+		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		errno = 0;
+	}
+
+	return has_exec_fence;
+}
+
 /**
  * gem_require_caching:
  * @fd: open i915 drm file descriptor
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 5f3bbd1..64628df 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -164,6 +164,7 @@ uint64_t gem_aperture_size(int fd);
 uint64_t gem_global_aperture_size(int fd);
 uint64_t gem_mappable_aperture_size(void);
 bool gem_has_softpin(int fd);
+bool gem_has_exec_fence(int fd);
 
 /* check functions which auto-skip tests by calling igt_skip() */
 void gem_require_caching(int fd);
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 3df436a..ddc5e7f 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -309,20 +309,6 @@ static void test_fence_flip(int i915)
 	igt_skip_on_f(1, "no fence-in for atomic flips\n");
 }
 
-static bool gem_has_exec_fence(int fd)
-{
-	struct drm_i915_getparam gp;
-	int val = -1;
-
-	memset(&gp, 0, sizeof(gp));
-	gp.param = LOCAL_PARAM_HAS_EXEC_FENCE;
-	gp.value = &val;
-
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
-
-	return val > 0;
-}
-
 igt_main
 {
 	const struct intel_execution_engine *e;
diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
index f9a2054..68cdef6 100644
--- a/tests/gem_exec_params.c
+++ b/tests/gem_exec_params.c
@@ -45,6 +45,8 @@
 #define LOCAL_I915_EXEC_BSD_RING1 (1<<13)
 #define LOCAL_I915_EXEC_BSD_RING2 (2<<13)
 #define LOCAL_I915_EXEC_RESOURCE_STREAMER (1<<15)
+#define LOCAL_I915_EXEC_FENCE_IN (1 << 16)
+#define LOCAL_I915_EXEC_FENCE_OUT (1 << 17)
 
 static bool has_ring(int fd, unsigned ring_exec_flags)
 {
@@ -239,13 +241,15 @@ igt_main
 				    &execbuf) == 0);
 	}
 
-	/* HANDLE_LUT and NO_RELOC are already exercised by gem_exec_lut_handle */
+	/* HANDLE_LUT and NO_RELOC are already exercised by gem_exec_lut_handle,
+	 * EXEC_FENCE_IN and EXEC_FENCE_OUT correct usage is tested by
+	 * gem_exec_fence, invalid usage of EXEC_FENCE_IN is tested below. */
 
 	igt_subtest("invalid-flag") {
 		/* NOTE: This test intentionally exercise the next available
 		 * flag. Don't "fix" this testcase without adding the required
 		 * tests for the new flag first. */
-		execbuf.flags = I915_EXEC_RENDER | (LOCAL_I915_EXEC_RESOURCE_STREAMER << 1);
+		execbuf.flags = I915_EXEC_RENDER | (LOCAL_I915_EXEC_FENCE_OUT << 1);
 		RUN_FAIL(EINVAL);
 	}
 
@@ -283,6 +287,21 @@ igt_main
 		RUN_FAIL(EINVAL);
 	}
 
+	igt_subtest("invalid-fence-in") {
+		igt_require(gem_has_exec_fence(fd));
+		execbuf.flags = LOCAL_I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = -1;
+		RUN_FAIL(EINVAL);
+	}
+
+	igt_subtest("rsvd2-dirt") {
+		igt_require(!gem_has_exec_fence(fd));
+		execbuf.flags = 0;
+		execbuf.rsvd2 = 1;
+		RUN_FAIL(EINVAL);
+		execbuf.rsvd2 = 0;
+	}
+
 #define DIRT(name) \
 	igt_subtest(#name "-dirt") { \
 		execbuf.flags = 0; \
@@ -291,7 +310,6 @@ igt_main
 		execbuf.name = 0; \
 	}
 
-	DIRT(rsvd2);
 	DIRT(cliprects_ptr);
 	DIRT(DR1);
 	DIRT(DR4);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix pm refcounting on fence error in execbuf

[Chris Wilson]

On Fri, Feb 03, 2017 at 02:45:29PM -0800, Daniele Ceraolo Spurio wrote:
> Fences are creted/checked before the pm ref is taken, so if we jump to
> pre_mutex_err we will uncorrectly call intel_runtime_pm_put.
> 
> Fixes: fec0445caa27 (drm/i915: Support explicit fencing for execbuf)
> Testcase: igt/gem_exec_params
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Sigh. The tree I was using has this:

	if (args->flags & I915_EXEC_FENCE_IN) {
		in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2));
		if (!in_fence)
			return -EINVAL;
	}

	if (args->flags & I915_EXEC_FENCE_OUT) {
		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
		if (out_fence_fd < 0) {
			ret = out_fence_fd;
			goto err_in_fence;
		}
	}

	...

	err_unlock:
		mutex_unlock(&dev->struct_mutex);
	err_rpm:
		intel_runtime_pm_put(eb.i915);
		eb_destroy(&eb);
		if (out_fence_fd != -1)
			put_unused_fd(out_fence_fd);
	err_in_fence:
		dma_fence_put(in_fence);
		return ret;
	}

Transforming the unwind sequence to match would be appreciated.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_exec_params: add test for exec_fence params

[Chris Wilson]

On Fri, Feb 03, 2017 at 02:45:30PM -0800, Daniele Ceraolo Spurio wrote:
> @@ -283,6 +287,21 @@ igt_main
>  		RUN_FAIL(EINVAL);
>  	}
>  
> +	igt_subtest("invalid-fence-in") {
> +		igt_require(gem_has_exec_fence(fd));
> +		execbuf.flags = LOCAL_I915_EXEC_FENCE_IN;
> +		execbuf.rsvd2 = -1;
> +		RUN_FAIL(EINVAL);

Another good invalid fd to test would be the device fd itself.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix pm refcounting on fence error in execbuf

[Chris Wilson]

On Fri, Feb 03, 2017 at 10:55:32PM +0000, Chris Wilson wrote:
> On Fri, Feb 03, 2017 at 02:45:29PM -0800, Daniele Ceraolo Spurio wrote:
> > Fences are creted/checked before the pm ref is taken, so if we jump to
> > pre_mutex_err we will uncorrectly call intel_runtime_pm_put.
> > 
> > Fixes: fec0445caa27 (drm/i915: Support explicit fencing for execbuf)
> > Testcase: igt/gem_exec_params
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Sigh. The tree I was using has this:
> 
> 	if (args->flags & I915_EXEC_FENCE_IN) {
> 		in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2));
> 		if (!in_fence)
> 			return -EINVAL;
> 	}
> 
> 	if (args->flags & I915_EXEC_FENCE_OUT) {
> 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> 		if (out_fence_fd < 0) {
> 			ret = out_fence_fd;
> 			goto err_in_fence;
> 		}
> 	}
> 
> 	...
> 
> 	err_unlock:
> 		mutex_unlock(&dev->struct_mutex);
> 	err_rpm:
> 		intel_runtime_pm_put(eb.i915);
> 		eb_destroy(&eb);
> 		if (out_fence_fd != -1)
> 			put_unused_fd(out_fence_fd);
> 	err_in_fence:
> 		dma_fence_put(in_fence);
> 		return ret;
> 	}
> 
> Transforming the unwind sequence to match would be appreciated.

Just in case I wasn't clear, just do the unwind gotos for the out_fence,
i.e add err_in_fence:
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: fix pm refcounting on fence error in execbuf

[Patchwork]

== Series Details ==

Series: drm/i915: fix pm refcounting on fence error in execbuf
URL   : https://patchwork.freedesktop.org/series/19087/
State : warning

== Summary ==

Series 19087v1 drm/i915: fix pm refcounting on fence error in execbuf
https://patchwork.freedesktop.org/api/1.0/series/19087/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-edid:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:252  pass:238  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:230  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:14   pass:13   dwarn:0   dfail:0   fail:0   skip:0  
fi-ivb-3520m     total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:252  pass:229  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:252  pass:232  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:252  pass:227  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:252  pass:220  dwarn:1   dfail:0   fail:0   skip:31 
fi-snb-2600      total:252  pass:220  dwarn:0   dfail:0   fail:0   skip:32 

7d165238a46e6376f77bad1f7f54879bb414cb23 drm-tip: 2017y-02m-03d-17h-56m-24s UTC integration manifest
4f345be drm/i915: fix pm refcounting on fence error in execbuf

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3701/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix pm refcounting on fence error in execbuf

[Chris Wilson]

On Fri, Feb 03, 2017 at 02:45:29PM -0800, Daniele Ceraolo Spurio wrote:
> Fences are creted/checked before the pm ref is taken, so if we jump to
> pre_mutex_err we will uncorrectly call intel_runtime_pm_put.
> 
> Fixes: fec0445caa27 (drm/i915: Support explicit fencing for execbuf)
> Testcase: igt/gem_exec_params
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Pushed this patch and the update to gem_exec_params, thanks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx