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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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>

> +	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