On Tue, 23 Apr 2013 23:15:34 -0700
Ben Widawsky <ben at bwidawsk.net> wrote:
> I think this is a nice generalization on it's own, but it's primarily
> prep work for my PPGTT support. Does this bother anyone?
>
> The only down side I can see is we waste 2k of cpu unmappable space
> (unless we have something else that is <= 2k and doesn't need to be page
> aligned).
>
> v2: Align PDEs to 64b in GTT
> Allocate the node dynamically so we can use drm_mm_put_block
> Now tested on IGT
> Allocate node at the top to avoid fragmentation (Chris)
>
> v3: Use Chris' top down allocator
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem_gtt.c | 54 ++++++++++++++++++++++++++-----------
> include/drm/drm_mm.h | 3 +++
> 3 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0a9c7cd..9ab6254 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -432,6 +432,7 @@ struct i915_gtt {
> #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
>
> struct i915_hw_ppgtt {
> + struct drm_mm_node *node;
> struct drm_device *dev;
> unsigned num_pd_entries;
> struct page **pt_pages;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 975adaa..b825d7b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -219,6 +219,8 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> {
> int i;
>
> + drm_mm_put_block(ppgtt->node);
> +
> if (ppgtt->pt_dma_addr) {
> for (i = 0; i < ppgtt->num_pd_entries; i++)
> pci_unmap_page(ppgtt->dev->pdev,
> @@ -235,16 +237,27 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
>
> static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> {
> +#define GEN6_PD_ALIGN (PAGE_SIZE * 16)
> +#define GEN6_PD_SIZE (512 * PAGE_SIZE)
> struct drm_device *dev = ppgtt->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - unsigned first_pd_entry_in_global_pt;
> int i;
> int ret = -ENOMEM;
>
> - /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024
> - * entries. For aliasing ppgtt support we just steal them at the end for
> - * now. */
> - first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
> + /* PPGTT PDEs reside in the GGTT stolen space, and consists of 512
> + * entries. The allocator works in address space sizes, so it's
> + * multiplied by page size. We allocate at the top of the GTT to avoid
> + * fragmentation.
> + */
> + BUG_ON(!drm_mm_initialized(&dev_priv->mm.gtt_space));
> + ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space,
> + ppgtt->node, GEN6_PD_SIZE,
> + GEN6_PD_ALIGN, 0,
> + dev_priv->gtt.mappable_end,
> + dev_priv->gtt.total - PAGE_SIZE,
> + DRM_MM_TOPDOWN);
> + if (ret)
> + return ret;
I guess I won't see why you're doing this until later; presumably it
makes PPGTT management easier in subsequent patches?
>
> ppgtt->num_pd_entries = 512;
> ppgtt->enable = gen6_ppgtt_enable;
> @@ -253,8 +266,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> ppgtt->cleanup = gen6_ppgtt_cleanup;
> ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
> GFP_KERNEL);
> - if (!ppgtt->pt_pages)
> + if (!ppgtt->pt_pages) {
> + drm_mm_put_block(ppgtt->node);
> return -ENOMEM;
> + }
>
> for (i = 0; i < ppgtt->num_pd_entries; i++) {
> ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL);
> @@ -284,7 +299,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> ppgtt->clear_range(ppgtt, 0,
> ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES);
>
> - ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gen6_gtt_pte_t);
> + DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n",
> + ppgtt->node->size >> 20,
> + ppgtt->node->start / PAGE_SIZE);
> + ppgtt->pd_offset = ppgtt->node->start / PAGE_SIZE * sizeof(gen6_gtt_pte_t);
>
> return 0;
>
> @@ -315,6 +333,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> if (!ppgtt)
> return -ENOMEM;
>
> + ppgtt->node = kzalloc(sizeof(*ppgtt->node), GFP_KERNEL);
> + if (!ppgtt->node) {
> + kfree(ppgtt);
> + return -ENOMEM;
> + }
> +
Why not just put the drm_mm node directly into the hw_ppgtt struct
instead of allocating it? Every context/ppgtt instance will need one,
right?
> ppgtt->dev = dev;
> ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma;
>
> @@ -323,10 +347,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> else
> BUG();
>
> - if (ret)
> + if (ret) {
> + drm_mm_put_block(ppgtt->node);
> kfree(ppgtt);
> - else
> + } else {
> dev_priv->mm.aliasing_ppgtt = ppgtt;
> + }
>
> return ret;
> }
> @@ -407,6 +433,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE,
> dev_priv->gtt.total / PAGE_SIZE);
>
> + if (dev_priv->mm.aliasing_ppgtt)
> + gen6_write_pdes(dev_priv->mm.aliasing_ppgtt);
> +
> list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
> i915_gem_clflush_object(obj);
> i915_gem_gtt_bind_object(obj, obj->cache_level);
> @@ -651,12 +680,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
> if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
> int ret;
>
> - if (INTEL_INFO(dev)->gen <= 7) {
> - /* PPGTT pdes are stolen from global gtt ptes, so shrink the
> - * aperture accordingly when using aliasing ppgtt. */
> - gtt_size -= 512 * PAGE_SIZE;
> - }
> -
> i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
>
> ret = i915_gem_init_aliasing_ppgtt(dev);
> @@ -665,7 +688,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
>
> DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
> drm_mm_takedown(&dev_priv->mm.gtt_space);
> - gtt_size += 512 * PAGE_SIZE;
> }
> i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> }
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index 752e846..ce7b13b 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -53,6 +53,9 @@ enum drm_mm_search_flags {
> DRM_MM_SEARCH_BELOW = 1<<1,
> };
>
> +#define DRM_MM_BOTTOMUP DRM_MM_CREATE_DEFAULT, DRM_MM_SEARCH_DEFAULT
> +#define DRM_MM_TOPDOWN DRM_MM_CREATE_TOP, DRM_MM_SEARCH_BELOW
> +
> struct drm_mm_node {
> struct list_head node_list;
> struct list_head hole_stack;
These last few hunks seem like they belong in different patches?
With the above addressed:
Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
