[PATCH] drm/i915/guc: Dynamically alloc GuC descriptor

* [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
@ 2017-02-02 15:27 Oscar Mateo
  2017-02-02 23:52 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-02 15:27 UTC (permalink / raw)
  To: intel-gfx; +Cc: Deepak S

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

The GuC descriptor is big in size. If we use local definition of
guc_desc we have a chance to overflow stack. Use allocated one.

v2: Rebased
v3: Split
v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)

Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
 1 file changed, 57 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8ced9e2..b4f14f3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
 	struct sg_table *sg = guc->ctx_pool_vma->pages;
 	void *doorbell_bitmap = guc->doorbell_bitmap;
 	struct guc_doorbell_info *doorbell;
-	struct guc_context_desc desc;
+	struct guc_context_desc *desc;
 	size_t len;
 
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
 	doorbell = client->vaddr + client->doorbell_offset;
 
 	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
@@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
 	}
 
 	/* Update the GuC's idea of the doorbell ID */
-	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
-	if (len != sizeof(desc))
+	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+				 sizeof(*desc) * client->ctx_index);
+	if (len != sizeof(*desc)) {
+		kfree(desc);
 		return -EFAULT;
-	desc.db_id = new_id;
-	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
-	if (len != sizeof(desc))
+	}
+
+	desc->db_id = new_id;
+	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+				   sizeof(*desc) * client->ctx_index);
+	if (len != sizeof(*desc)) {
+		kfree(desc);
 		return -EFAULT;
+	}
+
+	kfree(desc);
 
 	client->doorbell_id = new_id;
 	if (new_id == GUC_INVALID_DOORBELL_ID)
@@ -229,30 +240,33 @@ static void guc_proc_desc_init(struct intel_guc *guc,
  * This descriptor tells the GuC where (in GGTT space) to find the important
  * data structures relating to this client (doorbell, process descriptor,
  * write queue, etc).
+ *
+ * Returns: 0 on success, negative error code on failure.
  */
-
-static void guc_ctx_desc_init(struct intel_guc *guc,
+static int guc_ctx_desc_init(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx = client->owner;
-	struct guc_context_desc desc;
+	struct guc_context_desc *desc;
 	struct sg_table *sg;
 	unsigned int tmp;
 	u32 gfx_addr;
 
-	memset(&desc, 0, sizeof(desc));
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
 
-	desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
-	desc.context_id = client->ctx_index;
-	desc.priority = client->priority;
-	desc.db_id = client->doorbell_id;
+	desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+	desc->context_id = client->ctx_index;
+	desc->priority = client->priority;
+	desc->db_id = client->doorbell_id;
 
 	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
 		struct intel_context *ce = &ctx->engine[engine->id];
 		uint32_t guc_engine_id = engine->guc_id;
-		struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+		struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
 
 		/* TODO: We have a design issue to be solved here. Only when we
 		 * receive the first batch, we know which engine is used by the
@@ -277,50 +291,56 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 		lrc->ring_next_free_location = lrc->ring_begin;
 		lrc->ring_current_tail_pointer_value = 0;
 
-		desc.engines_used |= (1 << guc_engine_id);
+		desc->engines_used |= (1 << guc_engine_id);
 	}
 
 	DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
-			client->engines, desc.engines_used);
-	WARN_ON(desc.engines_used == 0);
+			client->engines, desc->engines_used);
+	WARN_ON(desc->engines_used == 0);
 
 	/*
 	 * The doorbell, process descriptor, and workqueue are all parts
 	 * of the client object, which the GuC will reference via the GGTT
 	 */
 	gfx_addr = guc_ggtt_offset(client->vma);
-	desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+	desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
 				client->doorbell_offset;
-	desc.db_trigger_cpu =
-		(uintptr_t)client->vaddr + client->doorbell_offset;
-	desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
-	desc.process_desc = gfx_addr + client->proc_desc_offset;
-	desc.wq_addr = gfx_addr + client->wq_offset;
-	desc.wq_size = client->wq_size;
+	desc->db_trigger_cpu = (uintptr_t)client->vaddr +
+				client->doorbell_offset;
+	desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+	desc->process_desc = gfx_addr + client->proc_desc_offset;
+	desc->wq_addr = gfx_addr + client->wq_offset;
+	desc->wq_size = client->wq_size;
 
 	/*
 	 * XXX: Take LRCs from an existing context if this is not an
 	 * IsKMDCreatedContext client
 	 */
-	desc.desc_private = (uintptr_t)client;
+	desc->desc_private = (uintptr_t)client;
 
 	/* Pool context is pinned already */
 	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	sg_pcopy_from_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+			     sizeof(*desc) * client->ctx_index);
+
+	kfree(desc);
+	return 0;
 }
 
 static void guc_ctx_desc_fini(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
-	struct guc_context_desc desc;
+	struct guc_context_desc *desc;
 	struct sg_table *sg;
 
-	memset(&desc, 0, sizeof(desc));
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return;
 
 	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	sg_pcopy_from_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+			     sizeof(*desc) * client->ctx_index);
+	kfree(desc);
 }
 
 /**
@@ -751,7 +771,9 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
 	guc_proc_desc_init(guc, client);
-	guc_ctx_desc_init(guc, client);
+
+	if (guc_ctx_desc_init(guc, client) < 0)
+		goto err;
 
 	/* For runtime client allocation we need to enable the doorbell. Not
 	 * required yet for the static execbuf_client as this special kernel
@@ -1040,5 +1062,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
-
-
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread