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

[Chris Wilson]

On Tue, Feb 07, 2017 at 12:55:21AM -0800, Oscar Mateo wrote:
> 
> 
> On 02/02/2017 11:33 PM, Chris Wilson wrote:
> >On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
> >>From: Michal Wajdeczko <michal.wajdeczko at 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 at intel.com>
> >>Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >>Signed-off-by: Oscar Mateo <oscar.mateo at 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);
> >This is silly. You are creating a pointer using kmalloc to copy into a
> >pointer created using alloc_page. Just write directly into the backing
> >store.
> 
> I guess I deserve this for not digging any deeper. From what I can
> see, the backing store is an array of 1024 context descriptors. If
> the whole context descriptor fell in one page, I could kmap_atomic
> only that. As it is, I would need to vmap a couple of pages to make
> sure I always get a complete pointer to guc_context_desc. Would you
> be happy with that?

One of the suggested usecases for i915_gem_object_pin_map() was this code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre