Re: [RFC 25/28] drm/i915: Compartmentalize i915_gem_init_ggtt

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

On 13/06/2019 16:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-13 16:19:01)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Continuing on the theme of better logical organization of our code, make
>> the first step towards making the ggtt code better isolated from wider
>> struct drm_i915_private.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 40 +++++++++++++++++++----------
>>   1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 285a7a02c015..ea276ed9021a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2873,7 +2873,13 @@ static void ggtt_release_guc_top(struct i915_ggtt *ggtt)
>>                  drm_mm_remove_node(&ggtt->uc_fw);
>>   }
>>   
>> -int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>> +static void cleanup_init_ggtt(struct i915_ggtt *ggtt)
>> +{
>> +       ggtt_release_guc_top(ggtt);
>> +       drm_mm_remove_node(&ggtt->error_capture);
>> +}
>> +
>> +static int init_ggtt(struct i915_ggtt *ggtt)
>>   {
>>          /* Let GEM Manage all of the aperture.
>>           *
>> @@ -2884,7 +2890,6 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>           * aperture.  One page should be enough to keep any prefetching inside
>>           * of the aperture.
>>           */
>> -       struct i915_ggtt *ggtt = &dev_priv->ggtt;
>>          struct i915_address_space *vm = &ggtt->vm;
>>          unsigned long hole_start, hole_end;
>>          struct drm_mm_node *entry;
>> @@ -2897,7 +2902,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>           * why.
>>           */
>>          ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
>> -                              intel_wopcm_guc_size(&dev_priv->wopcm));
>> +                              intel_wopcm_guc_size(&vm->i915->wopcm));
>>   
>>          ret = intel_vgt_balloon(ggtt);
>>          if (ret)
>> @@ -2917,8 +2922,10 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>           * GGTT as it can comfortably hold GuC/HuC firmware images.
>>           */
>>          ret = ggtt_reserve_guc_top(ggtt);
>> -       if (ret)
>> -               goto err_reserve;
>> +       if (ret) {
>> +               cleanup_init_ggtt(ggtt);
> 
> Joonas would be turning in his grave, can you not hear the haunting call
> of onions?

Wasn't happy with how that under-developed onion looked, but I am also 
not happy without it. :/

>> +               return ret;
>> +       }
>>   
>>          /* Clear any non-preallocated blocks */
>>          drm_mm_for_each_hole(entry, &vm->mm, hole_start, hole_end) {
>> @@ -2930,19 +2937,24 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>          /* And finally clear the reserved guard page */
>>          vm->clear_range(vm, vm->total - PAGE_SIZE, PAGE_SIZE);
>>   
>> -       if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
>> -               ret = init_aliasing_ppgtt(dev_priv);
>> +       return 0;
>> +}
>> +
>> +int i915_gem_init_ggtt(struct drm_i915_private *i915)
> 
> Less with the i915_gem; this is not about gem/ i915->gem or the GEM API :)

Sometimes I try to minimize the diff, and sometimes I create a big one 
for no reason. Go figure. :)

i915_init_ggtt I suppose?

Regards,

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