| Message ID | 20190707210024.26192-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] drm/i915/gtt: Use shallow dma pages for scratch | expand |
Quoting Chris Wilson (2019-07-07 22:00:18) > 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)) > +#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) > +{ > + 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) > +{ > + 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); > + else > + return end - addr; > +} So this is the question, do you want these as 512 and 0x1ff? Or just define gen8_pd_shift(lvl) as ((lvl) * ilog(512)) and work from there. Hmm. -Chris
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)) > +#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) > +{ > + const int shift = gen8_pd_shift(lvl); > + const u64 mask = ~0ull << gen8_pd_shift(lvl + 1); > + > + GEM_BUG_ON(addr >= end); GEM_BUG_ON(!lvl) ? > + end += ~mask >> gen8_pd_shift(1); > + > + *idx = i915_pde_index(addr, shift); As I see no usage of this macro yet, this looks wrong as the shift doesn't include the pte shift? For example for address for first page, we could get index of 7. -Mika > + 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) > +{ > + 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); > + else > + return end - addr; > +} > + > static void gen8_free_page_tables(struct i915_address_space *vm, > struct i915_page_directory *pd) > { > -- > 2.20.1
Quoting Mika Kuoppala (2019-07-10 10:24:48) > 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)) > > +#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) > > +{ > > + const int shift = gen8_pd_shift(lvl); > > + const u64 mask = ~0ull << gen8_pd_shift(lvl + 1); > > + > > + GEM_BUG_ON(addr >= end); > > GEM_BUG_ON(!lvl) ? It worked for !lvl so I left it out. > > + end += ~mask >> gen8_pd_shift(1); > > + > > + *idx = i915_pde_index(addr, shift); > > As I see no usage of this macro yet, this looks > wrong as the shift doesn't include the pte shift? Why would it since I'm not working on page addresses but pd indices? :-p -Chris
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
Quoting Mika Kuoppala (2019-07-10 14:49:05) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > +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 I had already removed them all! :-p Give or take a few GEN8_PDES since that ends up being the single constant we require. -Chris
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
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)) +#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) +{ + 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) +{ + 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); + else + return end - addr; +} + static void gen8_free_page_tables(struct i915_address_space *vm, struct i915_page_directory *pd) {
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(+)