From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 12/26] drm/i915: Page table helpers, and define renames Date: Tue, 18 Mar 2014 17:58:08 -0700 Message-ID: <20140319005807.GA7811@bwidawsk.net> References: <1395121738-29126-1-git-send-email-benjamin.widawsky@intel.com> <1395121738-29126-13-git-send-email-benjamin.widawsky@intel.com> <20140318090558.GD18530@nuc-i3427.alporthouse.com> <20140318112958.440d1278@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id B03D06E496 for ; Tue, 18 Mar 2014 17:58:43 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140318112958.440d1278@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Tue, Mar 18, 2014 at 11:29:58AM -0700, Jesse Barnes wrote: > On Tue, 18 Mar 2014 09:05:58 +0000 > Chris Wilson 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