Re: [Intel-gfx] [PATCH 14/73] drm/i915: Unify request submission

[Joonas Lahtinen]

On ma, 2016-08-01 at 10:10 +0100, Chris Wilson wrote:
> Move request submission from emit_request into its own common vfunc
> from i915_add_request().
> 
> v2: Convert I915_DISPATCH_flags to BIT(x) whilst passing
> v3: Rename a few functions to match.
> v4: Reenable execlists submission after disabling guc.
> 
> Signed-off-by: Chris Wilson 
> Link: 
> http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-23-git-send-email-ch...@chris-wilson.co.uk
> Reviewed-by: Joonas Lahtinen  #v3

I'll keep mine, Dave might want to give a look too (CC'd).

Reviewed-by: Joonas Lahtinen 

> ---
>  drivers/gpu/drm/i915/i915_gem_request.c|  8 +++-
>  drivers/gpu/drm/i915/i915_guc_submission.c | 12 +---
>  drivers/gpu/drm/i915/intel_guc.h   |  1 -
>  drivers/gpu/drm/i915/intel_lrc.c   | 26 +++---
>  drivers/gpu/drm/i915/intel_lrc.h   |  2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c| 23 +--
>  drivers/gpu/drm/i915/intel_ringbuffer.h| 27 +--
>  7 files changed, 51 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c 
> b/drivers/gpu/drm/i915/i915_gem_request.c
> index a885905df3bb..e378eb61979b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -466,12 +466,9 @@ void __i915_add_request(struct drm_i915_gem_request 
> *request,
>    */
>   request->postfix = ring->tail;
>  
> - if (i915.enable_execlists)
> - ret = engine->emit_request(request);
> - else
> - ret = engine->add_request(request);
>   /* Not allowed to fail! */
> - WARN(ret, "emit|add_request failed: %d!\n", ret);
> + ret = engine->emit_request(request);
> + WARN(ret, "(%s)->emit_request failed: %d!\n", engine->name, ret);
>  
>   /* Sanity check that the reserved size was large enough. */
>   ret = ring->tail - request_start;
> @@ -483,6 +480,7 @@ void __i915_add_request(struct drm_i915_gem_request 
> *request,
>     reserved_tail, ret);
>  
>   i915_gem_mark_busy(engine);
> + engine->submit_request(request);
>  }
>  
>  static unsigned long local_clock_us(unsigned int *cpu)
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index eccd34832fe6..23bbd0ff86c7 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -585,7 +585,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
>   * The only error here arises if the doorbell hardware isn't functioning
>   * as expected, which really shouln't happen.
>   */
> -int i915_guc_submit(struct drm_i915_gem_request *rq)
> +static void i915_guc_submit(struct drm_i915_gem_request *rq)
>  {
>   unsigned int engine_id = rq->engine->id;
>   struct intel_guc *guc = >i915->guc;
> @@ -602,8 +602,6 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
>  
>   guc->submissions[engine_id] += 1;
>   guc->last_seqno[engine_id] = rq->fence.seqno;
> -
> - return b_ret;
>  }
>  
>  /*
> @@ -992,6 +990,7 @@ int i915_guc_submission_enable(struct drm_i915_private 
> *dev_priv)
>  {
>   struct intel_guc *guc = _priv->guc;
>   struct i915_guc_client *client;
> + struct intel_engine_cs *engine;
>  
>   /* client for execbuf submission */
>   client = guc_client_alloc(dev_priv,
> @@ -1006,6 +1005,10 @@ int i915_guc_submission_enable(struct drm_i915_private 
> *dev_priv)
>   host2guc_sample_forcewake(guc, client);
>   guc_init_doorbell_hw(guc);
>  
> + /* Take over from manual control of ELSP (execlists) */
> + for_each_engine(engine, dev_priv)
> + engine->submit_request = i915_guc_submit;
> +
>   return 0;
>  }
>  
> @@ -1015,6 +1018,9 @@ void i915_guc_submission_disable(struct 
> drm_i915_private *dev_priv)
>  
>   guc_client_free(dev_priv, guc->execbuf_client);
>   guc->execbuf_client = NULL;
> +
> + /* Revert back to manual ELSP submission */
> + intel_execlists_enable_submission(dev_priv);
>  }
>  
>  void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
> b/drivers/gpu/drm/i915/intel_guc.h
> index 3e3e743740c0..623cf26cd784 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -160,7 +160,6 @@ extern int intel_guc_resume(struct drm_device *dev);
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>  int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
>  int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
> -int i915_guc_submit(struct drm_i915_gem_request *rq);
>  void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>  void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>  
> diff 

[Intel-gfx] [PATCH 14/73] drm/i915: Unify request submission

[Chris Wilson]

Move request submission from emit_request into its own common vfunc
from i915_add_request().

v2: Convert I915_DISPATCH_flags to BIT(x) whilst passing
v3: Rename a few functions to match.
v4: Reenable execlists submission after disabling guc.

Signed-off-by: Chris Wilson 
Link: 
http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-23-git-send-email-ch...@chris-wilson.co.uk
Reviewed-by: Joonas Lahtinen  #v3
---
 drivers/gpu/drm/i915/i915_gem_request.c|  8 +++-
 drivers/gpu/drm/i915/i915_guc_submission.c | 12 +---
 drivers/gpu/drm/i915/intel_guc.h   |  1 -
 drivers/gpu/drm/i915/intel_lrc.c   | 26 +++---
 drivers/gpu/drm/i915/intel_lrc.h   |  2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c| 23 +--
 drivers/gpu/drm/i915/intel_ringbuffer.h| 27 +--
 7 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c 
b/drivers/gpu/drm/i915/i915_gem_request.c
index a885905df3bb..e378eb61979b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -466,12 +466,9 @@ void __i915_add_request(struct drm_i915_gem_request 
*request,
 */
request->postfix = ring->tail;
 
-   if (i915.enable_execlists)
-   ret = engine->emit_request(request);
-   else
-   ret = engine->add_request(request);
/* Not allowed to fail! */
-   WARN(ret, "emit|add_request failed: %d!\n", ret);
+   ret = engine->emit_request(request);
+   WARN(ret, "(%s)->emit_request failed: %d!\n", engine->name, ret);
 
/* Sanity check that the reserved size was large enough. */
ret = ring->tail - request_start;
@@ -483,6 +480,7 @@ void __i915_add_request(struct drm_i915_gem_request 
*request,
  reserved_tail, ret);
 
i915_gem_mark_busy(engine);
+   engine->submit_request(request);
 }
 
 static unsigned long local_clock_us(unsigned int *cpu)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index eccd34832fe6..23bbd0ff86c7 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -585,7 +585,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
  * The only error here arises if the doorbell hardware isn't functioning
  * as expected, which really shouln't happen.
  */
-int i915_guc_submit(struct drm_i915_gem_request *rq)
+static void i915_guc_submit(struct drm_i915_gem_request *rq)
 {
unsigned int engine_id = rq->engine->id;
struct intel_guc *guc = >i915->guc;
@@ -602,8 +602,6 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
 
guc->submissions[engine_id] += 1;
guc->last_seqno[engine_id] = rq->fence.seqno;
-
-   return b_ret;
 }
 
 /*
@@ -992,6 +990,7 @@ int i915_guc_submission_enable(struct drm_i915_private 
*dev_priv)
 {
struct intel_guc *guc = _priv->guc;
struct i915_guc_client *client;
+   struct intel_engine_cs *engine;
 
/* client for execbuf submission */
client = guc_client_alloc(dev_priv,
@@ -1006,6 +1005,10 @@ int i915_guc_submission_enable(struct drm_i915_private 
*dev_priv)
host2guc_sample_forcewake(guc, client);
guc_init_doorbell_hw(guc);
 
+   /* Take over from manual control of ELSP (execlists) */
+   for_each_engine(engine, dev_priv)
+   engine->submit_request = i915_guc_submit;
+
return 0;
 }
 
@@ -1015,6 +1018,9 @@ void i915_guc_submission_disable(struct drm_i915_private 
*dev_priv)
 
guc_client_free(dev_priv, guc->execbuf_client);
guc->execbuf_client = NULL;
+
+   /* Revert back to manual ELSP submission */
+   intel_execlists_enable_submission(dev_priv);
 }
 
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3e3e743740c0..623cf26cd784 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -160,7 +160,6 @@ extern int intel_guc_resume(struct drm_device *dev);
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
 int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
-int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 824f7efe4e64..8a72ee86e825 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -738,7 +738,7 @@ err_unpin:
 }
 
 /*
- * intel_logical_ring_advance_and_submit() - advance the tail and submit the 
workload
+ *