Re: [PATCH 03/24] drm/i915: Rename to GEN8_LEGACY_PDPES

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx