Quoting Mika Kuoppala (2019-07-09 13:24:27)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>
> > We only use the dma pages for scratch, and so do not need to allocate
> > the extra storage for the shadow page directory.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 192 ++++++++++++----------------
> > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +-
> > 2 files changed, 85 insertions(+), 113 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 236c964dd761..937236913e70 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -594,25 +594,17 @@ static void cleanup_page_dma(struct i915_address_space *vm,
> >
> > #define kmap_atomic_px(px) kmap_atomic(px_base(px)->page)
> >
> > -#define fill_px(vm, px, v) fill_page_dma((vm), px_base(px), (v))
> > -#define fill32_px(vm, px, v) fill_page_dma_32((vm), px_base(px), (v))
> > +#define fill_px(px, v) fill_page_dma(px_base(px), (v))
> > +#define fill32_px(px, v) fill_page_dma_32(px_base(px), (v))
> >
> > -static void fill_page_dma(struct i915_address_space *vm,
> > - struct i915_page_dma *p,
> > - const u64 val)
> > +static void fill_page_dma(struct i915_page_dma *p, const u64 val)
> > {
> > - u64 * const vaddr = kmap_atomic(p->page);
> > -
> > - memset64(vaddr, val, PAGE_SIZE / sizeof(val));
> > -
> > - kunmap_atomic(vaddr);
> > + kunmap_atomic(memset64(kmap_atomic(p->page), val, I915_PDES));
>
> Neat.
>
> I would go for 512 instead of I915_PDES. There is no magic
> and there never will be magic as it is as const as carved into stone.
I was just going with I915_PDES and I915_PDE_MASK throughout. Later this
one becomes count, fwiw.
>
> > }
> >
> > -static void fill_page_dma_32(struct i915_address_space *vm,
> > - struct i915_page_dma *p,
> > - const u32 v)
> > +static void fill_page_dma_32(struct i915_page_dma *p, const u32 v)
> > {
> > - fill_page_dma(vm, p, (u64)v << 32 | v);
> > + fill_page_dma(p, (u64)v << 32 | v);
> > }
> >
> > static int
> > @@ -687,6 +679,21 @@ static void cleanup_scratch_page(struct i915_address_space *vm)
> > __free_pages(p->page, order);
> > }
> >
> > +static void free_scratch(struct i915_address_space *vm)
> > +{
> > + if (!vm->scratch_page.daddr) /* set to 0 on clones */
> > + return;
> > +
> > + if (vm->scratch_pdp.daddr)
> > + cleanup_page_dma(vm, &vm->scratch_pdp);
> > + if (vm->scratch_pd.daddr)
> > + cleanup_page_dma(vm, &vm->scratch_pd);
> > + if (vm->scratch_pt.daddr)
> > + cleanup_page_dma(vm, &vm->scratch_pt);
> > +
> > + cleanup_scratch_page(vm);
> > +}
> > +
> > static struct i915_page_table *alloc_pt(struct i915_address_space *vm)
> > {
> > struct i915_page_table *pt;
> > @@ -711,18 +718,6 @@ static void free_pt(struct i915_address_space *vm, struct i915_page_table *pt)
> > kfree(pt);
> > }
> >
> > -static void gen8_initialize_pt(struct i915_address_space *vm,
> > - struct i915_page_table *pt)
> > -{
> > - fill_px(vm, pt, vm->scratch_pte);
> > -}
> > -
> > -static void gen6_initialize_pt(struct i915_address_space *vm,
> > - struct i915_page_table *pt)
> > -{
> > - fill32_px(vm, pt, vm->scratch_pte);
> > -}
> > -
> > static struct i915_page_directory *__alloc_pd(void)
> > {
> > struct i915_page_directory *pd;
> > @@ -765,9 +760,11 @@ static void free_pd(struct i915_address_space *vm,
> > kfree(pd);
> > }
> >
> > -#define init_pd(vm, pd, to) { \
> > - fill_px((vm), (pd), gen8_pde_encode(px_dma(to), I915_CACHE_LLC)); \
> > - memset_p((pd)->entry, (to), 512); \
> > +static void init_pd(struct i915_page_directory *pd,
> > + struct i915_page_dma *scratch)
> > +{
> > + fill_px(pd, gen8_pde_encode(scratch->daddr, I915_CACHE_LLC));
> > + memset_p(pd->entry, scratch, 512);
> > }
> >
> > static inline void
> > @@ -869,12 +866,11 @@ static void gen8_ppgtt_clear_pd(struct i915_address_space *vm,
> > u32 pde;
> >
> > gen8_for_each_pde(pt, pd, start, length, pde) {
> > - GEM_BUG_ON(pt == vm->scratch_pt);
> > + GEM_BUG_ON(px_base(pt) == &vm->scratch_pt);
> >
> > atomic_inc(&pt->used);
> > gen8_ppgtt_clear_pt(vm, pt, start, length);
> > - if (release_pd_entry(pd, pde, &pt->used,
> > - px_base(vm->scratch_pt)))
> > + if (release_pd_entry(pd, pde, &pt->used, &vm->scratch_pt))
> > free_pt(vm, pt);
> > }
> > }
> > @@ -890,12 +886,11 @@ static void gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
> > unsigned int pdpe;
> >
> > gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
> > - GEM_BUG_ON(pd == vm->scratch_pd);
> > + GEM_BUG_ON(px_base(pd) == &vm->scratch_pd);
>
> Perhaps future will bring pd_points_scratch(pd).
>
> Now the intriguing, bordering irritating, question in my mind is
> that can we fold the scratch_pd and scratch_pdp to be the same thing.
No, we can fold the scratch_pd to be the same (dma wise) as they do need
to end up at the scratch_pte. And sadly we can't use the scratch_pte as
the filler for scratch_pd.
> Patch lgtm with some dislike towards I915_PDES,
I'm not keen on it tbh. But the mix of alternating between 512/0x1ff
does suggest to use some name.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
