Re: [RFC 21/28] drm/i915/gtt: Reduce source verbosity by caching repeated dereferences

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

On 13/06/2019 15:12, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-13 14:35:32)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> There is a lot of code in i915_gem_gtt.c which repeatadly dereferences
>> either ggtt or ppgtt in order to get to the vm. Cache those accesses in
>> local variables for better readability.
> 
> There isn't a dereference though, it's just using the base struct. Meh.
> 
> I don't really mind, but I chose to write it the other way, specifically
> using vm to keep it short.
> 
> At the end of the day, the compiler *should* eliminate the redundant
> local, so it is merely a matter of which readers prefer. I think I still
> have a slight preference to using ppgtt throughout rather than mixing
> ppgtt and vm for the same object.

Agreed. I'll drop it. It was a silly ho-hum moment of misguided optimism 
how it will save some line wraps and then it didn't even do that.

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 254 +++++++++++++++-------------
>>   1 file changed, 134 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 516ffc4a521a..d09a4d9b71da 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -1004,10 +1004,11 @@ gen8_ppgtt_insert_pte_entries(struct i915_ppgtt *ppgtt,
>>   {
>>          struct i915_page_directory *pd;
>>          const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
>> +       struct i915_address_space *vm = &ppgtt->vm;
>>          gen8_pte_t *vaddr;
>>          bool ret;
>>   
>> -       GEM_BUG_ON(idx->pdpe >= i915_pdpes_per_pdp(&ppgtt->vm));
>> +       GEM_BUG_ON(idx->pdpe >= i915_pdpes_per_pdp(vm));
>>          pd = pdp->page_directory[idx->pdpe];
>>          vaddr = kmap_atomic_px(pd->page_table[idx->pde]);
>>          do {
>> @@ -1038,7 +1039,7 @@ gen8_ppgtt_insert_pte_entries(struct i915_ppgtt *ppgtt,
>>                                          break;
>>                                  }
>>   
>> -                               GEM_BUG_ON(idx->pdpe >= i915_pdpes_per_pdp(&ppgtt->vm));
>> +                               GEM_BUG_ON(idx->pdpe >= i915_pdpes_per_pdp(vm));
> 
> I don't see any code here. :-p

I don't get it, maybe for the better. :)

Regards,

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