[PATCH 1/3] drm/i915: s/i915_gem_do_init/i915_gem_init_global_gtt

[PATCH 1/3] drm/i915: s/i915_gem_do_init/i915_gem_init_global_gtt

[Daniel Vetter]

... because this is what it actually doesn now that we have the global
gtt vs. ppgtt split.

Also move it to the other global gtt functions in i915_gem_gtt.c

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c     |    5 +++--
 drivers/gpu/drm/i915/i915_drv.h     |    8 ++++----
 drivers/gpu/drm/i915/i915_gem.c     |   22 ++--------------------
 drivers/gpu/drm/i915/i915_gem_gtt.c |   19 +++++++++++++++++++
 4 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fdff009..b6e0a45 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1210,7 +1210,7 @@ static int i915_load_gem_init(struct drm_device *dev)
 		/* For paranoia keep the guard page in between. */
 		gtt_size -= PAGE_SIZE;
 
-		i915_gem_do_init(dev, 0, mappable_size, gtt_size);
+		i915_gem_init_global_gtt(dev, 0, mappable_size, gtt_size);
 
 		ret = i915_gem_init_aliasing_ppgtt(dev);
 		if (ret)
@@ -1226,7 +1226,8 @@ static int i915_load_gem_init(struct drm_device *dev)
 		 * should be enough to keep any prefetching inside of the
 		 * aperture.
 		 */
-		i915_gem_do_init(dev, 0, mappable_size, gtt_size - PAGE_SIZE);
+		i915_gem_init_global_gtt(dev, 0, mappable_size,
+					 gtt_size - PAGE_SIZE);
 	}
 
 	ret = i915_gem_init_hw(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6098b0..0499ded 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1245,10 +1245,6 @@ int __must_check i915_gem_init_hw(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);
-void i915_gem_do_init(struct drm_device *dev,
-		      unsigned long start,
-		      unsigned long mappable_end,
-		      unsigned long end);
 int __must_check i915_gpu_idle(struct drm_device *dev, bool do_retire);
 int __must_check i915_gem_idle(struct drm_device *dev);
 int __must_check i915_add_request(struct intel_ring_buffer *ring,
@@ -1297,6 +1293,10 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
 				enum i915_cache_level cache_level);
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
 void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
+void i915_gem_init_global_gtt(struct drm_device *dev,
+			      unsigned long start,
+			      unsigned long mappable_end,
+			      unsigned long end);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 863e14a..e2bf58b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -125,25 +125,6 @@ i915_gem_object_is_inactive(struct drm_i915_gem_object *obj)
 	return obj->gtt_space && !obj->active && obj->pin_count == 0;
 }
 
-void i915_gem_do_init(struct drm_device *dev,
-		      unsigned long start,
-		      unsigned long mappable_end,
-		      unsigned long end)
-{
-	drm_i915_private_t *dev_priv = dev->dev_private;
-
-	drm_mm_init(&dev_priv->mm.gtt_space, start, end - start);
-
-	dev_priv->mm.gtt_start = start;
-	dev_priv->mm.gtt_mappable_end = mappable_end;
-	dev_priv->mm.gtt_end = end;
-	dev_priv->mm.gtt_total = end - start;
-	dev_priv->mm.mappable_gtt_total = min(end, mappable_end) - start;
-
-	/* Take over this portion of the GTT */
-	intel_gtt_clear_range(start / PAGE_SIZE, (end-start) / PAGE_SIZE);
-}
-
 int
 i915_gem_init_ioctl(struct drm_device *dev, void *data,
 		    struct drm_file *file)
@@ -155,7 +136,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 
 	mutex_lock(&dev->struct_mutex);
-	i915_gem_do_init(dev, args->gtt_start, args->gtt_end, args->gtt_end);
+	i915_gem_init_global_gtt(dev, args->gtt_start,
+				 args->gtt_end, args->gtt_end);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 72be806..98ed612 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -421,3 +421,22 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
 
 	undo_idling(dev_priv, interruptible);
 }
+
+void i915_gem_init_global_gtt(struct drm_device *dev,
+			      unsigned long start,
+			      unsigned long mappable_end,
+			      unsigned long end)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	drm_mm_init(&dev_priv->mm.gtt_space, start, end - start);
+
+	dev_priv->mm.gtt_start = start;
+	dev_priv->mm.gtt_mappable_end = mappable_end;
+	dev_priv->mm.gtt_end = end;
+	dev_priv->mm.gtt_total = end - start;
+	dev_priv->mm.mappable_gtt_total = min(end, mappable_end) - start;
+
+	/* Take over this portion of the GTT */
+	intel_gtt_clear_range(start / PAGE_SIZE, (end-start) / PAGE_SIZE);
+}
-- 
1.7.8.3

[PATCH 2/3] drm/i915: the intel gtt is _not_ an agp bridge!

[Daniel Vetter]

So don't call it like that.

Also rip out a confusing comment and instead explain what's really
going on.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b6e0a45..c2d9eae 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1957,7 +1957,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 {
 	struct drm_i915_private *dev_priv;
 	int ret = 0, mmio_bar;
-	uint32_t agp_size;
+	uint32_t aperture_size;
 
 	/* i915 has 4 more counters */
 	dev->counters += 4;
@@ -2011,16 +2011,16 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto out_rmmap;
 	}
 
-	agp_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-		io_mapping_create_wc(dev->agp->base, agp_size);
+		io_mapping_create_wc(dev->agp->base, aperture_size);
 	if (dev_priv->mm.gtt_mapping == NULL) {
 		ret = -EIO;
 		goto out_rmmap;
 	}
 
-	i915_mtrr_setup(dev_priv, dev->agp->base, agp_size);
+	i915_mtrr_setup(dev_priv, dev->agp->base, aperture_size);
 
 	/* The i915 workqueue is primarily used for batched retirement of
 	 * requests (and thus managing bo) once the task has been completed
@@ -2272,7 +2272,7 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
  * mode setting case, we want to restore the kernel's initial mode (just
  * in case the last client left us in a bad state).
  *
- * Additionally, in the non-mode setting case, we'll tear down the AGP
+ * Additionally, in the non-mode setting case, we'll tear down the GTT
  * and DMA structures, since the kernel won't be using them, and clea
  * up any GEM state.
  */
@@ -2350,16 +2350,10 @@ struct drm_ioctl_desc i915_ioctls[] = {
 
 int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
 
-/**
- * Determine if the device really is AGP or not.
- *
- * All Intel graphics chipsets are treated as AGP, even if they are really
- * PCI-e.
- *
- * \param dev   The device to be tested.
- *
- * \returns
- * A value of 1 is always retured to indictate every i9x5 is AGP.
+/*
+ * This is really ugly: Because old userspace abused the linux agp interface to
+ * manage the gtt, we need to claim that all intel devices are agp.  For
+ * otherwise the drm core refuses to initialize the agp support code.
  */
 int i915_driver_device_is_agp(struct drm_device * dev)
 {
-- 
1.7.8.3

Re: [PATCH 2/3] drm/i915: the intel gtt is _not_ an agp bridge!

[Chris Wilson]

On Mon, 26 Mar 2012 09:45:41 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> So don't call it like that.
> 
> Also rip out a confusing comment and instead explain what's really
> going on.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> +/*
> + * This is really ugly: Because old userspace abused the linux agp interface to
> + * manage the gtt, we need to claim that all intel devices are agp.  For
> + * otherwise the drm core refuses to initialize the agp support code.
>   */

So no new device entries, right?

For the patch itself,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: the intel gtt is _not_ an agp bridge!

[Daniel Vetter]

On Mon, Mar 26, 2012 at 09:49:36AM +0100, Chris Wilson wrote:
> On Mon, 26 Mar 2012 09:45:41 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > So don't call it like that.
> > 
> > Also rip out a confusing comment and instead explain what's really
> > going on.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > +/*
> > + * This is really ugly: Because old userspace abused the linux agp interface to
> > + * manage the gtt, we need to claim that all intel devices are agp.  For
> > + * otherwise the drm core refuses to initialize the agp support code.
> >   */
> 
> So no new device entries, right?

I plan to merge hsw and vlv support with the current intel-gtt mess and
clean up afterwards. This way there should be less risk that Jesse fumbles
a rebase ;-)

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/3] drm/i915: the intel gtt is _not_ an agp bridge!

[Daniel Vetter]

On Mon, Mar 26, 2012 at 09:49:36AM +0100, Chris Wilson wrote:
> On Mon, 26 Mar 2012 09:45:41 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > So don't call it like that.
> > 
> > Also rip out a confusing comment and instead explain what's really
> > going on.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > +/*
> > + * This is really ugly: Because old userspace abused the linux agp interface to
> > + * manage the gtt, we need to claim that all intel devices are agp.  For
> > + * otherwise the drm core refuses to initialize the agp support code.
> >   */
> 
> So no new device entries, right?
> 
> For the patch itself,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

All three queued up for -next, thanks for testing and reviewing.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 3/3] drm/i915: clear the entire gtt when using gem

[Daniel Vetter]

We've lost our guard page somewhere in the gtt rewrite, this patch
here will restore it.

Exercised by i-g-t/tests/gem_cs_prefetch.

v2: Substract the guard page from the range we're supposed to manage
with gem. Suggested by Chris Wilson to increase the odds of old ums +
gem userspace not blowing up. To compensate for the loss of a page,
don't substract the guard page in the modeset init code any longer.

Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44748
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c     |    4 +---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c2d9eae..4f690374 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1207,8 +1207,6 @@ static int i915_load_gem_init(struct drm_device *dev)
 		/* 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;
-		/* For paranoia keep the guard page in between. */
-		gtt_size -= PAGE_SIZE;
 
 		i915_gem_init_global_gtt(dev, 0, mappable_size, gtt_size);
 
@@ -1227,7 +1225,7 @@ static int i915_load_gem_init(struct drm_device *dev)
 		 * aperture.
 		 */
 		i915_gem_init_global_gtt(dev, 0, mappable_size,
-					 gtt_size - PAGE_SIZE);
+					 gtt_size);
 	}
 
 	ret = i915_gem_init_hw(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 98ed612..5a626f7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -429,7 +429,8 @@ void i915_gem_init_global_gtt(struct drm_device *dev,
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	drm_mm_init(&dev_priv->mm.gtt_space, start, end - start);
+	/* Substract the guard page ... */
+	drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - PAGE_SIZE);
 
 	dev_priv->mm.gtt_start = start;
 	dev_priv->mm.gtt_mappable_end = mappable_end;
@@ -437,6 +438,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev,
 	dev_priv->mm.gtt_total = end - start;
 	dev_priv->mm.mappable_gtt_total = min(end, mappable_end) - start;
 
-	/* Take over this portion of the GTT */
+	/* ... but ensure that we clear the entire range. */
 	intel_gtt_clear_range(start / PAGE_SIZE, (end-start) / PAGE_SIZE);
 }
-- 
1.7.8.3

Re: [PATCH 1/3] drm/i915: s/i915_gem_do_init/i915_gem_init_global_gtt

[Chris Wilson]

On Mon, 26 Mar 2012 09:45:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>  int
>  i915_gem_init_ioctl(struct drm_device *dev, void *data,
>  		    struct drm_file *file)
> @@ -155,7 +136,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	i915_gem_do_init(dev, args->gtt_start, args->gtt_end, args->gtt_end);
> +	i915_gem_init_global_gtt(dev, args->gtt_start,
> +				 args->gtt_end, args->gtt_end);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	return 0;

This raises the interesting question of ppgtt interacting with DRI1, a
never supported combination. Do we start making that expressly clear
with a few if (HAS_ALIASING_PPGTT()) return -ENODEV; or even if (gen >=
6) return -ENODEV; ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: s/i915_gem_do_init/i915_gem_init_global_gtt

[Daniel Vetter]

On Mon, Mar 26, 2012 at 09:48:01AM +0100, Chris Wilson wrote:
> On Mon, 26 Mar 2012 09:45:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >  int
> >  i915_gem_init_ioctl(struct drm_device *dev, void *data,
> >  		    struct drm_file *file)
> > @@ -155,7 +136,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data,
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > -	i915_gem_do_init(dev, args->gtt_start, args->gtt_end, args->gtt_end);
> > +	i915_gem_init_global_gtt(dev, args->gtt_start,
> > +				 args->gtt_end, args->gtt_end);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	return 0;
> 
> This raises the interesting question of ppgtt interacting with DRI1, a
> never supported combination. Do we start making that expressly clear
> with a few if (HAS_ALIASING_PPGTT()) return -ENODEV; or even if (gen >=
> 6) return -ENODEV; ?

Atm ppgtt setup is only done for the modeset case, so even if someone is
nutty enough to backport gen6 kms to ums it'll just work. But I'll plan to
rework intel-gtt some more and bail out with -EINVAL or whatever for the
ums-only ioctls on gen6+.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: s/i915_gem_do_init/i915_gem_init_global_gtt

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1100 bytes --]

On Mon, 26 Mar 2012 09:48:01 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 26 Mar 2012 09:45:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >  int
> >  i915_gem_init_ioctl(struct drm_device *dev, void *data,
> >  		    struct drm_file *file)
> > @@ -155,7 +136,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data,
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > -	i915_gem_do_init(dev, args->gtt_start, args->gtt_end, args->gtt_end);
> > +	i915_gem_init_global_gtt(dev, args->gtt_start,
> > +				 args->gtt_end, args->gtt_end);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	return 0;
> 
> This raises the interesting question of ppgtt interacting with DRI1, a
> never supported combination. Do we start making that expressly clear
> with a few if (HAS_ALIASING_PPGTT()) return -ENODEV; or even if (gen >=
> 6) return -ENODEV; ?

And do you want to tackle partial GTT init and saving of the stolen
framebuffer now that you've cleaned this up a little?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx