Re: [Intel-gfx] [PATCH] RFC drm/i915: Allow userspace to specify ringsize on construction

2019-10-21 Thread Janusz Krzysztofik 
Hi Chris,

On Thursday, October 10, 2019 4:23:16 PM CEST Chris Wilson wrote:
> No good reason why we must always use a static ringsize, so let
> userspace select one during construction.

I've heard from UMD people they like this solution :-)

> Signed-off-by: Chris Wilson 
> Cc: Joonas Lahtinen 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c | 83 +++--
>  include/uapi/drm/i915_drm.h | 12 +++
>  2 files changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 46e5b3b53288..9635e377c8ae 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -455,23 +455,30 @@ __create_context(struct drm_i915_private *i915)
>   return ERR_PTR(err);
>  }
>  
> -static void
> +static int
>  context_apply_all(struct i915_gem_context *ctx,
> -   void (*fn)(struct intel_context *ce, void *data),
> +   int (*fn)(struct intel_context *ce, void *data),
> void *data)
>  {
>   struct i915_gem_engines_iter it;
>   struct intel_context *ce;
> + int err = 0;
>  
> - for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it)
> - fn(ce, data);
> + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> + err = fn(ce, data);
> + if (err)
> + break;
> + }
>   i915_gem_context_unlock_engines(ctx);
> +
> + return err;
>  }
>  
> -static void __apply_ppgtt(struct intel_context *ce, void *vm)
> +static int __apply_ppgtt(struct intel_context *ce, void *vm)
>  {
>   i915_vm_put(ce->vm);
>   ce->vm = i915_vm_get(vm);
> + return 0;
>  }
>  
>  static struct i915_address_space *
> @@ -509,9 +516,10 @@ static void __set_timeline(struct intel_timeline **dst,
>   intel_timeline_put(old);
>  }
>  
> -static void __apply_timeline(struct intel_context *ce, void *timeline)
> +static int __apply_timeline(struct intel_context *ce, void *timeline)
>  {
>   __set_timeline(&ce->timeline, timeline);
> + return 0;
>  }
>  
>  static void __assign_timeline(struct i915_gem_context *ctx,
> @@ -1086,6 +1094,65 @@ static int set_ppgtt(struct drm_i915_file_private 
> *file_priv,
>   return err;
>  }
>  
> +static int __apply_ringsize(struct intel_context *ce, void *sz)
> +{
> + int err = 0;
> +
> + if (intel_context_lock_pinned(ce))
> + return -EINTR;
> +
> + if (intel_context_is_pinned(ce)) {
> + err = -EBUSY; /* In active use! Come back later! */
> + goto unlock;
> + }
> +
> + if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
> + struct intel_ring *ring;
> +
> + /* Replace the existing ringbuffer */
> + ring = intel_engine_create_ring(ce->engine,
> + (unsigned long)sz);
> + if (IS_ERR(ring)) {
> + err = PTR_ERR(ring);
> + goto unlock;
> + }
> +
> + intel_ring_put(ce->ring);
> + ce->ring = ring;
> +
> + /* Context image will be updated on next pin */
> + } else {
> + ce->ring = sz;
> + }
> +
> +unlock:
> + intel_context_unlock_pinned(ce);
> + return err;
> +}
> +
> +static int set_ringsize(struct i915_gem_context *ctx,
> + struct drm_i915_gem_context_param *args)
> +{
> + if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
> + return -ENODEV;
> +
> + if (args->size)
> + return -EINVAL;
> +
> + if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
> + return -EINVAL;
> +
> + if (args->value < I915_GTT_PAGE_SIZE)
> + return -EINVAL;
> +
> + if (args->value > 128 * I915_GTT_PAGE_SIZE)
> + return -EINVAL;
> +
> + return context_apply_all(ctx,
> +  __apply_ringsize,
> +  __intel_context_ring_size(args->value));
> +}
> +
>  static int gen8_emit_rpcs_config(struct i915_request *rq,
>struct intel_context *ce,
>struct intel_sseu sseu)
> @@ -1798,6 +1865,10 @@ static int ctx_setparam(struct drm_i915_file_private 
> *fpriv,
>   ret = set_persistence(ctx, args);
>   break;
>  
> + case I915_CONTEXT_PARAM_RINGSIZE:
> + ret = set_ringsize(ctx, args);
> + break;
> +
>   case I915_CONTEXT_PARAM_BAN_PERIOD:
>   default:
>   ret = -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index eb9e704d717a..e375cd2cf66b 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1580,6 +1580,18 @@ struct drm_i915_gem_context_param {
>   * By default, new contexts allow persistence.
>   */
>  #define I915_CONTEXT_PARAM_PERS

[Intel-gfx] [PATCH] RFC drm/i915: Allow userspace to specify ringsize on construction

2019-10-10 Thread Chris Wilson 
No good reason why we must always use a static ringsize, so let
userspace select one during construction.

Signed-off-by: Chris Wilson 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 83 +++--
 include/uapi/drm/i915_drm.h | 12 +++
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 46e5b3b53288..9635e377c8ae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -455,23 +455,30 @@ __create_context(struct drm_i915_private *i915)
return ERR_PTR(err);
 }
 
-static void
+static int
 context_apply_all(struct i915_gem_context *ctx,
- void (*fn)(struct intel_context *ce, void *data),
+ int (*fn)(struct intel_context *ce, void *data),
  void *data)
 {
struct i915_gem_engines_iter it;
struct intel_context *ce;
+   int err = 0;
 
-   for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it)
-   fn(ce, data);
+   for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
+   err = fn(ce, data);
+   if (err)
+   break;
+   }
i915_gem_context_unlock_engines(ctx);
+
+   return err;
 }
 
-static void __apply_ppgtt(struct intel_context *ce, void *vm)
+static int __apply_ppgtt(struct intel_context *ce, void *vm)
 {
i915_vm_put(ce->vm);
ce->vm = i915_vm_get(vm);
+   return 0;
 }
 
 static struct i915_address_space *
@@ -509,9 +516,10 @@ static void __set_timeline(struct intel_timeline **dst,
intel_timeline_put(old);
 }
 
-static void __apply_timeline(struct intel_context *ce, void *timeline)
+static int __apply_timeline(struct intel_context *ce, void *timeline)
 {
__set_timeline(&ce->timeline, timeline);
+   return 0;
 }
 
 static void __assign_timeline(struct i915_gem_context *ctx,
@@ -1086,6 +1094,65 @@ static int set_ppgtt(struct drm_i915_file_private 
*file_priv,
return err;
 }
 
+static int __apply_ringsize(struct intel_context *ce, void *sz)
+{
+   int err = 0;
+
+   if (intel_context_lock_pinned(ce))
+   return -EINTR;
+
+   if (intel_context_is_pinned(ce)) {
+   err = -EBUSY; /* In active use! Come back later! */
+   goto unlock;
+   }
+
+   if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
+   struct intel_ring *ring;
+
+   /* Replace the existing ringbuffer */
+   ring = intel_engine_create_ring(ce->engine,
+   (unsigned long)sz);
+   if (IS_ERR(ring)) {
+   err = PTR_ERR(ring);
+   goto unlock;
+   }
+
+   intel_ring_put(ce->ring);
+   ce->ring = ring;
+
+   /* Context image will be updated on next pin */
+   } else {
+   ce->ring = sz;
+   }
+
+unlock:
+   intel_context_unlock_pinned(ce);
+   return err;
+}
+
+static int set_ringsize(struct i915_gem_context *ctx,
+   struct drm_i915_gem_context_param *args)
+{
+   if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
+   return -ENODEV;
+
+   if (args->size)
+   return -EINVAL;
+
+   if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
+   return -EINVAL;
+
+   if (args->value < I915_GTT_PAGE_SIZE)
+   return -EINVAL;
+
+   if (args->value > 128 * I915_GTT_PAGE_SIZE)
+   return -EINVAL;
+
+   return context_apply_all(ctx,
+__apply_ringsize,
+__intel_context_ring_size(args->value));
+}
+
 static int gen8_emit_rpcs_config(struct i915_request *rq,
 struct intel_context *ce,
 struct intel_sseu sseu)
@@ -1798,6 +1865,10 @@ static int ctx_setparam(struct drm_i915_file_private 
*fpriv,
ret = set_persistence(ctx, args);
break;
 
+   case I915_CONTEXT_PARAM_RINGSIZE:
+   ret = set_ringsize(ctx, args);
+   break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index eb9e704d717a..e375cd2cf66b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1580,6 +1580,18 @@ struct drm_i915_gem_context_param {
  * By default, new contexts allow persistence.
  */
 #define I915_CONTEXT_PARAM_PERSISTENCE 0xb
+
+/*
+ *
+ * I915_CONTEXT_PARAM_RINGSIZE:
+ *
+ * Sets the size of the ringbuffer to use for logical ring contexts.
+ * Only possible to be set prior to first use, i.e. during construction.
+ * Only applies to the current set of engine and lost for those engines
+ * are repla