Re: [PATCH 20/21] drm/i915: Make GuC GGTT reservation work on ggtt

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/06/2019 12:58, Michal Wajdeczko wrote:
> On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin 
> <tvrtko.ursulin@linux.intel.com> wrote:
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> These functions operate on ggtt so make them take that directly as
>> parameter.
> 
> Not quite.
> 
> Function intel_guc_reserved_gtt_size() operates on struct intel_guc
> and is defined in intel_guc.c for proper layering, so NAK here.

But it doesn't really. It operates on intel_wopcm if we want to be true.

u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
{
	return ggtt->vm.i915->wopcm.guc.size;
}

And GuC portion it needs is just one part of struct intel_wopcm - so 
whether or not this function must live in intel_guc.c and so how hard we 
should see this as layering violation is not so clear in my opinion.

Unless it will need to actually use intel_guc in the future.

> For other two intel_guc_reserve|release_ggtt_top() I would rather:
> - rename them to i915_ggtt_reserve|release_guc_top(),
> - move to i915_gem_ggtt.c
> - make them static

Fair, will do.

Regards,

Tvrtko

> Michal
> 
>>
>> At the same time move the USES_GUC conditional down to
>> intel_guc_reserve_ggtt_top for symmetry with
>> intel_guc_reserved_gtt_size.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>>  3 files changed, 17 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index d3b3676d10f3..d967a4e9ceb0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private 
>> *dev_priv)
>>       * why.
>>       */
>>      ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
>> -                   intel_guc_reserved_gtt_size(&dev_priv->guc));
>> +                   intel_guc_reserved_gtt_size(ggtt));
>>     ret = intel_vgt_balloon(ggtt);
>>      if (ret)
>> @@ -2926,11 +2926,9 @@ int i915_gem_init_ggtt(struct drm_i915_private 
>> *dev_priv)
>>      if (ret)
>>          return ret;
>> -    if (USES_GUC(dev_priv)) {
>> -        ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
>> -        if (ret)
>> -            goto err_reserve;
>> -    }
>> +    ret = intel_guc_reserve_ggtt_top(ggtt);
>> +    if (ret)
>> +        goto err_reserve;
>>     /* Clear any non-preallocated blocks */
>>      drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
>> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private 
>> *dev_priv)
>>      return 0;
>> err_appgtt:
>> -    intel_guc_release_ggtt_top(&dev_priv->guc);
>> +    intel_guc_release_ggtt_top(ggtt);
>>  err_reserve:
>>      drm_mm_remove_node(&ggtt->error_capture);
>>      return ret;
>> @@ -2979,7 +2977,7 @@ void i915_ggtt_cleanup_hw(struct 
>> drm_i915_private *dev_priv)
>>      if (drm_mm_node_allocated(&ggtt->error_capture))
>>          drm_mm_remove_node(&ggtt->error_capture);
>> -    intel_guc_release_ggtt_top(&dev_priv->guc);
>> +    intel_guc_release_ggtt_top(ggtt);
>>     if (drm_mm_initialized(&ggtt->vm.mm)) {
>>          intel_vgt_deballoon(ggtt);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index b88c349c4fa6..633248b7da25 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct 
>> intel_guc *guc, u32 size)
>> /**
>>   * intel_guc_reserved_gtt_size()
>> - * @guc:    intel_guc structure
>> + * @ggtt:    Pointer to struct i915_ggtt
>>   *
>>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we 
>> are using
>>   * GuC we can't have any objects pinned in that region. This function 
>> returns
>> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct 
>> intel_guc *guc, u32 size)
>>   * 0 if GuC is not present or not in use.
>>   * Otherwise, the GuC WOPCM size.
>>   */
>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>>  {
>> -    return guc_to_i915(guc)->wopcm.guc.size;
>> +    return ggtt->vm.i915->wopcm.guc.size;
>>  }
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>>  {
>> -    struct drm_i915_private *i915 = guc_to_i915(guc);
>> -    struct i915_ggtt *ggtt = &i915->ggtt;
>>      u64 size;
>>      int ret;
>> +    if (!USES_GUC(ggtt->vm.i915))
>> +        return 0;
>> +
>>      size = ggtt->vm.total - GUC_GGTT_TOP;
>>     ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
>> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc 
>> *guc)
>>      return ret;
>>  }
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>>  {
>> -    struct drm_i915_private *i915 = guc_to_i915(guc);
>> -    struct i915_ggtt *ggtt = &i915->ggtt;
>> -
>>      if (drm_mm_node_allocated(&ggtt->uc_fw))
>>          drm_mm_remove_node(&ggtt->uc_fw);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index cbfed7a77c8b..55ea14176c5e 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -173,9 +173,9 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 
>> rsa_offset);
>>  int intel_guc_suspend(struct intel_guc *guc);
>>  int intel_guc_resume(struct intel_guc *guc);
>>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
>> size);
>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
>> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>>  {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx