[PATCH 1/5] drm/i915: trivial: kill-agp collateral cleanups

[PATCH 1/5] drm/i915: trivial: kill-agp collateral cleanups

[Ben Widawsky]

- i915_gem_init_aliasing_ppgtt should now be static
- move i915_gem_init_ppgtt declaration to the right place

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     | 3 +--
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2b93a4..9a57e66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1522,7 +1522,6 @@ int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_l3_remap(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
@@ -1576,7 +1575,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
-int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
+void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
 void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
 			    struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index eac2cec..597efbd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	}
 }
 
-int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
+static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_ppgtt *ppgtt;
-- 
1.8.0.2

[PATCH 2/5] drm/i915: Reclaim GTT space for failed PPGTT

[Ben Widawsky]

When the PPGTT init fails, we may as well reuse the space that we were
reserving for the PPGTT PDEs.

This also fixes an extraneous mutex_unlock.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 597efbd..94d21aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -525,12 +525,20 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node,
 			*end -= 4096;
 	}
 }
-
 void i915_gem_setup_global_gtt(struct drm_device *dev,
 			       unsigned long start,
 			       unsigned long mappable_end,
 			       unsigned long end)
 {
+	/* Let GEM Manage all of the aperture.
+	 *
+	 * However, leave one page at the end still bound to the scratch page.
+	 * There are a number of places where the hardware apparently prefetches
+	 * past the end of the object, and we've seen multiple hangs with the
+	 * GPU head pointer stuck in a batchbuffer bound at the last page of the
+	 * aperture.  One page should be enough to keep any prefetching inside
+	 * of the aperture.
+	 */
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_mm_node *entry;
 	struct drm_i915_gem_object *obj;
@@ -593,12 +601,12 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long gtt_size, mappable_size;
-	int ret;
 
 	gtt_size = dev_priv->mm.gtt->gtt_total_entries << PAGE_SHIFT;
 	mappable_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
+		int ret;
 		/* PPGTT pdes are stolen from global gtt ptes, so shrink the
 		 * aperture accordingly when using aliasing ppgtt. */
 		gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
@@ -606,23 +614,14 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
 		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 
 		ret = i915_gem_init_aliasing_ppgtt(dev);
-		if (ret) {
-			mutex_unlock(&dev->struct_mutex);
+		if (!ret)
 			return;
-		}
-	} else {
-		/* Let GEM Manage all of the aperture.
-		 *
-		 * However, leave one page at the end still bound to the scratch
-		 * page.  There are a number of places where the hardware
-		 * apparently prefetches past the end of the object, and we've
-		 * seen multiple hangs with the GPU head pointer stuck in a
-		 * batchbuffer bound at the last page of the aperture.  One page
-		 * should be enough to keep any prefetching inside of the
-		 * aperture.
-		 */
-		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
+
+		DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
+		drm_mm_takedown(&dev_priv->mm.gtt_space);
+		gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
 	}
+	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 }
 
 static int setup_scratch_page(struct drm_device *dev)
-- 
1.8.0.2

[PATCH 3/5] drm/i915: Extract gen6 aliasing ppgtt code

[Ben Widawsky]

This refactor clearly identifies the GEN specific portion of the aliased
ppgtt code. Aside from some of the gen specific parts being extracted,
it also preps us for an upcoming patch that pulls out the size, which
has some nice benefits.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 94d21aa..3b1bff1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	}
 }
 
-static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
+static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
 {
+	struct drm_device *dev = ppgtt->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_hw_ppgtt *ppgtt;
 	unsigned first_pd_entry_in_global_pt;
-	int i;
-	int ret = -ENOMEM;
+	int i, 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 = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES;
-
-	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
-	if (!ppgtt)
-		return ret;
-
-	ppgtt->dev = dev;
 	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
 	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
 				  GFP_KERNEL);
-	if (!ppgtt->pt_pages)
-		goto err_ppgtt;
 
 	for (i = 0; i < ppgtt->num_pd_entries; i++) {
 		ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL);
@@ -152,8 +143,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 					       0, 4096,
 					       PCI_DMA_BIDIRECTIONAL);
 
-			if (pci_dma_mapping_error(dev->pdev,
-						  pt_addr)) {
+			if (pci_dma_mapping_error(dev->pdev, pt_addr)) {
 				ret = -EIO;
 				goto err_pd_pin;
 
@@ -162,15 +152,11 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 		}
 	}
 
-	ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma;
-
 	i915_ppgtt_clear_range(ppgtt, 0,
 			       ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES);
 
 	ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t);
 
-	dev_priv->mm.aliasing_ppgtt = ppgtt;
-
 	return 0;
 
 err_pd_pin:
@@ -185,9 +171,32 @@ err_pt_alloc:
 		if (ppgtt->pt_pages[i])
 			__free_page(ppgtt->pt_pages[i]);
 	}
-	kfree(ppgtt->pt_pages);
-err_ppgtt:
-	kfree(ppgtt);
+
+	if (!ppgtt->pt_pages)
+		kfree(ppgtt->pt_pages);
+
+	return ret;
+}
+
+
+static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_hw_ppgtt *ppgtt;
+	int ret = -ENOMEM;
+
+	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
+	if (!ppgtt)
+		return ret;
+
+	ppgtt->dev = dev;
+	ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma;
+
+	ret = gen6_init_aliasing_ppgtt(ppgtt);
+	if (ret)
+		kfree(ppgtt);
+	else
+		dev_priv->mm.aliasing_ppgtt = ppgtt;
 
 	return ret;
 }
-- 
1.8.0.2

[PATCH 4/5] drm/i915: Aliased PPGTT size abstraction

[Ben Widawsky]

Aside from being a nice prep for some work I'm doing in PPGTT, this also
leaves us with a more accurate size in our gtt_total_entries member.
Since we can't actually use the PPGTT reserved part of the GGTT.

This will help some of the existing assertions find issues which would
overwrite the PPGTT PDEs.

Another benefit is for platforms which don't use the entire 2GB GTT,
this will save GTT space. For example if a platform has 1 1GB GTT, this
patch should save 1MB of GTT space (512MB has an even greater savings).
I don't have a platform to easily test this however.

If/when we ever move to real PPGTT, I think allocating the PDEs as
objects will make the most sense.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  1 +
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 31 ++++++++++++++++++++++++-------
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 882a735..bfd5039 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1604,6 +1604,7 @@ static int i915_ppgtt_info(struct seq_file *m, void *data)
 
 		seq_printf(m, "aliasing PPGTT:\n");
 		seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset);
+		seq_printf(m, "mapped size: 0x%08x\n", ppgtt->mapped_size);
 	}
 	seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9a57e66..61b8d6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -364,10 +364,11 @@ struct intel_device_info {
 	u8 has_llc:1;
 };
 
-#define I915_PPGTT_PD_ENTRIES 512
+#define GEN6_MAX_PD_ENTRIES 512
 #define I915_PPGTT_PT_ENTRIES 1024
 struct i915_hw_ppgtt {
 	struct drm_device *dev;
+	uint64_t mapped_size;
 	unsigned num_pd_entries;
 	struct page **pt_pages;
 	uint32_t pd_offset;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 3b1bff1..2788ca5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -108,18 +108,25 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	}
 }
 
-static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
+static int gen6_init_ppgtt(struct i915_hw_ppgtt *ppgtt, uint64_t 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, ret = -ENOMEM;
 
+	if (dev_priv->mm.aliasing_ppgtt)
+		return -EBUSY;
+
+	/* Each PDE maps 4MB (1k PAGE_SIZE pages)*/
+	ppgtt->mapped_size = round_up(size, 1<<22);
+	ppgtt->num_pd_entries = ppgtt->mapped_size >> 22;
+	BUG_ON(ppgtt->num_pd_entries > GEN6_MAX_PD_ENTRIES);
+
 	/* 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 = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES;
-	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
+	first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - ppgtt->num_pd_entries;
 	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
 				  GFP_KERNEL);
 
@@ -157,6 +164,9 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
 
 	ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t);
 
+	DRM_INFO("Allocated %d PDEs for PPGTT\n", ppgtt->num_pd_entries);
+	DRM_DEBUG_DRIVER("First PDE: %d\n", first_pd_entry_in_global_pt);
+
 	return 0;
 
 err_pd_pin:
@@ -192,7 +202,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 	ppgtt->dev = dev;
 	ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma;
 
-	ret = gen6_init_aliasing_ppgtt(ppgtt);
+	ret = gen6_init_ppgtt(ppgtt, dev_priv->mm.gtt_total);
 	if (ret)
 		kfree(ppgtt);
 	else
@@ -615,20 +625,27 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
 	mappable_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
+		/* Each PDE represents 1k PAGE_SIZE objects */
+		const int reserve_pd_space = gtt_size >> 10;
 		int ret;
+
+		BUG_ON(!reserve_pd_space);
+
 		/* PPGTT pdes are stolen from global gtt ptes, so shrink the
 		 * aperture accordingly when using aliasing ppgtt. */
-		gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
+		gtt_size -= reserve_pd_space;
 
 		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 
 		ret = i915_gem_init_aliasing_ppgtt(dev);
-		if (!ret)
+		if (!ret) {
+			dev_priv->mm.gtt->gtt_total_entries = gtt_size >> PAGE_SHIFT;
 			return;
+		}
 
 		DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
 		drm_mm_takedown(&dev_priv->mm.gtt_space);
-		gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
+		gtt_size += reserve_pd_space;
 	}
 	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 }
-- 
1.8.0.2

[PATCH 5/5] drm/i915: Dynamically calculate dclv

[Ben Widawsky]

Directory CacheLine Valid controls which PDEs are held in the directory
cache. Each bit represents 16 PDEs (16 PDEs at 4 bytes per entry is 1
cacheline, and therefore, almost makes sense).

Since we can now have an aliasing PPGTT which isn't 2GB, theoretically,
we should also modify the DCLV value to fit accordingly.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++--
 drivers/gpu/drm/i915/i915_reg.h     | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2788ca5..c26aa3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -348,11 +348,12 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
 	}
 
 	for_each_ring(ring, dev_priv, i) {
+		uint64_t dclv = ppgtt->mapped_size >> 26;
+		dclv = (1ULL << dclv) - 1;
 		if (INTEL_INFO(dev)->gen >= 7)
 			I915_WRITE(RING_MODE_GEN7(ring),
 				   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
-
-		I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G);
+		I915_WRITE(RING_PP_DIR_DCLV(ring), dclv);
 		I915_WRITE(RING_PP_DIR_BASE(ring), pd_offset);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5b3020f..4e8ecab 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -117,7 +117,6 @@
 #define RING_PP_DIR_BASE(ring)		((ring)->mmio_base+0x228)
 #define RING_PP_DIR_BASE_READ(ring)	((ring)->mmio_base+0x518)
 #define RING_PP_DIR_DCLV(ring)		((ring)->mmio_base+0x220)
-#define   PP_DIR_DCLV_2G		0xffffffff
 
 #define GAM_ECOCHK			0x4090
 #define   ECOCHK_SNB_BIT		(1<<10)
-- 
1.8.0.2

Re: [PATCH 3/5] drm/i915: Extract gen6 aliasing ppgtt code

[Chris Wilson]

On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> This refactor clearly identifies the GEN specific portion of the aliased
> ppgtt code. Aside from some of the gen specific parts being extracted,
> it also preps us for an upcoming patch that pulls out the size, which
> has some nice benefits.

Error handling looks a little wonky.

> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 94d21aa..3b1bff1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
>  	}
>  }
>  
> -static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> +static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
>  {
> +	struct drm_device *dev = ppgtt->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct i915_hw_ppgtt *ppgtt;
>  	unsigned first_pd_entry_in_global_pt;
> -	int i;
> -	int ret = -ENOMEM;
> +	int i, 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 = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES;
> -
> -	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> -	if (!ppgtt)
> -		return ret;
> -
> -	ppgtt->dev = dev;
>  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
>  				  GFP_KERNEL);
> -	if (!ppgtt->pt_pages)
> -		goto err_ppgtt;

I think you still want this...
  
> @@ -185,9 +171,32 @@ err_pt_alloc:
>  		if (ppgtt->pt_pages[i])
>  			__free_page(ppgtt->pt_pages[i]);
>  	}
> -	kfree(ppgtt->pt_pages);
> -err_ppgtt:
> -	kfree(ppgtt);
> +
> +	if (!ppgtt->pt_pages)
> +		kfree(ppgtt->pt_pages);

And will want to reconsider this chunk.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: Extract gen6 aliasing ppgtt code

[Ben Widawsky]

On Sat, 29 Dec 2012 11:49:01 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > This refactor clearly identifies the GEN specific portion of the
> > aliased ppgtt code. Aside from some of the gen specific parts being
> > extracted, it also preps us for an upcoming patch that pulls out
> > the size, which has some nice benefits.
> 
> Error handling looks a little wonky.

I've not looked in much detail yet, but I'll assume you're right since
I did this pretty quickly. So given that assumption, what do you think
of the patches, useful, or waste of code complexity?

> 
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 51
> > ++++++++++++++++++++++--------------- 1 file changed, 30
> > insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 94d21aa..3b1bff1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -108,29 +108,20 @@ static void i915_ppgtt_clear_range(struct
> > i915_hw_ppgtt *ppgtt, }
> >  }
> >  
> > -static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> > +static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
> >  {
> > +	struct drm_device *dev = ppgtt->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct i915_hw_ppgtt *ppgtt;
> >  	unsigned first_pd_entry_in_global_pt;
> > -	int i;
> > -	int ret = -ENOMEM;
> > +	int i, 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 =
> > dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; -
> > -	ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL);
> > -	if (!ppgtt)
> > -		return ret;
> > -
> > -	ppgtt->dev = dev;
> >  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> >  	ppgtt->pt_pages = kzalloc(sizeof(struct page
> > *)*ppgtt->num_pd_entries, GFP_KERNEL);
> > -	if (!ppgtt->pt_pages)
> > -		goto err_ppgtt;
> 
> I think you still want this...
>   
> > @@ -185,9 +171,32 @@ err_pt_alloc:
> >  		if (ppgtt->pt_pages[i])
> >  			__free_page(ppgtt->pt_pages[i]);
> >  	}
> > -	kfree(ppgtt->pt_pages);
> > -err_ppgtt:
> > -	kfree(ppgtt);
> > +
> > +	if (!ppgtt->pt_pages)
> > +		kfree(ppgtt->pt_pages);
> 
> And will want to reconsider this chunk.
> -Chris
> 

Re: [PATCH 3/5] drm/i915: Extract gen6 aliasing ppgtt code

[Chris Wilson]

On Sat, 29 Dec 2012 11:45:09 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sat, 29 Dec 2012 11:49:01 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, 28 Dec 2012 20:27:27 -0800, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > This refactor clearly identifies the GEN specific portion of the
> > > aliased ppgtt code. Aside from some of the gen specific parts being
> > > extracted, it also preps us for an upcoming patch that pulls out
> > > the size, which has some nice benefits.
> > 
> > Error handling looks a little wonky.
> 
> I've not looked in much detail yet, but I'll assume you're right since
> I did this pretty quickly. So given that assumption, what do you think
> of the patches, useful, or waste of code complexity?

The mention of transitioning ppgtt to use objects sounds intriguing, and
without digging into the details, does seem a worthwhile design. This
series looks fine as a mean of cutting the code up into manageable
chunks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: trivial: kill-agp collateral cleanups

[Ben Widawsky]

On Tue, Jan 29, 2013 at 01:49:22PM +0100, Daniel Vetter wrote:
> On Fri, Jan 25, 2013 at 04:41:03PM -0800, Ben Widawsky wrote:
> > - i915_gem_init_aliasing_ppgtt should now be static
> > - move i915_gem_init_ppgtt declaration to the right place
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I've tried to apply this patch on top of latest dinq, and there's nothing
> left after resolving conflicts. Have I screwed something up?
> -Daniel

No, I think you're right. I think it no longer applies.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/5] drm/i915: trivial: kill-agp collateral cleanups

[Daniel Vetter]

On Fri, Jan 25, 2013 at 04:41:03PM -0800, Ben Widawsky wrote:
> - i915_gem_init_aliasing_ppgtt should now be static
> - move i915_gem_init_ppgtt declaration to the right place
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I've tried to apply this patch on top of latest dinq, and there's nothing
left after resolving conflicts. Have I screwed something up?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 3 +--
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 09753e5..1af8e73 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1598,7 +1598,6 @@ int __must_check i915_gem_init(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  void i915_gem_l3_remap(struct drm_device *dev);
>  void i915_gem_init_swizzling(struct drm_device *dev);
> -void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
>  int __must_check i915_gem_idle(struct drm_device *dev);
> @@ -1653,7 +1652,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  				   struct drm_file *file);
>  
>  /* i915_gem_gtt.c */
> -int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> +void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
>  void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
>  			    struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a0ba4a9..462dec9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
>  	}
>  }
>  
> -int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> +static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_ppgtt *ppgtt;
> -- 
> 1.8.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/5] drm/i915: trivial: kill-agp collateral cleanups

[Ben Widawsky]

- i915_gem_init_aliasing_ppgtt should now be static
- move i915_gem_init_ppgtt declaration to the right place

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     | 3 +--
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 09753e5..1af8e73 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1598,7 +1598,6 @@ int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_l3_remap(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
@@ -1653,7 +1652,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
-int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
+void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
 void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
 			    struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a0ba4a9..462dec9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	}
 }
 
-int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
+static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_ppgtt *ppgtt;
-- 
1.8.1.1