[PATCH 0/3] (Final) tidying up of GuC code

[PATCH 0/3] (Final) tidying up of GuC code

[Dave Gordon]

Mostly renaming the GuC functions to use a more consistent naming
scheme, along with updating comments to clarify some of the code.

Dave Gordon (3):
  drm/i915: clarify PMINTRMSK/pm_intr_keep usage
  drm/i915/guc: general tidying up (loader)
  drm/i915/guc: general tidying up (submission)

 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_guc_reg.h        |  3 --
 drivers/gpu/drm/i915/i915_guc_submission.c | 63 +++++++++++++++---------------
 drivers/gpu/drm/i915/i915_irq.c            |  4 +-
 drivers/gpu/drm/i915/i915_reg.h            |  2 +-
 drivers/gpu/drm/i915/intel_guc.h           |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 54 +++++++++++++++++--------
 drivers/gpu/drm/i915/intel_lrc.c           |  2 +-
 8 files changed, 75 insertions(+), 56 deletions(-)

-- 
1.9.1

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

[PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage

[Dave Gordon]

No functional changes; just renaming a bit, tweaking a datatype,
prettifying layout, and adding comments, in particular in the
GuC setup code that touches this data.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_irq.c         |  4 ++--
 drivers/gpu/drm/i915/i915_reg.h         |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 27 +++++++++++++++++++++------
 4 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e2dda8..d01a50e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1184,6 +1184,7 @@ struct intel_gen6_power_mgmt {
 	bool interrupts_enabled;
 	u32 pm_iir;
 
+	/* PM interrupt bits that should never be masked */
 	u32 pm_intr_keep;
 
 	/* Frequencies are stored in potentially platform dependent multiples.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8462817..c128fdb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -371,7 +371,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->rps.interrupts_enabled = false;
 
-	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0));
+	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u));
 
 	__gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
 	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
@@ -4500,7 +4500,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8)
-		dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
+		dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_GUC;
 
 	INIT_DELAYED_WORK(&dev_priv->gpu_error.hangcheck_work,
 			  i915_hangcheck_elapsed);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a29d707..70d9616 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7067,7 +7067,7 @@ enum {
 #define VLV_RCEDATA				_MMIO(0xA0BC)
 #define GEN6_RC6pp_THRESHOLD			_MMIO(0xA0C0)
 #define GEN6_PMINTRMSK				_MMIO(0xA168)
-#define   GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
+#define   GEN8_PMINTR_REDIRECT_TO_GUC		  (1<<31)
 #define GEN8_MISC_CTRL0				_MMIO(0xA180)
 #define VLV_PWRDWNUPCTL				_MMIO(0xA294)
 #define GEN9_MEDIA_PG_IDLE_HYSTERESIS		_MMIO(0xA0C4)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 853928f..0021748 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -134,13 +134,28 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, ~irqs);
 
 	/*
-	 * If GuC has routed PM interrupts to itself, don't keep it.
-	 * and keep other interrupts those are unmasked by GuC.
-	*/
+	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
+	 * (unmasked) PM interrupts to the GuC. All other bits of this
+	 * register *disable* generation of a specific interrupt.
+	 *
+	 * 'pm_intr_keep' indicates bits that are NOT to be set when
+	 * writing to the PM interrupt mask register, i.e. interrupts
+	 * that must not be disabled.
+	 *
+	 * If the GuC is handling these interrupts, then we must not let
+	 * the PM code disable ANY interrupt that the GuC is expecting.
+	 * So for each ENABLED (0) bit in this register, we must SET the
+	 * bit in pm_intr_keep so that it's left enabled for the GuC.
+	 *
+	 * OTOH the REDIRECT_TO_GUC bit is initially SET in pm_intr_keep
+	 * (so interrupts go to the DISPLAY unit at first); but here we
+	 * need to CLEAR that bit, which will result in the register bit
+	 * being left SET!
+	 */
 	tmp = I915_READ(GEN6_PMINTRMSK);
-	if (tmp & GEN8_PMINTR_REDIRECT_TO_NON_DISP) {
-		dev_priv->rps.pm_intr_keep |= ~(tmp & ~GEN8_PMINTR_REDIRECT_TO_NON_DISP);
-		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_NON_DISP;
+	if (tmp & GEN8_PMINTR_REDIRECT_TO_GUC) {
+		dev_priv->rps.pm_intr_keep |= ~tmp;
+		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
 	}
 }
 
-- 
1.9.1

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

[PATCH 2/3] drm/i915/guc: general tidying up (loader)

[Dave Gordon]

Renaming to more consistent scheme, delete unused definitions

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_reg.h     |  3 ---
 drivers/gpu/drm/i915/intel_guc_loader.c | 27 ++++++++++++++++-----------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index cf5a65b..a47e1e4 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -103,9 +103,6 @@
 #define HOST2GUC_INTERRUPT		_MMIO(0xc4c8)
 #define   HOST2GUC_TRIGGER		  (1<<0)
 
-#define DRBMISC1			0x1984
-#define   DOORBELL_ENABLE		  (1<<0)
-
 #define GEN8_DRBREGL(x)			_MMIO(0x1000 + (x) * 8)
 #define   GEN8_DRB_VALID		  (1<<0)
 #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 0021748..6fd39ef 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
 	}
 };
 
-static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
+static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	int irqs;
@@ -114,7 +114,7 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
+static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	int irqs;
@@ -179,7 +179,12 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void set_guc_init_params(struct drm_i915_private *dev_priv)
+/*
+ * Initialise the GuC parameter block before starting the firmware
+ * transfer. These parameters are read by the firmware on startup
+ * and cannot be changed thereafter.
+ */
+static void guc_params_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 	u32 params[GUC_CTL_MAX_DWORDS];
@@ -392,11 +397,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
 					    I915_READ(GEN7_MISCCPCTL)));
 
-		/* allows for 5us before GT can go to RC6 */
+		/* allows for 5us (in 10ns units) before GT can go to RC6 */
 		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
 	}
 
-	set_guc_init_params(dev_priv);
+	guc_params_init(dev_priv);
 
 	ret = guc_ucode_xfer_dma(dev_priv, vma);
 
@@ -411,7 +416,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int i915_reset_guc(struct drm_i915_private *dev_priv)
+static int guc_hw_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 	u32 guc_status;
@@ -478,7 +483,7 @@ int intel_guc_setup(struct drm_device *dev)
 		goto fail;
 	}
 
-	direct_interrupts_to_host(dev_priv);
+	guc_interrupts_release(dev_priv);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
@@ -501,7 +506,7 @@ int intel_guc_setup(struct drm_device *dev)
 		 * Always reset the GuC just before (re)loading, so
 		 * that the state and timing are fairly predictable
 		 */
-		err = i915_reset_guc(dev_priv);
+		err = guc_hw_reset(dev_priv);
 		if (err)
 			goto fail;
 
@@ -526,7 +531,7 @@ int intel_guc_setup(struct drm_device *dev)
 		err = i915_guc_submission_enable(dev_priv);
 		if (err)
 			goto fail;
-		direct_interrupts_to_guc(dev_priv);
+		guc_interrupts_capture(dev_priv);
 	}
 
 	return 0;
@@ -535,7 +540,7 @@ int intel_guc_setup(struct drm_device *dev)
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
-	direct_interrupts_to_host(dev_priv);
+	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 
@@ -768,7 +773,7 @@ void intel_guc_fini(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
 	mutex_lock(&dev->struct_mutex);
-	direct_interrupts_to_host(dev_priv);
+	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 
-- 
1.9.1

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

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

[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 <david.s.gordon@intel.com>
---
 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
- * submit() when check() says there's no space, or calls submit() multiple
- * times with no intervening check().
+ * submit() when _reserve() says there's no space, or calls _submit()
+ * a different number of times from (successful) calls to _reserve().
  *
  * The only error here arises if the doorbell hardware isn't functioning
  * as expected, which really shouln't happen.
@@ -595,7 +595,7 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq)
 	int b_ret;
 
 	spin_lock(&client->wq_lock);
-	guc_add_workqueue_item(client, rq);
+	guc_wq_item_append(client, rq);
 	b_ret = guc_ring_doorbell(client);
 
 	client->submissions[engine_id] += 1;
@@ -686,7 +686,7 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size)
 	i915_vma_unpin_and_release(&client->vma);
 
 	if (client->ctx_index != GUC_INVALID_CTX_ID) {
-		guc_fini_ctx_desc(guc, client);
+		guc_ctx_desc_fini(guc, client);
 		ida_simple_remove(&guc->ctx_ids, client->ctx_index);
 	}
 
@@ -818,8 +818,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	else
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
-	guc_init_proc_desc(guc, client);
-	guc_init_ctx_desc(guc, client);
+	guc_proc_desc_init(guc, client);
+	guc_ctx_desc_init(guc, client);
 	if (guc_init_doorbell(guc, client, db_id))
 		goto err;
 
@@ -835,7 +835,7 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	return NULL;
 }
 
-static void guc_create_log(struct intel_guc *guc)
+static void guc_log_create(struct intel_guc *guc)
 {
 	struct i915_vma *vma;
 	unsigned long offset;
@@ -875,7 +875,7 @@ static void guc_create_log(struct intel_guc *guc)
 	guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 }
 
-static void init_guc_policies(struct guc_policies *policies)
+static void guc_policies_init(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
 	u32 p, i;
@@ -897,7 +897,7 @@ static void init_guc_policies(struct guc_policies *policies)
 	policies->is_valid = 1;
 }
 
-static void guc_create_ads(struct intel_guc *guc)
+static void guc_addon_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct i915_vma *vma;
@@ -940,7 +940,7 @@ static void guc_create_ads(struct intel_guc *guc)
 
 	/* GuC scheduling policies */
 	policies = (void *)ads + sizeof(struct guc_ads);
-	init_guc_policies(policies);
+	guc_policies_init(policies);
 
 	ads->scheduler_policies =
 		i915_ggtt_offset(vma) + sizeof(struct guc_ads);
@@ -971,9 +971,11 @@ static void guc_create_ads(struct intel_guc *guc)
  */
 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);
 	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;
 	}
-- 
1.9.1

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

✓ Fi.CI.BAT: success for (Final) tidying up of GuC code

[Patchwork]

== Series Details ==

Series: (Final) tidying up of GuC code
URL   : https://patchwork.freedesktop.org/series/12357/
State : success

== Summary ==

Series 12357v1 (Final) tidying up of GuC code
https://patchwork.freedesktop.org/api/1.0/series/12357/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                fail       -> PASS       (fi-bsw-n3050)

fi-bdw-5557u     total:254  pass:239  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:254  pass:208  dwarn:0   dfail:0   fail:0   skip:46 
fi-byt-n2820     total:254  pass:212  dwarn:0   dfail:0   fail:1   skip:41 
fi-hsw-4770k     total:254  pass:232  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:254  pass:228  dwarn:0   dfail:0   fail:0   skip:26 
fi-ilk-650       total:254  pass:184  dwarn:0   dfail:0   fail:2   skip:68 
fi-ivb-3520m     total:254  pass:223  dwarn:0   dfail:0   fail:0   skip:31 
fi-ivb-3770      total:254  pass:223  dwarn:0   dfail:0   fail:0   skip:31 
fi-skl-6260u     total:254  pass:240  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:254  pass:227  dwarn:0   dfail:0   fail:1   skip:26 
fi-skl-6700k     total:254  pass:225  dwarn:1   dfail:0   fail:0   skip:28 
fi-snb-2520m     total:254  pass:209  dwarn:0   dfail:0   fail:0   skip:45 
fi-snb-2600      total:254  pass:209  dwarn:0   dfail:0   fail:0   skip:45 

Results at /archive/results/CI_IGT_test/Patchwork_2515/

93d9d9dd030c6eb360b500bf9872663b30b5ebe5 drm-intel-nightly: 2016y-09m-12d-14h-34m-11s UTC integration manifest
9f221491 drm/i915/guc: general tidying up (submission)
6e8d886 drm/i915/guc: general tidying up (loader)
26c088f drm/i915: clarify PMINTRMSK/pm_intr_keep usage

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

Re: [PATCH 1/3] drm/i915: clarify PMINTRMSK/pm_intr_keep usage

[Tvrtko Ursulin]

On 12/09/2016 21:19, Dave Gordon wrote:
> No functional changes; just renaming a bit, tweaking a datatype,
> prettifying layout, and adding comments, in particular in the
> GuC setup code that touches this data.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  1 +
>   drivers/gpu/drm/i915/i915_irq.c         |  4 ++--
>   drivers/gpu/drm/i915/i915_reg.h         |  2 +-
>   drivers/gpu/drm/i915/intel_guc_loader.c | 27 +++++++++++++++++++++------
>   4 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e2dda8..d01a50e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1184,6 +1184,7 @@ struct intel_gen6_power_mgmt {
>   	bool interrupts_enabled;
>   	u32 pm_iir;
>   
> +	/* PM interrupt bits that should never be masked */
>   	u32 pm_intr_keep;
>   
>   	/* Frequencies are stored in potentially platform dependent multiples.
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8462817..c128fdb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -371,7 +371,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	spin_lock_irq(&dev_priv->irq_lock);
>   	dev_priv->rps.interrupts_enabled = false;
>   
> -	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0));
> +	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u));
>   
>   	__gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>   	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
> @@ -4500,7 +4500,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>   		dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED;
>   
>   	if (INTEL_INFO(dev_priv)->gen >= 8)
> -		dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> +		dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_GUC;
>   
>   	INIT_DELAYED_WORK(&dev_priv->gpu_error.hangcheck_work,
>   			  i915_hangcheck_elapsed);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a29d707..70d9616 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7067,7 +7067,7 @@ enum {
>   #define VLV_RCEDATA				_MMIO(0xA0BC)
>   #define GEN6_RC6pp_THRESHOLD			_MMIO(0xA0C0)
>   #define GEN6_PMINTRMSK				_MMIO(0xA168)
> -#define   GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
> +#define   GEN8_PMINTR_REDIRECT_TO_GUC		  (1<<31)
>   #define GEN8_MISC_CTRL0				_MMIO(0xA180)
>   #define VLV_PWRDWNUPCTL				_MMIO(0xA294)
>   #define GEN9_MEDIA_PG_IDLE_HYSTERESIS		_MMIO(0xA0C4)
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 853928f..0021748 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -134,13 +134,28 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
>   	I915_WRITE(GUC_WD_VECS_IER, ~irqs);
>   
>   	/*
> -	 * If GuC has routed PM interrupts to itself, don't keep it.
> -	 * and keep other interrupts those are unmasked by GuC.
> -	*/
> +	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
> +	 * (unmasked) PM interrupts to the GuC. All other bits of this
> +	 * register *disable* generation of a specific interrupt.
> +	 *
> +	 * 'pm_intr_keep' indicates bits that are NOT to be set when
> +	 * writing to the PM interrupt mask register, i.e. interrupts
> +	 * that must not be disabled.
> +	 *
> +	 * If the GuC is handling these interrupts, then we must not let
> +	 * the PM code disable ANY interrupt that the GuC is expecting.
> +	 * So for each ENABLED (0) bit in this register, we must SET the
> +	 * bit in pm_intr_keep so that it's left enabled for the GuC.
> +	 *
> +	 * OTOH the REDIRECT_TO_GUC bit is initially SET in pm_intr_keep
> +	 * (so interrupts go to the DISPLAY unit at first); but here we
> +	 * need to CLEAR that bit, which will result in the register bit
> +	 * being left SET!
> +	 */
>   	tmp = I915_READ(GEN6_PMINTRMSK);
> -	if (tmp & GEN8_PMINTR_REDIRECT_TO_NON_DISP) {
> -		dev_priv->rps.pm_intr_keep |= ~(tmp & ~GEN8_PMINTR_REDIRECT_TO_NON_DISP);
> -		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> +	if (tmp & GEN8_PMINTR_REDIRECT_TO_GUC) {
> +		dev_priv->rps.pm_intr_keep |= ~tmp;
> +		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
>   	}
>   }
>   

More comments is always good and the cleanup just above obviously good 
as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH 2/3] drm/i915/guc: general tidying up (loader)

[Tvrtko Ursulin]

On 12/09/2016 21:19, Dave Gordon wrote:
> Renaming to more consistent scheme, delete unused definitions
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_reg.h     |  3 ---
>   drivers/gpu/drm/i915/intel_guc_loader.c | 27 ++++++++++++++++-----------
>   2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index cf5a65b..a47e1e4 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -103,9 +103,6 @@
>   #define HOST2GUC_INTERRUPT		_MMIO(0xc4c8)
>   #define   HOST2GUC_TRIGGER		  (1<<0)
>   
> -#define DRBMISC1			0x1984
> -#define   DOORBELL_ENABLE		  (1<<0)
> -
>   #define GEN8_DRBREGL(x)			_MMIO(0x1000 + (x) * 8)
>   #define   GEN8_DRB_VALID		  (1<<0)
>   #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 0021748..6fd39ef 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
>   	}
>   };
>   
> -static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
> +static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_engine_cs *engine;
>   	int irqs;
> @@ -114,7 +114,7 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
>   	I915_WRITE(GUC_WD_VECS_IER, 0);
>   }
>   
> -static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
> +static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_engine_cs *engine;
>   	int irqs;
> @@ -179,7 +179,12 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
>   	}
>   }
>   
> -static void set_guc_init_params(struct drm_i915_private *dev_priv)
> +/*
> + * Initialise the GuC parameter block before starting the firmware
> + * transfer. These parameters are read by the firmware on startup
> + * and cannot be changed thereafter.
> + */
> +static void guc_params_init(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
>   	u32 params[GUC_CTL_MAX_DWORDS];
> @@ -392,11 +397,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>   		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
>   					    I915_READ(GEN7_MISCCPCTL)));
>   
> -		/* allows for 5us before GT can go to RC6 */
> +		/* allows for 5us (in 10ns units) before GT can go to RC6 */
>   		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
>   	}
>   
> -	set_guc_init_params(dev_priv);
> +	guc_params_init(dev_priv);
>   
>   	ret = guc_ucode_xfer_dma(dev_priv, vma);
>   
> @@ -411,7 +416,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>   	return ret;
>   }
>   
> -static int i915_reset_guc(struct drm_i915_private *dev_priv)
> +static int guc_hw_reset(struct drm_i915_private *dev_priv)
>   {
>   	int ret;
>   	u32 guc_status;
> @@ -478,7 +483,7 @@ int intel_guc_setup(struct drm_device *dev)
>   		goto fail;
>   	}
>   
> -	direct_interrupts_to_host(dev_priv);
> +	guc_interrupts_release(dev_priv);
>   
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>   
> @@ -501,7 +506,7 @@ int intel_guc_setup(struct drm_device *dev)
>   		 * Always reset the GuC just before (re)loading, so
>   		 * that the state and timing are fairly predictable
>   		 */
> -		err = i915_reset_guc(dev_priv);
> +		err = guc_hw_reset(dev_priv);
>   		if (err)
>   			goto fail;
>   
> @@ -526,7 +531,7 @@ int intel_guc_setup(struct drm_device *dev)
>   		err = i915_guc_submission_enable(dev_priv);
>   		if (err)
>   			goto fail;
> -		direct_interrupts_to_guc(dev_priv);
> +		guc_interrupts_capture(dev_priv);
>   	}
>   
>   	return 0;
> @@ -535,7 +540,7 @@ int intel_guc_setup(struct drm_device *dev)
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>   
> -	direct_interrupts_to_host(dev_priv);
> +	guc_interrupts_release(dev_priv);
>   	i915_guc_submission_disable(dev_priv);
>   	i915_guc_submission_fini(dev_priv);
>   
> @@ -768,7 +773,7 @@ void intel_guc_fini(struct drm_device *dev)
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>   
>   	mutex_lock(&dev->struct_mutex);
> -	direct_interrupts_to_host(dev_priv);
> +	guc_interrupts_release(dev_priv);
>   	i915_guc_submission_disable(dev_priv);
>   	i915_guc_submission_fini(dev_priv);
>   

I liked the direct interrupts naming more, but it is not that critical. 
The rest is OK.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

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

[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 <david.s.gordon@intel.com>
> ---
>   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
> - * submit() when check() says there's no space, or calls submit() multiple
> - * times with no intervening check().
> + * submit() when _reserve() says there's no space, or calls _submit()
> + * a different number of times from (successful) calls to _reserve().
>    *
>    * The only error here arises if the doorbell hardware isn't functioning
>    * as expected, which really shouln't happen.
> @@ -595,7 +595,7 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq)
>   	int b_ret;
>   
>   	spin_lock(&client->wq_lock);
> -	guc_add_workqueue_item(client, rq);
> +	guc_wq_item_append(client, rq);
>   	b_ret = guc_ring_doorbell(client);
>   
>   	client->submissions[engine_id] += 1;
> @@ -686,7 +686,7 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size)
>   	i915_vma_unpin_and_release(&client->vma);
>   
>   	if (client->ctx_index != GUC_INVALID_CTX_ID) {
> -		guc_fini_ctx_desc(guc, client);
> +		guc_ctx_desc_fini(guc, client);
>   		ida_simple_remove(&guc->ctx_ids, client->ctx_index);
>   	}
>   
> @@ -818,8 +818,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
>   	else
>   		client->proc_desc_offset = (GUC_DB_SIZE / 2);
>   
> -	guc_init_proc_desc(guc, client);
> -	guc_init_ctx_desc(guc, client);
> +	guc_proc_desc_init(guc, client);
> +	guc_ctx_desc_init(guc, client);
>   	if (guc_init_doorbell(guc, client, db_id))
>   		goto err;
>   
> @@ -835,7 +835,7 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
>   	return NULL;
>   }
>   
> -static void guc_create_log(struct intel_guc *guc)
> +static void guc_log_create(struct intel_guc *guc)
>   {
>   	struct i915_vma *vma;
>   	unsigned long offset;
> @@ -875,7 +875,7 @@ static void guc_create_log(struct intel_guc *guc)
>   	guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
>   }
>   
> -static void init_guc_policies(struct guc_policies *policies)
> +static void guc_policies_init(struct guc_policies *policies)
>   {
>   	struct guc_policy *policy;
>   	u32 p, i;
> @@ -897,7 +897,7 @@ static void init_guc_policies(struct guc_policies *policies)
>   	policies->is_valid = 1;
>   }
>   
> -static void guc_create_ads(struct intel_guc *guc)
> +static void guc_addon_create(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct i915_vma *vma;
> @@ -940,7 +940,7 @@ static void guc_create_ads(struct intel_guc *guc)
>   
>   	/* GuC scheduling policies */
>   	policies = (void *)ads + sizeof(struct guc_ads);
> -	init_guc_policies(policies);
> +	guc_policies_init(policies);
>   
>   	ads->scheduler_policies =
>   		i915_ggtt_offset(vma) + sizeof(struct guc_ads);
> @@ -971,9 +971,11 @@ static void guc_create_ads(struct intel_guc *guc)
>    */
>   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.
>   	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: [PATCH 3/3] drm/i915/guc: general tidying up (submission)

[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 <david.s.gordon@intel.com>
>> ---
>>   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: [PATCH 3/3] drm/i915/guc: general tidying up (submission)

[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 <david.s.gordon@intel.com>
>>> ---
>>>   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 <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

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

[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 <david.s.gordon@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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