Re: [PATCH 05/11] drm/i915/gtt: Compute the radix for gen8 page table levels

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/11] drm/i915/gtt: Compute the radix for gen8 page table levels
Date: Wed, 10 Jul 2019 17:55:52 +0300	[thread overview]
Message-ID: <87a7dmq6pz.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <87pnmiq9ta.fsf@gaia.fi.intel.com>

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> The radix levels of each page directory are easily determined so replace
>> the numerous hardcoded constants with precomputed derived constants.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 39 +++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 2fc60e8acd9a..271305705c1c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -868,6 +868,45 @@ static int gen8_ppgtt_notify_vgt(struct i915_ppgtt *ppgtt, bool create)
>>  	return 0;
>>  }
>>  
>> +/* Index shifts into the pagetable are offset by GEN8_PTE_SHIFT [12] */
>> +#define gen8_pd_shift(lvl) ((lvl) * ilog2(I915_PDES))
>
> Could be just (lvl) * 9. But looking at ilog2() it will be
> so both are fine.
>
>> +#define gen8_pd_index(i, lvl) i915_pde_index((i), gen8_pd_shift(lvl))
>> +#define __gen8_pte_shift(lvl) (GEN8_PTE_SHIFT + gen8_pd_shift(lvl))
>> +#define __gen8_pte_index(a, lvl) i915_pde_index((a), __gen8_pte_shift(lvl))
>> +
>> +static inline unsigned int
>> +gen8_pd_range(u64 addr, u64 end, int lvl, unsigned int *idx)
>
> I was enough confused (even tho the last function reveals
> it clearly) that in irc we concluded that addr as a
> first parameter is misleading and converged on 'start'
>
>> +{
>> +	const int shift = gen8_pd_shift(lvl);
>> +	const u64 mask = ~0ull << gen8_pd_shift(lvl + 1);
>> +
>> +	GEM_BUG_ON(addr >= end);
>> +	end += ~mask >> gen8_pd_shift(1);
>> +
>> +	*idx = i915_pde_index(addr, shift);
>> +	if ((addr ^ end) & mask)
>> +		return I915_PDES - *idx;
>> +	else
>> +		return i915_pde_index(end, shift) - *idx;
>> +}
>> +
>> +static inline bool gen8_pd_subsumes(u64 addr, u64 end, int lvl)
>> +{
>
> Just a suggestion gen8_pd_contains() for emphasis on exclusivity.
> But well, this is fine too. I guess what reads better in callsite,
> (which we dont see yet!)
>
>> +	const u64 mask = ~0ull << gen8_pd_shift(lvl + 1);
>> +
>> +	GEM_BUG_ON(addr >= end);
>> +	return (addr ^ end) & mask && (addr & ~mask) == 0;
>> +}
>> +
>> +static inline unsigned int gen8_pt_count(u64 addr, u64 end)
>> +{
>> +	GEM_BUG_ON(addr >= end);
>> +	if ((addr ^ end) & ~I915_PDE_MASK)
>> +		return I915_PDES - (addr & I915_PDE_MASK);
>
> Ok, I yield on 512. I915_PDES is fine as it atleast
> couples it to mask :O
>
> With s/addr/start,
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@mika.kuoppala@linux.intel.com>

not long till vacation, hanging there but it starts to show...

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>
>> +	else
>> +		return end - addr;
>> +}
>> +
>>  static void gen8_free_page_tables(struct i915_address_space *vm,
>>  				  struct i915_page_directory *pd)
>>  {
>> -- 
>> 2.20.1
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx