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

[Oscar Mateo <oscar.mateo@intel.com>]

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@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);
> 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.
> -Chris
>

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?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx