Re: [Intel-gfx] [PATCH v4] i915: Add support for drm syncobjs

[Jason Ekstrand]

On Thu, Aug 3, 2017 at 11:58 AM, Chris Wilson 
wrote:

> From: Jason Ekstrand 
>
> This commit adds support for waiting on or signaling DRM syncobjs as
> part of execbuf.  It does so by hijacking the currently unused cliprects
> pointer to instead point to an array of i915_gem_exec_fence structs
> which containe a DRM syncobj and a flags parameter which specifies
> whether to wait on it or to signal it.  This implementation
> theoretically allows for both flags to be set in which case it waits on
> the dma_fence that was in the syncobj and then immediately replaces it
> with the dma_fence from the current execbuf.
>
> v2:
>  - Rebase on new syncobj API
> v3:
>  - Pull everything out into helpers
>  - Do all allocation in gem_execbuffer2
>  - Pack the flags in the bottom 2 bits of the drm_syncobj*
> v4:
>  - Prevent a potential race on syncobj->fence
>
> Testcase: igt/gem_exec_fence/syncobj*
> Signed-off-by: Jason Ekstrand 
> Link: https://patchwork.freedesktop.org/patch/msgid/1499289202-
> 25441-1-git-send-email-jason.ekstr...@intel.com
> Reviewed-by: Chris Wilson 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_drv.c|   3 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 146
> -
>  include/uapi/drm/i915_drm.h|  31 +-
>  3 files changed, 172 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_
> drv.c
> index cc25115c2db7..e55d1224a74f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -388,6 +388,7 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> case I915_PARAM_HAS_EXEC_FENCE:
> case I915_PARAM_HAS_EXEC_CAPTURE:
> case I915_PARAM_HAS_EXEC_BATCH_FIRST:
> +   case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> /* For the time being all of these are always true;
>  * if some supported hardware does not have one of these
>  * features this value needs to be provided from
> @@ -2739,7 +2740,7 @@ static struct drm_driver driver = {
>  */
> .driver_features =
> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> DRIVER_PRIME |
> -   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC,
> +   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC |
> DRIVER_SYNCOBJ,
> .release = i915_driver_release,
> .open = i915_driver_open,
> .lastclose = i915_driver_lastclose,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 5fa44767c29e..c4db64cfcdae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -32,6 +32,7 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
>  #include "i915_drv.h"
> @@ -1884,8 +1885,10 @@ static bool i915_gem_check_execbuffer(struct
> drm_i915_gem_execbuffer2 *exec)
> return false;
>
> /* Kernel clipping was a DRI1 misfeature */
> -   if (exec->num_cliprects || exec->cliprects_ptr)
> -   return false;
> +   if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) {
> +   if (exec->num_cliprects || exec->cliprects_ptr)
> +   return false;
> +   }
>
> if (exec->DR4 == 0x) {
> DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> @@ -2116,11 +2119,125 @@ eb_select_engine(struct drm_i915_private
> *dev_priv,
> return engine;
>  }
>
> +static void __free_fence_array(struct drm_syncobj **fences, unsigned int
> n)
> +{
> +   while (n--)
> +   drm_syncobj_put(ptr_mask_bits(fences[n], 2));
> +   kvfree(fences);
> +}
> +
> +static struct drm_syncobj **get_fence_array(struct
> drm_i915_gem_execbuffer2 *args,
> +   struct drm_file *file)
> +{
> +   const unsigned int nfences = args->num_cliprects;
> +   struct drm_i915_gem_exec_fence __user *user;
> +   struct drm_syncobj **fences;
> +   unsigned int n;
> +   int err;
> +
> +   if (!(args->flags & I915_EXEC_FENCE_ARRAY))
> +   return NULL;
> +
> +   if (nfences > SIZE_MAX / sizeof(*fences))
> +   return ERR_PTR(-EINVAL);
> +
> +   user = u64_to_user_ptr(args->cliprects_ptr);
> +   if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
> +   return ERR_PTR(-EFAULT);
> +
> +   fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
> +   __GFP_NOWARN | GFP_TEMPORARY);
> +   if (!fences)
> +   return ERR_PTR(-ENOMEM);
> +
> +   for (n = 0; n < nfences; n++) {
> +   struct drm_i915_gem_exec_fence fence;
> +   struct drm_syncobj *syncobj;
> +
> +   if (__copy_from_user(, 

Re: [Intel-gfx] [PATCH v4] i915: Add support for drm syncobjs

[Jason Ekstrand]

Here's a re-spin of the userspace bits:

https://patchwork.freedesktop.org/series/27278/

On Thu, Aug 3, 2017 at 11:58 AM, Chris Wilson 
wrote:

> From: Jason Ekstrand 
>
> This commit adds support for waiting on or signaling DRM syncobjs as
> part of execbuf.  It does so by hijacking the currently unused cliprects
> pointer to instead point to an array of i915_gem_exec_fence structs
> which containe a DRM syncobj and a flags parameter which specifies
> whether to wait on it or to signal it.  This implementation
> theoretically allows for both flags to be set in which case it waits on
> the dma_fence that was in the syncobj and then immediately replaces it
> with the dma_fence from the current execbuf.
>
> v2:
>  - Rebase on new syncobj API
> v3:
>  - Pull everything out into helpers
>  - Do all allocation in gem_execbuffer2
>  - Pack the flags in the bottom 2 bits of the drm_syncobj*
> v4:
>  - Prevent a potential race on syncobj->fence
>
> Testcase: igt/gem_exec_fence/syncobj*
> Signed-off-by: Jason Ekstrand 
> Link: https://patchwork.freedesktop.org/patch/msgid/1499289202-
> 25441-1-git-send-email-jason.ekstr...@intel.com
> Reviewed-by: Chris Wilson 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_drv.c|   3 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 146
> -
>  include/uapi/drm/i915_drm.h|  31 +-
>  3 files changed, 172 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_
> drv.c
> index cc25115c2db7..e55d1224a74f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -388,6 +388,7 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> case I915_PARAM_HAS_EXEC_FENCE:
> case I915_PARAM_HAS_EXEC_CAPTURE:
> case I915_PARAM_HAS_EXEC_BATCH_FIRST:
> +   case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> /* For the time being all of these are always true;
>  * if some supported hardware does not have one of these
>  * features this value needs to be provided from
> @@ -2739,7 +2740,7 @@ static struct drm_driver driver = {
>  */
> .driver_features =
> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> DRIVER_PRIME |
> -   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC,
> +   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC |
> DRIVER_SYNCOBJ,
> .release = i915_driver_release,
> .open = i915_driver_open,
> .lastclose = i915_driver_lastclose,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 5fa44767c29e..c4db64cfcdae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -32,6 +32,7 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
>  #include "i915_drv.h"
> @@ -1884,8 +1885,10 @@ static bool i915_gem_check_execbuffer(struct
> drm_i915_gem_execbuffer2 *exec)
> return false;
>
> /* Kernel clipping was a DRI1 misfeature */
> -   if (exec->num_cliprects || exec->cliprects_ptr)
> -   return false;
> +   if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) {
> +   if (exec->num_cliprects || exec->cliprects_ptr)
> +   return false;
> +   }
>
> if (exec->DR4 == 0x) {
> DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> @@ -2116,11 +2119,125 @@ eb_select_engine(struct drm_i915_private
> *dev_priv,
> return engine;
>  }
>
> +static void __free_fence_array(struct drm_syncobj **fences, unsigned int
> n)
> +{
> +   while (n--)
> +   drm_syncobj_put(ptr_mask_bits(fences[n], 2));
> +   kvfree(fences);
> +}
> +
> +static struct drm_syncobj **get_fence_array(struct
> drm_i915_gem_execbuffer2 *args,
> +   struct drm_file *file)
> +{
> +   const unsigned int nfences = args->num_cliprects;
> +   struct drm_i915_gem_exec_fence __user *user;
> +   struct drm_syncobj **fences;
> +   unsigned int n;
> +   int err;
> +
> +   if (!(args->flags & I915_EXEC_FENCE_ARRAY))
> +   return NULL;
> +
> +   if (nfences > SIZE_MAX / sizeof(*fences))
> +   return ERR_PTR(-EINVAL);
> +
> +   user = u64_to_user_ptr(args->cliprects_ptr);
> +   if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
> +   return ERR_PTR(-EFAULT);
> +
> +   fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
> +   __GFP_NOWARN | GFP_TEMPORARY);
> +   if (!fences)
> +   return ERR_PTR(-ENOMEM);
> +
> +   for (n = 0; n < nfences; n++) {
> +   struct drm_i915_gem_exec_fence fence;
> + 

[Intel-gfx] [PATCH v4] i915: Add support for drm syncobjs

[Chris Wilson]

From: Jason Ekstrand 

This commit adds support for waiting on or signaling DRM syncobjs as
part of execbuf.  It does so by hijacking the currently unused cliprects
pointer to instead point to an array of i915_gem_exec_fence structs
which containe a DRM syncobj and a flags parameter which specifies
whether to wait on it or to signal it.  This implementation
theoretically allows for both flags to be set in which case it waits on
the dma_fence that was in the syncobj and then immediately replaces it
with the dma_fence from the current execbuf.

v2:
 - Rebase on new syncobj API
v3:
 - Pull everything out into helpers
 - Do all allocation in gem_execbuffer2
 - Pack the flags in the bottom 2 bits of the drm_syncobj*
v4:
 - Prevent a potential race on syncobj->fence

Testcase: igt/gem_exec_fence/syncobj*
Signed-off-by: Jason Ekstrand 
Link: 
https://patchwork.freedesktop.org/patch/msgid/1499289202-25441-1-git-send-email-jason.ekstr...@intel.com
Reviewed-by: Chris Wilson 
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_drv.c|   3 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 146 -
 include/uapi/drm/i915_drm.h|  31 +-
 3 files changed, 172 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cc25115c2db7..e55d1224a74f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -388,6 +388,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_HAS_EXEC_FENCE:
case I915_PARAM_HAS_EXEC_CAPTURE:
case I915_PARAM_HAS_EXEC_BATCH_FIRST:
+   case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
/* For the time being all of these are always true;
 * if some supported hardware does not have one of these
 * features this value needs to be provided from
@@ -2739,7 +2740,7 @@ static struct drm_driver driver = {
 */
.driver_features =
DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
-   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC,
+   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ,
.release = i915_driver_release,
.open = i915_driver_open,
.lastclose = i915_driver_lastclose,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5fa44767c29e..c4db64cfcdae 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -32,6 +32,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "i915_drv.h"
@@ -1884,8 +1885,10 @@ static bool i915_gem_check_execbuffer(struct 
drm_i915_gem_execbuffer2 *exec)
return false;
 
/* Kernel clipping was a DRI1 misfeature */
-   if (exec->num_cliprects || exec->cliprects_ptr)
-   return false;
+   if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) {
+   if (exec->num_cliprects || exec->cliprects_ptr)
+   return false;
+   }
 
if (exec->DR4 == 0x) {
DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
@@ -2116,11 +2119,125 @@ eb_select_engine(struct drm_i915_private *dev_priv,
return engine;
 }
 
+static void __free_fence_array(struct drm_syncobj **fences, unsigned int n)
+{
+   while (n--)
+   drm_syncobj_put(ptr_mask_bits(fences[n], 2));
+   kvfree(fences);
+}
+
+static struct drm_syncobj **get_fence_array(struct drm_i915_gem_execbuffer2 
*args,
+   struct drm_file *file)
+{
+   const unsigned int nfences = args->num_cliprects;
+   struct drm_i915_gem_exec_fence __user *user;
+   struct drm_syncobj **fences;
+   unsigned int n;
+   int err;
+
+   if (!(args->flags & I915_EXEC_FENCE_ARRAY))
+   return NULL;
+
+   if (nfences > SIZE_MAX / sizeof(*fences))
+   return ERR_PTR(-EINVAL);
+
+   user = u64_to_user_ptr(args->cliprects_ptr);
+   if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
+   return ERR_PTR(-EFAULT);
+
+   fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
+   __GFP_NOWARN | GFP_TEMPORARY);
+   if (!fences)
+   return ERR_PTR(-ENOMEM);
+
+   for (n = 0; n < nfences; n++) {
+   struct drm_i915_gem_exec_fence fence;
+   struct drm_syncobj *syncobj;
+
+   if (__copy_from_user(, user++, sizeof(fence))) {
+   err = -EFAULT;
+   goto err;
+   }
+
+   syncobj = drm_syncobj_find(file, fence.handle);
+   if (!syncobj) {
+   DRM_DEBUG("Invalid syncobj handle provided\n");
+   err =