diff mbox

[03/12] drm/i915: make PDE|PTE platform specific

Message ID 1366784140-2670-4-git-send-email-ben@bwidawsk.net (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky April 24, 2013, 6:15 a.m. UTC
Accomplish this be removing the PDE count define which is (and has
always been) part of the PPGTT structure anyway. With the addition of
the gen specific init function, we can nicely tuck away the magic number
in there.

In this vain, make the PTE define less of a magic number.

The remaining code in the global gtt setup is a bit messy, but an
upcoming patch will clean that one up.

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

Comments

Jesse Barnes May 2, 2013, 9:26 p.m. UTC | #1
On Tue, 23 Apr 2013 23:15:31 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Accomplish this be removing the PDE count define which is (and has
> always been) part of the PPGTT structure anyway. With the addition of
> the gen specific init function, we can nicely tuck away the magic number
> in there.
> 
> In this vain, make the PTE define less of a magic number.

vein

> 
> The remaining code in the global gtt setup is a bit messy, but an
> upcoming patch will clean that one up.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 --
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++-----
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d5dcf7f..0a9c7cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -431,8 +431,6 @@ struct i915_gtt {
>  };
>  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
>  
> -#define I915_PPGTT_PD_ENTRIES 512
> -#define I915_PPGTT_PT_ENTRIES 1024
>  struct i915_hw_ppgtt {
>  	struct drm_device *dev;
>  	unsigned num_pd_entries;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0503f09..11a50cf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -29,6 +29,7 @@
>  #include "intel_drv.h"
>  
>  typedef uint32_t gen6_gtt_pte_t;
> +#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
>  
>  /* PPGTT stuff */
>  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
> @@ -235,10 +236,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	/* 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) - I915_PPGTT_PD_ENTRIES;
> +	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
>  
> -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> +	ppgtt->num_pd_entries = 512;
>  	ppgtt->enable = gen6_ppgtt_enable;
>  	ppgtt->clear_range = gen6_ppgtt_clear_range;
>  	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
> @@ -646,7 +646,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
>  		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> +			gtt_size -= 512 * PAGE_SIZE;
>  		}
>  
>  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> @@ -657,7 +657,7 @@ 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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> +		gtt_size += 512 * PAGE_SIZE;
>  	}
>  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
>  }

I thought we could have 1024 PDEs?  Either way, this just drops the
macro, which is fine, unless we want to increase our PDE count.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Ben Widawsky May 2, 2013, 10:49 p.m. UTC | #2
On Thu, May 02, 2013 at 02:26:17PM -0700, Jesse Barnes wrote:
> On Tue, 23 Apr 2013 23:15:31 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Accomplish this be removing the PDE count define which is (and has
> > always been) part of the PPGTT structure anyway. With the addition of
> > the gen specific init function, we can nicely tuck away the magic number
> > in there.
> > 
> > In this vain, make the PTE define less of a magic number.
> 
> vein
> 
> > 
> > The remaining code in the global gtt setup is a bit messy, but an
> > upcoming patch will clean that one up.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |  2 --
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++-----
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d5dcf7f..0a9c7cd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -431,8 +431,6 @@ struct i915_gtt {
> >  };
> >  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
> >  
> > -#define I915_PPGTT_PD_ENTRIES 512
> > -#define I915_PPGTT_PT_ENTRIES 1024
> >  struct i915_hw_ppgtt {
> >  	struct drm_device *dev;
> >  	unsigned num_pd_entries;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 0503f09..11a50cf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -29,6 +29,7 @@
> >  #include "intel_drv.h"
> >  
> >  typedef uint32_t gen6_gtt_pte_t;
> > +#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> >  
> >  /* PPGTT stuff */
> >  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
> > @@ -235,10 +236,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> >  	/* 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) - I915_PPGTT_PD_ENTRIES;
> > +	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
> >  
> > -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> > +	ppgtt->num_pd_entries = 512;
> >  	ppgtt->enable = gen6_ppgtt_enable;
> >  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> >  	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
> > @@ -646,7 +646,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
> >  		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > +			gtt_size -= 512 * PAGE_SIZE;
> >  		}
> >  
> >  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > @@ -657,7 +657,7 @@ 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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > +		gtt_size += 512 * PAGE_SIZE;
> >  	}
> >  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> >  }
> 
> I thought we could have 1024 PDEs?  Either way, this just drops the
> macro, which is fine, unless we want to increase our PDE count.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks for the review.

I think the docs must be wrong if they say this. 512PDEs are all you
need to map a 2GB address space (which is the max), and furthermore, the
DCLV which is used to indicate the size of the PDEs can only allow up to 2GB.

> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
Jesse Barnes May 2, 2013, 10:55 p.m. UTC | #3
On Thu, 2 May 2013 15:49:17 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Thu, May 02, 2013 at 02:26:17PM -0700, Jesse Barnes wrote:
> > On Tue, 23 Apr 2013 23:15:31 -0700
> > Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > > Accomplish this be removing the PDE count define which is (and has
> > > always been) part of the PPGTT structure anyway. With the addition of
> > > the gen specific init function, we can nicely tuck away the magic number
> > > in there.
> > > 
> > > In this vain, make the PTE define less of a magic number.
> > 
> > vein
> > 
> > > 
> > > The remaining code in the global gtt setup is a bit messy, but an
> > > upcoming patch will clean that one up.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h     |  2 --
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++-----
> > >  2 files changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index d5dcf7f..0a9c7cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -431,8 +431,6 @@ struct i915_gtt {
> > >  };
> > >  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
> > >  
> > > -#define I915_PPGTT_PD_ENTRIES 512
> > > -#define I915_PPGTT_PT_ENTRIES 1024
> > >  struct i915_hw_ppgtt {
> > >  	struct drm_device *dev;
> > >  	unsigned num_pd_entries;
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 0503f09..11a50cf 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -29,6 +29,7 @@
> > >  #include "intel_drv.h"
> > >  
> > >  typedef uint32_t gen6_gtt_pte_t;
> > > +#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> > >  
> > >  /* PPGTT stuff */
> > >  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
> > > @@ -235,10 +236,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> > >  	/* 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) - I915_PPGTT_PD_ENTRIES;
> > > +	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
> > >  
> > > -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> > > +	ppgtt->num_pd_entries = 512;
> > >  	ppgtt->enable = gen6_ppgtt_enable;
> > >  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> > >  	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
> > > @@ -646,7 +646,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
> > >  		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > > +			gtt_size -= 512 * PAGE_SIZE;
> > >  		}
> > >  
> > >  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > > @@ -657,7 +657,7 @@ 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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > > +		gtt_size += 512 * PAGE_SIZE;
> > >  	}
> > >  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > >  }
> > 
> > I thought we could have 1024 PDEs?  Either way, this just drops the
> > macro, which is fine, unless we want to increase our PDE count.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Thanks for the review.
> 
> I think the docs must be wrong if they say this. 512PDEs are all you
> need to map a 2GB address space (which is the max), and furthermore, the
> DCLV which is used to indicate the size of the PDEs can only allow up to 2GB.

Agreed, one of the diagrams indicates a 4k set of PDEs, but the DCLV
reg says 32 * 16 entries.  So the 512 is fine.
Daniel Vetter May 6, 2013, 9:47 a.m. UTC | #4
On Thu, May 02, 2013 at 02:26:17PM -0700, Jesse Barnes wrote:
> On Tue, 23 Apr 2013 23:15:31 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Accomplish this be removing the PDE count define which is (and has
> > always been) part of the PPGTT structure anyway. With the addition of
> > the gen specific init function, we can nicely tuck away the magic number
> > in there.
> > 
> > In this vain, make the PTE define less of a magic number.
> 
> vein
> 
> > 
> > The remaining code in the global gtt setup is a bit messy, but an
> > upcoming patch will clean that one up.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |  2 --
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++-----
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d5dcf7f..0a9c7cd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -431,8 +431,6 @@ struct i915_gtt {
> >  };
> >  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
> >  
> > -#define I915_PPGTT_PD_ENTRIES 512
> > -#define I915_PPGTT_PT_ENTRIES 1024
> >  struct i915_hw_ppgtt {
> >  	struct drm_device *dev;
> >  	unsigned num_pd_entries;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 0503f09..11a50cf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -29,6 +29,7 @@
> >  #include "intel_drv.h"
> >  
> >  typedef uint32_t gen6_gtt_pte_t;
> > +#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> >  
> >  /* PPGTT stuff */
> >  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
> > @@ -235,10 +236,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> >  	/* 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) - I915_PPGTT_PD_ENTRIES;
> > +	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
> >  
> > -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> > +	ppgtt->num_pd_entries = 512;
> >  	ppgtt->enable = gen6_ppgtt_enable;
> >  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> >  	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
> > @@ -646,7 +646,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
> >  		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > +			gtt_size -= 512 * PAGE_SIZE;
> >  		}
> >  
> >  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > @@ -657,7 +657,7 @@ 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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > +		gtt_size += 512 * PAGE_SIZE;
> >  	}
> >  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> >  }
> 
> I thought we could have 1024 PDEs?  Either way, this just drops the
> macro, which is fine, unless we want to increase our PDE count.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've written this code, so I know what the magic 512 is all about. But
since Jesse is already confused about them I don't like removing the
#define that much. Heck, we even tend to use PAGE_SIZE instead of 4096.

What about a GEN6_ prefix instead? Or if you want to make this dynamic
(iirc we can allocated PDEs in groups of 32) call it _MAX or so?

I like the PT entries part though.
-Daniel
Ben Widawsky May 8, 2013, 4:49 p.m. UTC | #5
On Mon, May 06, 2013 at 11:47:42AM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2013 at 02:26:17PM -0700, Jesse Barnes wrote:
> > On Tue, 23 Apr 2013 23:15:31 -0700
> > Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > > Accomplish this be removing the PDE count define which is (and has
> > > always been) part of the PPGTT structure anyway. With the addition of
> > > the gen specific init function, we can nicely tuck away the magic number
> > > in there.
> > > 
> > > In this vain, make the PTE define less of a magic number.
> > 
> > vein
> > 
> > > 
> > > The remaining code in the global gtt setup is a bit messy, but an
> > > upcoming patch will clean that one up.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h     |  2 --
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++-----
> > >  2 files changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index d5dcf7f..0a9c7cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -431,8 +431,6 @@ struct i915_gtt {
> > >  };
> > >  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
> > >  
> > > -#define I915_PPGTT_PD_ENTRIES 512
> > > -#define I915_PPGTT_PT_ENTRIES 1024
> > >  struct i915_hw_ppgtt {
> > >  	struct drm_device *dev;
> > >  	unsigned num_pd_entries;
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 0503f09..11a50cf 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -29,6 +29,7 @@
> > >  #include "intel_drv.h"
> > >  
> > >  typedef uint32_t gen6_gtt_pte_t;
> > > +#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> > >  
> > >  /* PPGTT stuff */
> > >  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
> > > @@ -235,10 +236,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> > >  	/* 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) - I915_PPGTT_PD_ENTRIES;
> > > +	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
> > >  
> > > -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
> > > +	ppgtt->num_pd_entries = 512;
> > >  	ppgtt->enable = gen6_ppgtt_enable;
> > >  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> > >  	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
> > > @@ -646,7 +646,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
> > >  		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > > +			gtt_size -= 512 * PAGE_SIZE;
> > >  		}
> > >  
> > >  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > > @@ -657,7 +657,7 @@ 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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> > > +		gtt_size += 512 * PAGE_SIZE;
> > >  	}
> > >  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
> > >  }
> > 
> > I thought we could have 1024 PDEs?  Either way, this just drops the
> > macro, which is fine, unless we want to increase our PDE count.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> I've written this code, so I know what the magic 512 is all about. But
> since Jesse is already confused about them I don't like removing the
> #define that much. Heck, we even tend to use PAGE_SIZE instead of 4096.
> 
> What about a GEN6_ prefix instead? Or if you want to make this dynamic
> (iirc we can allocated PDEs in groups of 32) call it _MAX or so?
> 
> I like the PT entries part though.
> -Daniel

If I just bring back #define I915_PPGTT_PD_ENTRIES 512, does this make
you sufficient happy to accept the patch?

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter May 8, 2013, 5:52 p.m. UTC | #6
On Wed, May 8, 2013 at 6:49 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> I've written this code, so I know what the magic 512 is all about. But
>> since Jesse is already confused about them I don't like removing the
>> #define that much. Heck, we even tend to use PAGE_SIZE instead of 4096.
>>
>> What about a GEN6_ prefix instead? Or if you want to make this dynamic
>> (iirc we can allocated PDEs in groups of 32) call it _MAX or so?
>>
>> I like the PT entries part though.
>> -Daniel
>
> If I just bring back #define I915_PPGTT_PD_ENTRIES 512, does this make
> you sufficient happy to accept the patch?

Yeah, maybe call it GEN6_ instead of I915_ while at it ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d5dcf7f..0a9c7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -431,8 +431,6 @@  struct i915_gtt {
 };
 #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
 
-#define I915_PPGTT_PD_ENTRIES 512
-#define I915_PPGTT_PT_ENTRIES 1024
 struct i915_hw_ppgtt {
 	struct drm_device *dev;
 	unsigned num_pd_entries;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0503f09..11a50cf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -29,6 +29,7 @@ 
 #include "intel_drv.h"
 
 typedef uint32_t gen6_gtt_pte_t;
+#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
 
 /* PPGTT stuff */
 #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
@@ -235,10 +236,9 @@  static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	/* 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) - I915_PPGTT_PD_ENTRIES;
+	first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512;
 
-	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
+	ppgtt->num_pd_entries = 512;
 	ppgtt->enable = gen6_ppgtt_enable;
 	ppgtt->clear_range = gen6_ppgtt_clear_range;
 	ppgtt->insert_entries = gen6_ppgtt_insert_entries;
@@ -646,7 +646,7 @@  void i915_gem_init_global_gtt(struct drm_device *dev)
 		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 -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
+			gtt_size -= 512 * PAGE_SIZE;
 		}
 
 		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
@@ -657,7 +657,7 @@  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 += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
+		gtt_size += 512 * PAGE_SIZE;
 	}
 	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 }