Re: [PATCH 24/62] drm/i915/bdw: Initialize the PDEs

Damien Lespiau
2013-11-04T14:10:57Z

On Sat, Nov 02, 2013 at 09:07:22PM -0700, Ben Widawsky wrote:
> The upcoming clear and insert routines will expect that PDEs all point
> to valid Page Directories. Doing that lazily doesn't really buy us
> anything.
>
> The page allocation is done regardless earlier in init so it shouldn't
> hurt set the PDEs.
>
> v2: Squash in patches to implement fixed PDE write function:
>
> - If I had done this in the first place, the bug that's going to be
> fixed in an upcoming patch would have been much easier to find.
>
> - Use WB for PDEs.
>
> The PAT bit is used for page size. 2ME PDEs aren't even supported in
> BDW, so this was completely invalid. The solution is to make our
> PDEs WB+LLC instead of the pervious WB+eLLC. As far as I can guess,
> this change won't matter for performance.
>
> Thanks to Ville for the quick correction when discussing on IRC.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

With the small bikeshed below:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

--
Damien

> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4a11f51..bae71b4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -80,6 +80,19 @@ static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr,
> return pte;
> }
>
> +static inline gen8_gtt_pte_t gen8_pde_encode(struct drm_device *dev,
> + dma_addr_t addr,
> + enum i915_cache_level level)

Should be returning a gen8_ppgtt_pde_t (not a bug as they are the same
size, of course).

> +{
> + gen8_ppgtt_pde_t pde = _PAGE_PRESENT | _PAGE_RW;
> + pde |= addr;
> + if (level != I915_CACHE_NONE)
> + pde |= PPAT_CACHED_PDE_INDEX;
> + else
> + pde |= PPAT_UNCACHED_INDEX;
> + return pde;
> +}
> +
> static gen6_gtt_pte_t snb_pte_encode(dma_addr_t addr,
> enum i915_cache_level level,
> bool valid)
> @@ -285,6 +298,20 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
> }
> }
>
> + /* For now, the PPGTT helper functions all require that the PDEs are
> + * plugged in correctly. So we do that now/here. For aliasing PPGTT, we
> + * will never need to touch the PDEs again */
> + for (i = 0; i < max_pdp; i++) {
> + gen8_ppgtt_pde_t *pd_vaddr;
> + pd_vaddr = kmap_atomic(&ppgtt->pd_pages[i]);
> + for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
> + dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
> + pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
> + I915_CACHE_LLC);
> + }
> + kunmap_atomic(pd_vaddr);
> + }
> +
> DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d wasted)\n",
> ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp);
> DRM_DEBUG_DRIVER("Allocated %d pages for page tables (%lld wasted)\n",
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx