| Message ID | 1418922621-25818-4-git-send-email-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 18, 2014 at 05:10:00PM +0000, Michel Thierry wrote: > From: Ben Widawsky <benjamin.widawsky@intel.com> > > In gen8, 32b PPGTT has always had one "pdp" (it doesn't actually have > one, but it resembles having one). The #define was confusing as is, and > using "PDPE" is a much better description. > > sed -i 's/GEN8_LEGACY_PDPS/GEN8_LEGACY_PDPES/' drivers/gpu/drm/i915/*.[ch] Hm generally I've thought the abbreviations are pdp (for the page itself) and pde (for the entries within). I still have no idea what pdpe means ... So either please explain that or pick one of the others. -Daniel > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++--- > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 75a29a3..9639310 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -375,7 +375,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm, > pt_vaddr = NULL; > > for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) { > - if (WARN_ON(pdpe >= GEN8_LEGACY_PDPS)) > + if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES)) > break; > > if (pt_vaddr == NULL) > @@ -486,7 +486,7 @@ bail: > static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt, > const int max_pdp) > { > - struct page **pt_pages[GEN8_LEGACY_PDPS]; > + struct page **pt_pages[GEN8_LEGACY_PDPES]; > int i, ret; > > for (i = 0; i < max_pdp; i++) { > @@ -537,7 +537,7 @@ static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt, > return -ENOMEM; > > ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT); > - BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); > + BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPES); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index e377c7d..9d998ec 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -88,7 +88,7 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; > #define GEN8_PDE_MASK 0x1ff > #define GEN8_PTE_SHIFT 12 > #define GEN8_PTE_MASK 0x1ff > -#define GEN8_LEGACY_PDPS 4 > +#define GEN8_LEGACY_PDPES 4 > #define GEN8_PTES_PER_PAGE (PAGE_SIZE / sizeof(gen8_gtt_pte_t)) > #define GEN8_PDES_PER_PAGE (PAGE_SIZE / sizeof(gen8_ppgtt_pde_t)) > > @@ -273,12 +273,12 @@ struct i915_hw_ppgtt { > unsigned num_pd_pages; /* gen8+ */ > union { > struct page **pt_pages; > - struct page **gen8_pt_pages[GEN8_LEGACY_PDPS]; > + struct page **gen8_pt_pages[GEN8_LEGACY_PDPES]; > }; > struct page *pd_pages; > union { > uint32_t pd_offset; > - dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPS]; > + dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPES]; > }; > union { > dma_addr_t *pt_dma_addr; > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 18, 2014 at 09:40:51PM +0100, Daniel Vetter wrote: > On Thu, Dec 18, 2014 at 05:10:00PM +0000, Michel Thierry wrote: > > From: Ben Widawsky <benjamin.widawsky@intel.com> > > > > In gen8, 32b PPGTT has always had one "pdp" (it doesn't actually have > > one, but it resembles having one). The #define was confusing as is, and > > using "PDPE" is a much better description. > > > > sed -i 's/GEN8_LEGACY_PDPS/GEN8_LEGACY_PDPES/' drivers/gpu/drm/i915/*.[ch] > > Hm generally I've thought the abbreviations are pdp (for the page itself) > and pde (for the entries within). I still have no idea what pdpe means ... > > So either please explain that or pick one of the others. In case you fear the rebase pain of renaming this: 1. Export entire series as patches with git format-patch. 2. sed -e 's/PDPE/PDE/g' on all the patch files 3. Import the changed patches into a new fresh branch.' That's all. Feels really crazy the first time you do it, but after having done this a lot with the internal branch when something random (function name or so) changed in upstream it's a fairly simple trick to pull off ;-) Cheers, Daniel
On 18/12/14 20:44, Daniel Vetter wrote: > On Thu, Dec 18, 2014 at 09:40:51PM +0100, Daniel Vetter wrote: >> On Thu, Dec 18, 2014 at 05:10:00PM +0000, Michel Thierry wrote: >>> From: Ben Widawsky <benjamin.widawsky@intel.com> >>> >>> In gen8, 32b PPGTT has always had one "pdp" (it doesn't actually have >>> one, but it resembles having one). The #define was confusing as is, and >>> using "PDPE" is a much better description. >>> >>> sed -i 's/GEN8_LEGACY_PDPS/GEN8_LEGACY_PDPES/' drivers/gpu/drm/i915/*.[ch] >> >> Hm generally I've thought the abbreviations are pdp (for the page itself) >> and pde (for the entries within). I still have no idea what pdpe means ... >> >> So either please explain that or pick one of the others. > > In case you fear the rebase pain of renaming this: > > 1. Export entire series as patches with git format-patch. > 2. sed -e 's/PDPE/PDE/g' on all the patch files > 3. Import the changed patches into a new fresh branch.' > > That's all. Feels really crazy the first time you do it, but after having > done this a lot with the internal branch when something random (function > name or so) changed in upstream it's a fairly simple trick to pull off ;-) > > Cheers, Daniel The specific #define is inconsistent with the naming used for other #defines and in the associated comments. Here's the relevant chunk of i915_gem_gtt.h: /* GEN8 legacy style address is defined as a 3 level page table: * 31:30 | 29:21 | 20:12 | 11:0 * PDPE | PDE | PTE | offset * The difference as compared to normal x86 3 level page table is the PDPEs are * programmed via register. */ #define GEN8_PDPE_SHIFT 30 #define GEN8_PDPE_MASK 0x3 #define GEN8_PDE_SHIFT 21 #define GEN8_PDE_MASK 0x1ff #define GEN8_PTE_SHIFT 12 #define GEN8_PTE_MASK 0x1ff #define GEN8_LEGACY_PDPS 4 #define GEN8_PTES_PER_PAGE (PAGE_SIZE / sizeof(gen8_gtt_pte_t)) #define GEN8_PDES_PER_PAGE (PAGE_SIZE / sizeof(gen8_ppgtt_pde_t)) So 'LEGACY_PDPS' is inconsistent with 'PDPE_SHIFT/MASK'. PTE = Page Table Entry PDE = Page Directory Entry PDPE = Page Directory Pointer Entry See http://www.pagetable.com/?p=308 .Dave.
On Fri, Dec 19, 2014 at 12:32:23PM +0000, Dave Gordon wrote: > On 18/12/14 20:44, Daniel Vetter wrote: > > On Thu, Dec 18, 2014 at 09:40:51PM +0100, Daniel Vetter wrote: > >> On Thu, Dec 18, 2014 at 05:10:00PM +0000, Michel Thierry wrote: > >>> From: Ben Widawsky <benjamin.widawsky@intel.com> > >>> > >>> In gen8, 32b PPGTT has always had one "pdp" (it doesn't actually have > >>> one, but it resembles having one). The #define was confusing as is, and > >>> using "PDPE" is a much better description. > >>> > >>> sed -i 's/GEN8_LEGACY_PDPS/GEN8_LEGACY_PDPES/' drivers/gpu/drm/i915/*.[ch] > >> > >> Hm generally I've thought the abbreviations are pdp (for the page itself) > >> and pde (for the entries within). I still have no idea what pdpe means ... > >> > >> So either please explain that or pick one of the others. > > > > In case you fear the rebase pain of renaming this: > > > > 1. Export entire series as patches with git format-patch. > > 2. sed -e 's/PDPE/PDE/g' on all the patch files > > 3. Import the changed patches into a new fresh branch.' > > > > That's all. Feels really crazy the first time you do it, but after having > > done this a lot with the internal branch when something random (function > > name or so) changed in upstream it's a fairly simple trick to pull off ;-) > > > > Cheers, Daniel > > The specific #define is inconsistent with the naming used for other > #defines and in the associated comments. Here's the relevant chunk of > i915_gem_gtt.h: > > /* GEN8 legacy style address is defined as a 3 level page table: > * 31:30 | 29:21 | 20:12 | 11:0 > * PDPE | PDE | PTE | offset > * The difference as compared to normal x86 3 level page table is the > PDPEs are > * programmed via register. > */ > #define GEN8_PDPE_SHIFT 30 > #define GEN8_PDPE_MASK 0x3 > #define GEN8_PDE_SHIFT 21 > #define GEN8_PDE_MASK 0x1ff > #define GEN8_PTE_SHIFT 12 > #define GEN8_PTE_MASK 0x1ff > #define GEN8_LEGACY_PDPS 4 > #define GEN8_PTES_PER_PAGE (PAGE_SIZE / sizeof(gen8_gtt_pte_t)) > #define GEN8_PDES_PER_PAGE (PAGE_SIZE / > sizeof(gen8_ppgtt_pde_t)) > > So 'LEGACY_PDPS' is inconsistent with 'PDPE_SHIFT/MASK'. > > PTE = Page Table Entry > PDE = Page Directory Entry > PDPE = Page Directory Pointer Entry Ok, I just wasn't aware of the intel nomeclatura for page directories. Imo if we want to rename we should consider the names established by the linux vm, mostly because svm and also because they're actually sane: pgd -> pud -> pmd -> pt if you have 4 levels pgd -> pmd -> pt if you have 3 levels and just pgd -> pt for just 2 g = global u = upper m = middle But I guess we can bikeshed this later on. Just please add the above expansion to the commit message and mention that this is what intel has picked in the IA PRM, too. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 75a29a3..9639310 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -375,7 +375,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm, pt_vaddr = NULL; for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) { - if (WARN_ON(pdpe >= GEN8_LEGACY_PDPS)) + if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES)) break; if (pt_vaddr == NULL) @@ -486,7 +486,7 @@ bail: static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt, const int max_pdp) { - struct page **pt_pages[GEN8_LEGACY_PDPS]; + struct page **pt_pages[GEN8_LEGACY_PDPES]; int i, ret; for (i = 0; i < max_pdp; i++) { @@ -537,7 +537,7 @@ static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt, return -ENOMEM; ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT); - BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); + BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPES); return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index e377c7d..9d998ec 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -88,7 +88,7 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; #define GEN8_PDE_MASK 0x1ff #define GEN8_PTE_SHIFT 12 #define GEN8_PTE_MASK 0x1ff -#define GEN8_LEGACY_PDPS 4 +#define GEN8_LEGACY_PDPES 4 #define GEN8_PTES_PER_PAGE (PAGE_SIZE / sizeof(gen8_gtt_pte_t)) #define GEN8_PDES_PER_PAGE (PAGE_SIZE / sizeof(gen8_ppgtt_pde_t)) @@ -273,12 +273,12 @@ struct i915_hw_ppgtt { unsigned num_pd_pages; /* gen8+ */ union { struct page **pt_pages; - struct page **gen8_pt_pages[GEN8_LEGACY_PDPS]; + struct page **gen8_pt_pages[GEN8_LEGACY_PDPES]; }; struct page *pd_pages; union { uint32_t pd_offset; - dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPS]; + dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPES]; }; union { dma_addr_t *pt_dma_addr;