[Intel-gfx] [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 at 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.