[Intel-gfx] [PATCH 3/3] drm/i915/guc: general tidying up (submission)

2016-09-12 Thread Dave Gordon 
Renaming to more consistent scheme, and updating comments, mostly
about i915_guc_wq_reserve(), aka i915_guc_wq_check_space().

Signed-off-by: Dave Gordon 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++---
 drivers/gpu/drm/i915/intel_guc.h   |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c   |  2 +-
 3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 279a4d0..43358e1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -59,7 +59,7 @@
  * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which
  * represents in-order queue. The kernel driver packs ring tail pointer and an
  * ELSP context descriptor dword into Work Item.
- * See guc_add_workqueue_item()
+ * See guc_wq_item_append()
  *
  */
 
@@ -288,7 +288,7 @@ static uint32_t select_doorbell_cacheline(struct intel_guc 
*guc)
 /*
  * Initialise the process descriptor shared with the GuC firmware.
  */
-static void guc_init_proc_desc(struct intel_guc *guc,
+static void guc_proc_desc_init(struct intel_guc *guc,
   struct i915_guc_client *client)
 {
struct guc_process_desc *desc;
@@ -320,7 +320,7 @@ static void guc_init_proc_desc(struct intel_guc *guc,
  * write queue, etc).
  */
 
-static void guc_init_ctx_desc(struct intel_guc *guc,
+static void guc_ctx_desc_init(struct intel_guc *guc,
  struct i915_guc_client *client)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -399,7 +399,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 sizeof(desc) * client->ctx_index);
 }
 
-static void guc_fini_ctx_desc(struct intel_guc *guc,
+static void guc_ctx_desc_fini(struct intel_guc *guc,
  struct i915_guc_client *client)
 {
struct guc_context_desc desc;
@@ -413,7 +413,7 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 }
 
 /**
- * i915_guc_wq_check_space() - check that the GuC can accept a request
+ * i915_guc_wq_reserve() - reserve space in the GuC's workqueue
  * @request:   request associated with the commands
  *
  * Return: 0 if space is available
@@ -421,14 +421,14 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
  *
  * This function must be called (and must return 0) before a request
  * is submitted to the GuC via i915_guc_submit() below. Once a result
- * of 0 has been returned, it remains valid until (but only until)
- * the next call to submit().
+ * of 0 has been returned, it must be balanced by a corresponding
+ * call to submit().
  *
- * This precheck allows the caller to determine in advance that space
+ * Reservation allows the caller to determine in advance that space
  * will be available for the next submission before committing resources
  * to it, and helps avoid late failures with complicated recovery paths.
  */
-int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
+int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
 {
const size_t wqi_size = sizeof(struct guc_wq_item);
struct i915_guc_client *gc = request->i915->guc.execbuf_client;
@@ -451,8 +451,9 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request 
*request)
return ret;
 }
 
-static void guc_add_workqueue_item(struct i915_guc_client *gc,
-  struct drm_i915_gem_request *rq)
+/* Construct a Work Item and append it to the GuC's Work Queue */
+static void guc_wq_item_append(struct i915_guc_client *gc,
+  struct drm_i915_gem_request *rq)
 {
/* wqi_len is in DWords, and does not include the one-word header */
const size_t wqi_size = sizeof(struct guc_wq_item);
@@ -465,7 +466,7 @@ static void guc_add_workqueue_item(struct i915_guc_client 
*gc,
 
desc = gc->client_base + gc->proc_desc_offset;
 
-   /* Free space is guaranteed, see i915_guc_wq_check_space() above */
+   /* Free space is guaranteed, see i915_guc_wq_reserve() above */
freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
GEM_BUG_ON(freespace < wqi_size);
 
@@ -575,14 +576,13 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
  * Return: 0 on success, otherwise an errno.
  * (Note: nonzero really shouldn't happen!)
  *
- * The caller must have already called i915_guc_wq_check_space() above
- * with a result of 0 (success) since the last request submission. This
- * guarantees that there is space in the work queue for the new request,
- * so enqueuing the item cannot fail.
+ * The caller must have already called i915_guc_wq_reserve() above with
+ * a result of 0 (success), guaranteeing that there is space in the work
+ * queue for the new request, so enqueuing the item cannot fail.
  *
  * Bad Things Will Happen if the caller violates this protocol e.g. calls
-

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc: general tidying up (submission)

2016-09-14 Thread Tvrtko Ursulin 


On 12/09/2016 21:19, Dave Gordon wrote:

Renaming to more consistent scheme, and updating comments, mostly
about i915_guc_wq_reserve(), aka i915_guc_wq_check_space().

Signed-off-by: Dave Gordon 
---
  drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_lrc.c   |  2 +-
  3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 279a4d0..43358e1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -59,7 +59,7 @@
   * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which
   * represents in-order queue. The kernel driver packs ring tail pointer and an
   * ELSP context descriptor dword into Work Item.
- * See guc_add_workqueue_item()
+ * See guc_wq_item_append()
   *
   */
  
@@ -288,7 +288,7 @@ static uint32_t select_doorbell_cacheline(struct intel_guc *guc)

  /*
   * Initialise the process descriptor shared with the GuC firmware.
   */
-static void guc_init_proc_desc(struct intel_guc *guc,
+static void guc_proc_desc_init(struct intel_guc *guc,
   struct i915_guc_client *client)
  {
struct guc_process_desc *desc;
@@ -320,7 +320,7 @@ static void guc_init_proc_desc(struct intel_guc *guc,
   * write queue, etc).
   */
  
-static void guc_init_ctx_desc(struct intel_guc *guc,

+static void guc_ctx_desc_init(struct intel_guc *guc,
  struct i915_guc_client *client)
  {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -399,7 +399,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 sizeof(desc) * client->ctx_index);
  }
  
-static void guc_fini_ctx_desc(struct intel_guc *guc,

+static void guc_ctx_desc_fini(struct intel_guc *guc,
  struct i915_guc_client *client)
  {
struct guc_context_desc desc;
@@ -413,7 +413,7 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
  }
  
  /**

- * i915_guc_wq_check_space() - check that the GuC can accept a request
+ * i915_guc_wq_reserve() - reserve space in the GuC's workqueue
   * @request:  request associated with the commands
   *
   * Return:0 if space is available
@@ -421,14 +421,14 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
   *
   * This function must be called (and must return 0) before a request
   * is submitted to the GuC via i915_guc_submit() below. Once a result
- * of 0 has been returned, it remains valid until (but only until)
- * the next call to submit().
+ * of 0 has been returned, it must be balanced by a corresponding
+ * call to submit().
   *
- * This precheck allows the caller to determine in advance that space
+ * Reservation allows the caller to determine in advance that space
   * will be available for the next submission before committing resources
   * to it, and helps avoid late failures with complicated recovery paths.
   */
-int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
+int i915_guc_wq_reserve(struct drm_i915_gem_request *request)
  {
const size_t wqi_size = sizeof(struct guc_wq_item);
struct i915_guc_client *gc = request->i915->guc.execbuf_client;
@@ -451,8 +451,9 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request 
*request)
return ret;
  }
  
-static void guc_add_workqueue_item(struct i915_guc_client *gc,

-  struct drm_i915_gem_request *rq)
+/* Construct a Work Item and append it to the GuC's Work Queue */
+static void guc_wq_item_append(struct i915_guc_client *gc,
+  struct drm_i915_gem_request *rq)
  {
/* wqi_len is in DWords, and does not include the one-word header */
const size_t wqi_size = sizeof(struct guc_wq_item);
@@ -465,7 +466,7 @@ static void guc_add_workqueue_item(struct i915_guc_client 
*gc,
  
  	desc = gc->client_base + gc->proc_desc_offset;
  
-	/* Free space is guaranteed, see i915_guc_wq_check_space() above */

+   /* Free space is guaranteed, see i915_guc_wq_reserve() above */
freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
GEM_BUG_ON(freespace < wqi_size);
  
@@ -575,14 +576,13 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)

   * Return:0 on success, otherwise an errno.
   *(Note: nonzero really shouldn't happen!)
   *
- * The caller must have already called i915_guc_wq_check_space() above
- * with a result of 0 (success) since the last request submission. This
- * guarantees that there is space in the work queue for the new request,
- * so enqueuing the item cannot fail.
+ * The caller must have already called i915_guc_wq_reserve() above with
+ * a result of 0 (success), guaranteeing that there is space in the work
+ * queue for the new request, so enqueuing the item cannot fail.

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc: general tidying up (submission)

2016-09-14 Thread Dave Gordon 

On 14/09/16 16:22, Tvrtko Ursulin wrote:


On 12/09/2016 21:19, Dave Gordon wrote:

Renaming to more consistent scheme, and updating comments, mostly
about i915_guc_wq_reserve(), aka i915_guc_wq_check_space().

Signed-off-by: Dave Gordon 
---
  drivers/gpu/drm/i915/i915_guc_submission.c | 63
+++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_lrc.c   |  2 +-
  3 files changed, 34 insertions(+), 33 deletions(-)



[snip]


  int i915_guc_submission_init(struct drm_i915_private *dev_priv)
  {
+const size_t ctxsize = sizeof(struct guc_context_desc);
+const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
+const size_t gemsize = round_up(poolsize, PAGE_SIZE);
  struct intel_guc *guc = &dev_priv->guc;
  struct i915_vma *vma;
-u32 size;
/* Wipe bitmap & delete client in case of reinitialisation */
  bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
@@ -985,15 +987,14 @@ int i915_guc_submission_init(struct
drm_i915_private *dev_priv)
  if (guc->ctx_pool_vma)
  return 0; /* already allocated */
  -size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct
guc_context_desc));
-vma = guc_allocate_vma(guc, size);
+vma = guc_allocate_vma(guc, gemsize);


PAGE_ALIGN lost - lower layers do that for us? I don't have easy access
to the tree at the moment to check and I kind of can't remember right now.


PAGE_ALIGN here is replaced by using round_up(..., PAGE_SIZE) at the 
point where the constant is defined a few lines above. I think 
round_up() is clearer, because "align" could equally well mean round 
down. Anyway "align" (up or down) is something you do to addresses or 
offsets, not sizes.


.Dave.


  if (IS_ERR(vma))
  return PTR_ERR(vma);
guc->ctx_pool_vma = vma;
  ida_init(&guc->ctx_ids);
-guc_create_log(guc);
-guc_create_ads(guc);
+guc_log_create(guc);
+guc_addon_create(guc);
return 0;
  }
diff --git a/drivers/gpu/drm/i915/intel_guc.h
b/drivers/gpu/drm/i915/intel_guc.h
index 4678459..b1ba869 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -159,7 +159,7 @@ extern int intel_guc_resume(struct drm_device *dev);
  /* i915_guc_submission.c */
  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_wq_reserve(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 16d7cdd..25114336 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -627,7 +627,7 @@ int intel_logical_ring_alloc_request_extras(struct
drm_i915_gem_request *request
   * going any further, as the i915_add_request() call
   * later on mustn't fail ...
   */
-ret = i915_guc_wq_check_space(request);
+ret = i915_guc_wq_reserve(request);
  if (ret)
  return ret;
  }


Name changes make sense. Just the PAGE_ALIGN question above.

Regards,

Tvrtko


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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc: general tidying up (submission)

2016-09-15 Thread Tvrtko Ursulin 


On 14/09/2016 18:00, Dave Gordon wrote:

On 14/09/16 16:22, Tvrtko Ursulin wrote:


On 12/09/2016 21:19, Dave Gordon wrote:

Renaming to more consistent scheme, and updating comments, mostly
about i915_guc_wq_reserve(), aka i915_guc_wq_check_space().

Signed-off-by: Dave Gordon 
---
  drivers/gpu/drm/i915/i915_guc_submission.c | 63
+++---
  drivers/gpu/drm/i915/intel_guc.h   |  2 +-
  drivers/gpu/drm/i915/intel_lrc.c   |  2 +-
  3 files changed, 34 insertions(+), 33 deletions(-)



[snip]


  int i915_guc_submission_init(struct drm_i915_private *dev_priv)
  {
+const size_t ctxsize = sizeof(struct guc_context_desc);
+const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
+const size_t gemsize = round_up(poolsize, PAGE_SIZE);
  struct intel_guc *guc = &dev_priv->guc;
  struct i915_vma *vma;
-u32 size;
/* Wipe bitmap & delete client in case of reinitialisation */
  bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
@@ -985,15 +987,14 @@ int i915_guc_submission_init(struct
drm_i915_private *dev_priv)
  if (guc->ctx_pool_vma)
  return 0; /* already allocated */
  -size = PAGE_ALIGN(GUC_MAX_GPU_CONTEXTS*sizeof(struct
guc_context_desc));
-vma = guc_allocate_vma(guc, size);
+vma = guc_allocate_vma(guc, gemsize);


PAGE_ALIGN lost - lower layers do that for us? I don't have easy access
to the tree at the moment to check and I kind of can't remember right 
now.


PAGE_ALIGN here is replaced by using round_up(..., PAGE_SIZE) at the 
point where the constant is defined a few lines above. I think 
round_up() is clearer, because "align" could equally well mean round 
down. Anyway "align" (up or down) is something you do to addresses or 
offsets, not sizes.


I failed to spot that line. :( Should really take a long holiday. :)

Reviewed-by: Tvrtko Ursulin 

Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc: general tidying up (submission)

2016-09-15 Thread Chris Wilson 
On Thu, Sep 15, 2016 at 09:57:18AM +0100, Tvrtko Ursulin wrote:
> On 14/09/2016 18:00, Dave Gordon wrote:
> >On 14/09/16 16:22, Tvrtko Ursulin wrote:
> >>
> >>On 12/09/2016 21:19, Dave Gordon wrote:
> >>>Renaming to more consistent scheme, and updating comments, mostly
> >>>about i915_guc_wq_reserve(), aka i915_guc_wq_check_space().
> >>>
> >>>Signed-off-by: Dave Gordon 
> Reviewed-by: Tvrtko Ursulin 

And pushed.
-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