Re: [PATCH 12/26] drm/i915: Page table helpers, and define renames

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Mar 18, 2014 at 11:29:58AM -0700, Jesse Barnes wrote:
> On Tue, 18 Mar 2014 09:05:58 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Mon, Mar 17, 2014 at 10:48:44PM -0700, Ben Widawsky wrote:
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > > @@ -1,8 +1,11 @@
> > >  #ifndef _I915_GEM_GTT_H
> > >  #define _I915_GEM_GTT_H
> > >  
> > > -#define GEN6_PPGTT_PD_ENTRIES 512
> > > -#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> > > +/* GEN Agnostic defines */
> > > +#define I915_PDES_PER_PD		512
> > > +#define I915_PTE_MASK			(PAGE_SHIFT-1)
> > 
> > That looks decidely fishy.
> > 
> > PAGE_SHIFT is 12 -> PTE_MASK = 0xb
> > 

Thanks for catching this. I'll presume the define isn't even used.

> > > +#define I915_PDE_MASK			(I915_PDES_PER_PD-1)
> > > +
> > >  typedef uint32_t gen6_gtt_pte_t;
> > >  typedef uint64_t gen8_gtt_pte_t;
> > >  typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
> > > @@ -23,6 +26,98 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
> > >  #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
> > >  #define HSW_PTE_ADDR_ENCODE(addr)	HSW_GTT_ADDR_ENCODE(addr)
> > >  
> > > +
> > > +/* GEN6 PPGTT resembles a 2 level page table:
> > > + * 31:22 | 21:12 |  11:0
> > > + *  PDE  |  PTE  | offset
> > > + */
> > > +#define GEN6_PDE_SHIFT			22
> > > +#define GEN6_PTES_PER_PT		(PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> > > +
> > > +static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift)
> > > +{
> > > +	const uint32_t mask = (1 << (pde_shift - PAGE_SHIFT)) - 1;
> > > +	return (address >> PAGE_SHIFT) & mask;
> > > +}
> > > +
> > > +/* Helper to counts the number of PTEs within the given length. This count does
> > > + * not cross a page table boundary, so the max value would be
> > > + * GEN6_PTES_PER_PT for GEN6, and GEN8_PTES_PER_PT for GEN8.
> > > + */
> > > +static inline size_t i915_pte_count(uint64_t addr, size_t length,
> > > +				    uint32_t pde_shift)
> > > +{
> > > +	const uint64_t pd_mask = ~((1 << pde_shift) - 1);
> > > +	uint64_t end;
> > > +
> > > +	if (WARN_ON(!length))
> > > +		return 0;
> > > +
> > > +	if (WARN_ON(addr % PAGE_SIZE))
> > > +		addr = round_down(addr, PAGE_SIZE);
> > > +
> > > +	if (WARN_ON(length % PAGE_SIZE))
> > > +		length = round_up(length, PAGE_SIZE);
> > 
> > Oh oh. I think these fixups are very suspect, so just
> > BUG_ON(length == 0);
> > BUG_ON(offset_in_page(addr|length));
> > 

I thought someone might have an issue with the BUG_ON. But I prefer it
as well.

> > > +
> > > +	end = addr + length;
> > > +
> > > +	if ((addr & pd_mask) != (end & pd_mask))
> > > +		return (1 << (pde_shift - PAGE_SHIFT)) -
> > 
> > #define NUM_PTE(pde_shift) (1 << (pde_shift - PAGE_SHIFT))
> > here and for computing the pd_mask.
> > 
> > > +			i915_pte_index(addr, pde_shift);
> > > +
> > > +	return i915_pte_index(end, pde_shift) - i915_pte_index(addr, pde_shift);
> > > +}
> > 
> > Otherwise the helpers look a useful improvement in readability.
> > -Chris
> > 
> 
> Can we use GTT_PAGE_SIZE here too?  I'm worried the kernel PAGE_SIZE
> will change at some point and blow us up.  At least in places where
> we're doing our own thing rather than using the x86 bits...

That's fine with me. We have quite a few other places in our code which
depend on PAGE_SIZE being 4k though.

It's likely I'll be maintaining this branch myself for a while, but I'll
modify these both locally.

> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center