[Intel-gfx] [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 at 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