[RFC] i915: Add fence fds to execbuffer2 uapi

[RFC] i915: Add fence fds to execbuffer2 uapi

[Chad Versace]

Let the hight 32 bits of drm_i915_gem_execbuffer2::rsvd1 contain an
input and/or output fence fd, whose presence is controlled by flags.
Also add I915_PARAM_HAS_FENCE_FD.

Signed-off-by: Chad Versace <chad.versace@intel.com>
---
 include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index c17d63d..6f26b79 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -361,6 +361,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_GPU_RESET	 35
 #define I915_PARAM_HAS_RESOURCE_STREAMER 36
 #define I915_PARAM_HAS_EXEC_SOFTPIN	 37
+#define I915_PARAM_HAS_FENCE_FD		 38
 
 typedef struct drm_i915_getparam {
 	__s32 param;
@@ -742,7 +743,17 @@ struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
 	__u64 flags;
-	__u64 rsvd1; /* now used for context info */
+
+	/* The low word (bits 0:31) contains the context id.
+	 *
+	 * The high word (bits 32:63) contains an optional fence fd.  If flag
+	 * I915_EXEC_FENCE_FD_IN is set, then the high word is an input fence
+	 * fd.  The batch will not begin execution before the input fence
+	 * signals.  If flag I915_EXEC_FENCE_FD_OUT is set, then an output
+	 * fence fd is returned in the high word. The output fence will signal
+	 * after the batch completes execution. It is legal to set both flags.
+	 */
+	__u64 rsvd1;
 	__u64 rsvd2;
 };
 
@@ -788,7 +799,10 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_RESOURCE_STREAMER     (1<<15)
 
-#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER<<1)
+#define I915_EXEC_FENCE_FD_IN		(1<<16)
+#define I915_EXEC_FENCE_FD_OUT		(1<<17)
+
+#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_FENCE_FD_OUT<<1)
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -796,6 +810,12 @@ struct drm_i915_gem_execbuffer2 {
 #define i915_execbuffer2_get_context_id(eb2) \
 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
 
+#define I915_EXEC_FENCE_FD_MASK		(0xffffffff00000000)
+#define i915_execbuffer2_set_fence_fd(eb2, fence_fd) \
+	((eb2).rsvd2 = (fence_fd) & I915_EXEC_FENCE_MASK)
+#define i915_execbuffer2_get_fence_fd(eb2, fence_fd) \
+	((eb2).rsvd2 & I915_EXEC_FENCE_FD_MASK)
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
2.9.0

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

Re: [RFC] i915: Add fence fds to execbuffer2 uapi

[Chad Versace]

On Mon 27 Jun 2016, Chad Versace wrote:
> Let the hight 32 bits of drm_i915_gem_execbuffer2::rsvd1 contain an
> input and/or output fence fd, whose presence is controlled by flags.
> Also add I915_PARAM_HAS_FENCE_FD.
> 
> Signed-off-by: Chad Versace <chad.versace@intel.com>
> ---
>  include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)

Oops. git-send-email stripped the notes to the patch. Here's the notes:

This RFC proposes a uapi that integrates execbuf with Android sync fds.  Of
course, this is *only* an RFC because other devs are working on the i915
internals, and this patch depends on that work.

Why am I sending an RFC this early? I will soon begin prototyping Intel's
Mesa implementation of EGL_ANDROID_native_fence_sync, and that prototype will
be easier to write if I have a rough expectation of i915's eventual fence fd
uapi.

Please provide feedback: Does this roughly look like the uapi that the i915
devs expect?

(Pardon any stupidity in the patch. It's my first non-trivial kernel patch).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: Add fence fds to execbuffer2 uapi

[Chris Wilson]

On Mon, Jun 27, 2016 at 01:18:59PM -0700, Chad Versace wrote:
> On Mon 27 Jun 2016, Chad Versace wrote:
> > Let the hight 32 bits of drm_i915_gem_execbuffer2::rsvd1 contain an
> > input and/or output fence fd, whose presence is controlled by flags.
> > Also add I915_PARAM_HAS_FENCE_FD.
> > 
> > Signed-off-by: Chad Versace <chad.versace@intel.com>
> > ---
> >  include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> Oops. git-send-email stripped the notes to the patch. Here's the notes:
> 
> This RFC proposes a uapi that integrates execbuf with Android sync fds.  Of
> course, this is *only* an RFC because other devs are working on the i915
> internals, and this patch depends on that work.

Why not just use the earlier patches for the uAPI as well?

> Why am I sending an RFC this early? I will soon begin prototyping Intel's
> Mesa implementation of EGL_ANDROID_native_fence_sync, and that prototype will
> be easier to write if I have a rough expectation of i915's eventual fence fd
> uapi.
> 
> Please provide feedback: Does this roughly look like the uapi that the i915
> devs expect?

Not quite. You have to use separate in/out dwords (i.e. rsvd2) in order
to ensure that we don't overwite the in-fence when dealing with error
paths (i.e. so that userspace can feed in the same execbuf parameters
following EINTR, and you don't have confusion between in/out parameters).
You have to also mark the ioctl as writing the new structures which is an
ABI break and so requires a new identifier (otherwise you break userspace
passing in the args from read-only memory).

Playind devil's advocate, an alternative to every driver implementing
their own fence extension for execbuf/cmdsubmit would be to add support
for explicit sync_fences to be added via dmabuf. (Instead of setting the
fence on the execbuf, you would set the fence on the batch buffer obj,
or surface of interest - though for CreateSync, it would have to be the
batch. Extracting the fence is then supplied by querying the batch buffer
dmabuf. It's not as explicit, but I suspect such uABI will be added to
dmabuf and will be required to be supported in the driver to handle
implicit fencing between PRIME anyway.) 
-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

✓ Ro.CI.BAT: success for i915: Add fence fds to execbuffer2 uapi

[Patchwork]

== Series Details ==

Series: i915: Add fence fds to execbuffer2 uapi
URL   : https://patchwork.freedesktop.org/series/9196/
State : success

== Summary ==

Series 9196v1 i915: Add fence fds to execbuffer2 uapi
http://patchwork.freedesktop.org/api/1.0/series/9196/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-kbl-qkkr      total:229  pass:160  dwarn:29  dfail:0   fail:0   skip:40 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:3   dfail:1   fail:2   skip:21 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1315/

13bc84e drm-intel-nightly: 2016y-06m-27d-21h-27m-04s UTC integration manifest
13ba40e i915: Add fence fds to execbuffer2 uapi

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

Re: [RFC] i915: Add fence fds to execbuffer2 uapi

[John Harrison]

On 27/06/2016 21:18, Chad Versace wrote:
> On Mon 27 Jun 2016, Chad Versace wrote:
>> Let the hight 32 bits of drm_i915_gem_execbuffer2::rsvd1 contain an
>> input and/or output fence fd, whose presence is controlled by flags.
>> Also add I915_PARAM_HAS_FENCE_FD.
>>
>> Signed-off-by: Chad Versace <chad.versace@intel.com>
>> ---
>>   include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
> Oops. git-send-email stripped the notes to the patch. Here's the notes:
>
> This RFC proposes a uapi that integrates execbuf with Android sync fds.  Of
> course, this is *only* an RFC because other devs are working on the i915
> internals, and this patch depends on that work.
>
> Why am I sending an RFC this early? I will soon begin prototyping Intel's
> Mesa implementation of EGL_ANDROID_native_fence_sync, and that prototype will
> be easier to write if I have a rough expectation of i915's eventual fence fd
> uapi.
>
> Please provide feedback: Does this roughly look like the uapi that the i915
> devs expect?
>
> (Pardon any stupidity in the patch. It's my first non-trivial kernel patch).
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Not sure who you mean by 'the devs' but I am the one who implemented all 
of this for our Android releases. I have a patch series to add full 
native sync support to the i915 driver. It is currently used in our 
Android trees so has been tested quite extensively. It was also going 
through code review on the mailing list a while ago until it was decided 
that the de-staging of the Android code should be done by external 
people (currently in progress).

The latest set of patches (including changes from feedback about rsvd 
fields) never actually got posted to the mailing list due to the above 
issue with de-staging. I have just updated my FDO git account with them 
instead. The kernel patch is:
https://cgit.freedesktop.org/~johnharr/scheduler/commit/?h=all&id=b7cd5e85edce4af9d7d4c34bb640cd49e31236a8

The userland LibDRM patch is:
https://cgit.freedesktop.org/~johnharr/scheduler/commit/?h=LibDRM&id=f11b2d577904b1a096d5b36384a9cc83ba51cbb8


John.

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

Re: [RFC] i915: Add fence fds to execbuffer2 uapi

[Chad Versace]

On Mon 27 Jun 2016, Chris Wilson wrote:
> On Mon, Jun 27, 2016 at 01:18:59PM -0700, Chad Versace wrote:
> > On Mon 27 Jun 2016, Chad Versace wrote:
> > > Let the hight 32 bits of drm_i915_gem_execbuffer2::rsvd1 contain an
> > > input and/or output fence fd, whose presence is controlled by flags.
> > > Also add I915_PARAM_HAS_FENCE_FD.
> > > 
> > > Signed-off-by: Chad Versace <chad.versace@intel.com>
> > > ---
> > >  include/uapi/drm/i915_drm.h | 24 ++++++++++++++++++++++--
> > >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > Oops. git-send-email stripped the notes to the patch. Here's the notes:
> > 
> > This RFC proposes a uapi that integrates execbuf with Android sync fds.  Of
> > course, this is *only* an RFC because other devs are working on the i915
> > internals, and this patch depends on that work.
> 
> Why not just use the earlier patches for the uAPI as well?

I examined all the patches that John Harrison sent to the list, and they
contained no uapi. Is there another patchset, from someone else
(possibly you), that proposes a uapi?

> > Why am I sending an RFC this early? I will soon begin prototyping Intel's
> > Mesa implementation of EGL_ANDROID_native_fence_sync, and that prototype will
> > be easier to write if I have a rough expectation of i915's eventual fence fd
> > uapi.
> > 
> > Please provide feedback: Does this roughly look like the uapi that the i915
> > devs expect?
> 
> Not quite. You have to use separate in/out dwords (i.e. rsvd2) in order
> to ensure that we don't overwite the in-fence when dealing with error
> paths (i.e. so that userspace can feed in the same execbuf parameters
> following EINTR, and you don't have confusion between in/out parameters).

Right. I forgot about resubmission on EINTR.

> You have to also mark the ioctl as writing the new structures which is an
> ABI break and so requires a new identifier (otherwise you break userspace
> passing in the args from read-only memory).

Thanks for explaining the obvious ABI break to this kernel noob.

> Playind devil's advocate, an alternative to every driver implementing
> their own fence extension for execbuf/cmdsubmit would be to add support
> for explicit sync_fences to be added via dmabuf. (Instead of setting the
> fence on the execbuf, you would set the fence on the batch buffer obj,
> or surface of interest - though for CreateSync, it would have to be the
> batch. Extracting the fence is then supplied by querying the batch buffer
> dmabuf. It's not as explicit, but I suspect such uABI will be added to
> dmabuf and will be required to be supported in the driver to handle
> implicit fencing between PRIME anyway.) 

If I were arguing with the devil, I would claim that uapi that attached
fence fds to dma_bufs seems more elegant, but API that attaches fence
fds to batch bos prevents an optimized use case in Vulkan's submission
model.

In Vulkan, the user submits work by compiling a VkCommandBuffer (which
is closely related to Intel's batch bo) and then submitting it to
a VkQueue (which is related to a GPU ring). For repetive rendering
tasks, the Vulkan API encourages the user to re-use the compiled
VkCommandBuffer by re-submitting it to the VkQueue. When the user
resubmits a VkCommandBuffer, the Vulkan spec doesn't *require* the
driver to resubmit the same exact batch buffer; but that's the spec's
*intent*.  And Mesa does that today; when the user compiles
a VkCommandBuffer, we compile the batch buffer immediately, and resubmit
that exact batch buffer each time the user submits the VkCommandBuffer.

Vulkan doesn't use fence fds today, but it will someday.  If the kernel
doesn't add fence fds to the execbuffer ioctl, but instead requires that
the fences be associated with a batch buffer, then that would prevents
the natural batch buffer re-use in Vulkan.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: Add fence fds to execbuffer2 uapi

[Chad Versace]

On Tue 28 Jun 2016, John Harrison wrote:

> The latest set of patches (including changes from feedback about rsvd
> fields) never actually got posted to the mailing list due to the above issue
> with de-staging. I have just updated my FDO git account with them instead.
> The kernel patch is:
> https://cgit.freedesktop.org/~johnharr/scheduler/commit/?h=all&id=b7cd5e85edce4af9d7d4c34bb640cd49e31236a8
> 
> The userland LibDRM patch is:
> https://cgit.freedesktop.org/~johnharr/scheduler/commit/?h=LibDRM&id=f11b2d577904b1a096d5b36384a9cc83ba51cbb8

Thanks for the sharing the code.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx