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

Tvrtko Ursulin
2019-06-13T15:45:03Z

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