| Message ID | 1367962471-3534-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote: > +static void valleyview_setup_pctx(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *pctx; > + unsigned long pctx_paddr; > + u32 pcbr; > + int pctx_size = 24*1024; > + > + pcbr = I915_READ(VLV_PCBR); > + if (pcbr) { > + /* BIOS set it up already, grab the pre-alloc'd space */ > + int pcbr_offset; > + > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > + pcbr_offset, > + pcbr_offset, > + pctx_size); We're reserving global GTT space here for someting that just looks like it requires contiguous physical memory. This may cause issues if something else is already bound to that GTT range. I think we want to extend the interface to not reserve GTT space if gtt_offset == -1. Much happier with the placement of functions and the comments help a lot. -Chris
On Tue, 7 May 2013 22:57:37 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote: > > +static void valleyview_setup_pctx(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_i915_gem_object *pctx; > > + unsigned long pctx_paddr; > > + u32 pcbr; > > + int pctx_size = 24*1024; > > + > > + pcbr = I915_READ(VLV_PCBR); > > + if (pcbr) { > > + /* BIOS set it up already, grab the pre-alloc'd space */ > > + int pcbr_offset; > > + > > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > > + pcbr_offset, > > + pcbr_offset, > > + pctx_size); > > We're reserving global GTT space here for someting that just looks like > it requires contiguous physical memory. This may cause issues if > something else is already bound to that GTT range. I think we want to > extend the interface to not reserve GTT space if gtt_offset == -1. I should have added a comment for this too. The physical range for the power context must reside in stolen space. So while it needs contiguous physical memory, it also has a relationship to the stolen handling. I'd prefer to see a failure here than to try to do something fancy with funky GTT vs stolen setups... (Still waiting on the BIOS guys to promise the identity map going forward.)
On Tue, May 07, 2013 at 03:08:42PM -0700, Jesse Barnes wrote: > On Tue, 7 May 2013 22:57:37 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote: > > > +static void valleyview_setup_pctx(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct drm_i915_gem_object *pctx; > > > + unsigned long pctx_paddr; > > > + u32 pcbr; > > > + int pctx_size = 24*1024; > > > + > > > + pcbr = I915_READ(VLV_PCBR); > > > + if (pcbr) { > > > + /* BIOS set it up already, grab the pre-alloc'd space */ > > > + int pcbr_offset; > > > + > > > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > > > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > > > + pcbr_offset, > > > + pcbr_offset, > > > + pctx_size); > > > > We're reserving global GTT space here for someting that just looks like > > it requires contiguous physical memory. This may cause issues if > > something else is already bound to that GTT range. I think we want to > > extend the interface to not reserve GTT space if gtt_offset == -1. > > I should have added a comment for this too. The physical range for the > power context must reside in stolen space. So while it needs > contiguous physical memory, it also has a relationship to the stolen > handling. I'd prefer to see a failure here than to try to do something > fancy with funky GTT vs stolen setups... (Still waiting on the BIOS > guys to promise the identity map going forward.) Hmm, I am not understanding exactly what you need here. The register you are reading looks to be a physical address - so why do we need to also reserve a virtual address with the same offset? And if that is required, shouldn't the else branch also reserve the corresponding virtual address for its stolen allocation? -Chris
On Tue, 7 May 2013 23:14:40 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, May 07, 2013 at 03:08:42PM -0700, Jesse Barnes wrote: > > On Tue, 7 May 2013 22:57:37 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote: > > > > +static void valleyview_setup_pctx(struct drm_device *dev) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + struct drm_i915_gem_object *pctx; > > > > + unsigned long pctx_paddr; > > > > + u32 pcbr; > > > > + int pctx_size = 24*1024; > > > > + > > > > + pcbr = I915_READ(VLV_PCBR); > > > > + if (pcbr) { > > > > + /* BIOS set it up already, grab the pre-alloc'd space */ > > > > + int pcbr_offset; > > > > + > > > > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > > > > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > > > > + pcbr_offset, > > > > + pcbr_offset, > > > > + pctx_size); > > > > > > We're reserving global GTT space here for someting that just looks like > > > it requires contiguous physical memory. This may cause issues if > > > something else is already bound to that GTT range. I think we want to > > > extend the interface to not reserve GTT space if gtt_offset == -1. > > > > I should have added a comment for this too. The physical range for the > > power context must reside in stolen space. So while it needs > > contiguous physical memory, it also has a relationship to the stolen > > handling. I'd prefer to see a failure here than to try to do something > > fancy with funky GTT vs stolen setups... (Still waiting on the BIOS > > guys to promise the identity map going forward.) > > Hmm, I am not understanding exactly what you need here. The register you > are reading looks to be a physical address - so why do we need to also > reserve a virtual address with the same offset? And if that is required, > shouldn't the else branch also reserve the corresponding virtual > address for its stolen allocation? I thought the stolen create code would do that for me; on looking I see I'm wrong. I'm conflating the stolen physically contiguous range here with the first stolen_size portion of the GTT space. I don't know if anything depends on that or not, but given the way things are set up at boot time I thought it would be best not to break that assumption.
Alan, is this something your team can test since your BIOS doesn't allocate the power context? Thanks, Jesse On Tue, 7 May 2013 14:34:31 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > But we need to get the right stolen base and make pre-allocated objects > for BIOS stuff so we don't clobber it. If the BIOS hasn't allocated a > power context, we allocate one here too, from stolen space as required > by the docs. > > v2: fix stolen to phys if ladder (Ben) > keep BIOS reserved space out of allocator altogether (Ben) > v3: fix mask of stolen base (Ben) > v4: clean up preallocated object on unload (Ben) > don't zero reg on unload (Jesse) > fix mask harder (Jesse) > v5: use unref for freeing stolen bits (Chris) > move alloc/free to intel_pm.c (Chris) > v6: NULL pctx at disable time so error paths work (Ben) > > Reviewed-by: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++-- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c81100c..2fe5fd4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1073,6 +1073,8 @@ typedef struct drm_i915_private { > > struct i915_gpu_error gpu_error; > > + struct drm_i915_gem_object *vlv_pctx; > + > /* list of fbdev register on this device */ > struct intel_fbdev *fbdev; > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 67d3510..9137fa4c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -62,7 +62,10 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) > * its value of TOLUD. > */ > base = 0; > - if (INTEL_INFO(dev)->gen >= 6) { > + if (IS_VALLEYVIEW(dev)) { > + pci_read_config_dword(pdev, 0x5c, &base); > + base &= ~((1<<20) - 1); > + } else if (INTEL_INFO(dev)->gen >= 6) { > /* Read Base Data of Stolen Memory Register (BDSM) directly. > * Note that there is also a MCHBAR miror at 0x1080c0 or > * we could use device 2:0x5c instead. > @@ -183,6 +186,7 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > int i915_gem_init_stolen(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + int bios_reserved = 0; > > dev_priv->mm.stolen_base = i915_stolen_to_physical(dev); > if (dev_priv->mm.stolen_base == 0) > @@ -191,8 +195,12 @@ int i915_gem_init_stolen(struct drm_device *dev) > DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", > dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); > > + if (IS_VALLEYVIEW(dev)) > + bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ > + > /* Basic memrange allocator for stolen space */ > - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size); > + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - > + bios_reserved); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a470103..a809a56 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -700,6 +700,7 @@ > #define VLV_IIR (VLV_DISPLAY_BASE + 0x20a4) > #define VLV_IMR (VLV_DISPLAY_BASE + 0x20a8) > #define VLV_ISR (VLV_DISPLAY_BASE + 0x20ac) > +#define VLV_PCBR (VLV_DISPLAY_BASE + 0x2120) > #define I915_PIPE_CONTROL_NOTIFY_INTERRUPT (1<<18) > #define I915_DISPLAY_PORT_INTERRUPT (1<<17) > #define I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT (1<<15) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 9b3e90e..e60cd3e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2562,6 +2562,11 @@ static void valleyview_disable_rps(struct drm_device *dev) > spin_unlock_irq(&dev_priv->rps.lock); > > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > + > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } > } > > int intel_enable_rc6(const struct drm_device *dev) > @@ -2856,6 +2861,48 @@ static void vlv_rps_timer_work(struct work_struct *work) > mutex_unlock(&dev_priv->rps.hw_lock); > } > > +static void valleyview_setup_pctx(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *pctx; > + unsigned long pctx_paddr; > + u32 pcbr; > + int pctx_size = 24*1024; > + > + pcbr = I915_READ(VLV_PCBR); > + if (pcbr) { > + /* BIOS set it up already, grab the pre-alloc'd space */ > + int pcbr_offset; > + > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > + pcbr_offset, > + pcbr_offset, > + pctx_size); > + goto out; > + } > + > + /* > + * From the Gunit register HAS: > + * The Gfx driver is expected to program this register and ensure > + * proper allocation within Gfx stolen memory. For example, this > + * register should be programmed such than the PCBR range does not > + * overlap with other ranges, such as the frame buffer, protected > + * memory, or any other relevant ranges. > + */ > + pctx = i915_gem_object_create_stolen(dev, pctx_size); > + if (!pctx) { > + DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); > + return; > + } > + > + pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; > + I915_WRITE(VLV_PCBR, pctx_paddr); > + > +out: > + dev_priv->vlv_pctx = pctx; > +} > + > static void valleyview_enable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -2870,6 +2917,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > I915_WRITE(GTFIFODBG, gtfifodbg); > } > > + valleyview_setup_pctx(dev); > + > gen6_gt_force_wake_get(dev_priv); > > I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
On Tue, 7 May 2013 23:14:40 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, May 07, 2013 at 03:08:42PM -0700, Jesse Barnes wrote: > > On Tue, 7 May 2013 22:57:37 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote: > > > > +static void valleyview_setup_pctx(struct drm_device *dev) > > > > +{ > > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > > + struct drm_i915_gem_object *pctx; > > > > + unsigned long pctx_paddr; > > > > + u32 pcbr; > > > > + int pctx_size = 24*1024; > > > > + > > > > + pcbr = I915_READ(VLV_PCBR); > > > > + if (pcbr) { > > > > + /* BIOS set it up already, grab the pre-alloc'd space */ > > > > + int pcbr_offset; > > > > + > > > > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > > > > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > > > > + pcbr_offset, > > > > + pcbr_offset, > > > > + pctx_size); > > > > > > We're reserving global GTT space here for someting that just looks like > > > it requires contiguous physical memory. This may cause issues if > > > something else is already bound to that GTT range. I think we want to > > > extend the interface to not reserve GTT space if gtt_offset == -1. > > > > I should have added a comment for this too. The physical range for the > > power context must reside in stolen space. So while it needs > > contiguous physical memory, it also has a relationship to the stolen > > handling. I'd prefer to see a failure here than to try to do something > > fancy with funky GTT vs stolen setups... (Still waiting on the BIOS > > guys to promise the identity map going forward.) > > Hmm, I am not understanding exactly what you need here. The register you > are reading looks to be a physical address - so why do we need to also > reserve a virtual address with the same offset? And if that is required, > shouldn't the else branch also reserve the corresponding virtual > address for its stolen allocation? Ok ok I'm sold, at least until we find otherwise or Alan's team reports back that the GTT space is needed. Extending the interface to not reserve GTT space makes sense; I can do that as a follow up, if you're ok with the existing code.
I think we could. Joshua, do u still have access to the CrestViewHills platform with Soo Keong - I need to make some changes to our GEM and try some new code. Lets take this offline and try to get a reply back to Jesse later. ...alan -----Original Message----- From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org] Sent: Wednesday, May 08, 2013 7:19 AM To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org; Teres Alexis, Alan Previn Subject: Re: [Intel-gfx] [PATCH] drm/i915: BIOS and power context stolen mem handling for VLV v6 Alan, is this something your team can test since your BIOS doesn't allocate the power context? Thanks, Jesse On Tue, 7 May 2013 14:34:31 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > But we need to get the right stolen base and make pre-allocated > objects for BIOS stuff so we don't clobber it. If the BIOS hasn't > allocated a power context, we allocate one here too, from stolen space > as required by the docs. > > v2: fix stolen to phys if ladder (Ben) > keep BIOS reserved space out of allocator altogether (Ben) > v3: fix mask of stolen base (Ben) > v4: clean up preallocated object on unload (Ben) > don't zero reg on unload (Jesse) > fix mask harder (Jesse) > v5: use unref for freeing stolen bits (Chris) > move alloc/free to intel_pm.c (Chris) > v6: NULL pctx at disable time so error paths work (Ben) > > Reviewed-by: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++-- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++ > 4 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h index c81100c..2fe5fd4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1073,6 +1073,8 @@ typedef struct drm_i915_private { > > struct i915_gpu_error gpu_error; > > + struct drm_i915_gem_object *vlv_pctx; > + > /* list of fbdev register on this device */ > struct intel_fbdev *fbdev; > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 67d3510..9137fa4c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -62,7 +62,10 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) > * its value of TOLUD. > */ > base = 0; > - if (INTEL_INFO(dev)->gen >= 6) { > + if (IS_VALLEYVIEW(dev)) { > + pci_read_config_dword(pdev, 0x5c, &base); > + base &= ~((1<<20) - 1); > + } else if (INTEL_INFO(dev)->gen >= 6) { > /* Read Base Data of Stolen Memory Register (BDSM) directly. > * Note that there is also a MCHBAR miror at 0x1080c0 or > * we could use device 2:0x5c instead. > @@ -183,6 +186,7 @@ void i915_gem_cleanup_stolen(struct drm_device > *dev) int i915_gem_init_stolen(struct drm_device *dev) { > struct drm_i915_private *dev_priv = dev->dev_private; > + int bios_reserved = 0; > > dev_priv->mm.stolen_base = i915_stolen_to_physical(dev); > if (dev_priv->mm.stolen_base == 0) > @@ -191,8 +195,12 @@ int i915_gem_init_stolen(struct drm_device *dev) > DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", > dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); > > + if (IS_VALLEYVIEW(dev)) > + bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ > + > /* Basic memrange allocator for stolen space */ > - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size); > + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - > + bios_reserved); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h index a470103..a809a56 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -700,6 +700,7 @@ > #define VLV_IIR (VLV_DISPLAY_BASE + 0x20a4) > #define VLV_IMR (VLV_DISPLAY_BASE + 0x20a8) > #define VLV_ISR (VLV_DISPLAY_BASE + 0x20ac) > +#define VLV_PCBR (VLV_DISPLAY_BASE + 0x2120) > #define I915_PIPE_CONTROL_NOTIFY_INTERRUPT (1<<18) > #define I915_DISPLAY_PORT_INTERRUPT (1<<17) > #define I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT (1<<15) > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c index 9b3e90e..e60cd3e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2562,6 +2562,11 @@ static void valleyview_disable_rps(struct drm_device *dev) > spin_unlock_irq(&dev_priv->rps.lock); > > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > + > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } > } > > int intel_enable_rc6(const struct drm_device *dev) @@ -2856,6 > +2861,48 @@ static void vlv_rps_timer_work(struct work_struct *work) > mutex_unlock(&dev_priv->rps.hw_lock); > } > > +static void valleyview_setup_pctx(struct drm_device *dev) { > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *pctx; > + unsigned long pctx_paddr; > + u32 pcbr; > + int pctx_size = 24*1024; > + > + pcbr = I915_READ(VLV_PCBR); > + if (pcbr) { > + /* BIOS set it up already, grab the pre-alloc'd space */ > + int pcbr_offset; > + > + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; > + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, > + pcbr_offset, > + pcbr_offset, > + pctx_size); > + goto out; > + } > + > + /* > + * From the Gunit register HAS: > + * The Gfx driver is expected to program this register and ensure > + * proper allocation within Gfx stolen memory. For example, this > + * register should be programmed such than the PCBR range does not > + * overlap with other ranges, such as the frame buffer, protected > + * memory, or any other relevant ranges. > + */ > + pctx = i915_gem_object_create_stolen(dev, pctx_size); > + if (!pctx) { > + DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); > + return; > + } > + > + pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; > + I915_WRITE(VLV_PCBR, pctx_paddr); > + > +out: > + dev_priv->vlv_pctx = pctx; > +} > + > static void valleyview_enable_rps(struct drm_device *dev) { > struct drm_i915_private *dev_priv = dev->dev_private; @@ -2870,6 > +2917,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > I915_WRITE(GTFIFODBG, gtfifodbg); > } > > + valleyview_setup_pctx(dev); > + > gen6_gt_force_wake_get(dev_priv); > > I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400); -- Jesse Barnes, Intel Open Source Technology Center
On Tue, May 07, 2013 at 04:23:06PM -0700, Jesse Barnes wrote: > Ok ok I'm sold, at least until we find otherwise or Alan's team reports > back that the GTT space is needed. > > Extending the interface to not reserve GTT space makes sense; I can do > that as a follow up, if you're ok with the existing code. I'm happy with the rest of the patch - just wishing that the size was expressed in pages (6 * GTT_PAGE_SIZE) and similarly the mask ~(GTT_PAGE_SIZE-1). Bring on the change to avoid reserving GTT space and lets hope it sticks! :) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c81100c..2fe5fd4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1073,6 +1073,8 @@ typedef struct drm_i915_private { struct i915_gpu_error gpu_error; + struct drm_i915_gem_object *vlv_pctx; + /* list of fbdev register on this device */ struct intel_fbdev *fbdev; diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 67d3510..9137fa4c 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -62,7 +62,10 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) * its value of TOLUD. */ base = 0; - if (INTEL_INFO(dev)->gen >= 6) { + if (IS_VALLEYVIEW(dev)) { + pci_read_config_dword(pdev, 0x5c, &base); + base &= ~((1<<20) - 1); + } else if (INTEL_INFO(dev)->gen >= 6) { /* Read Base Data of Stolen Memory Register (BDSM) directly. * Note that there is also a MCHBAR miror at 0x1080c0 or * we could use device 2:0x5c instead. @@ -183,6 +186,7 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) int i915_gem_init_stolen(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + int bios_reserved = 0; dev_priv->mm.stolen_base = i915_stolen_to_physical(dev); if (dev_priv->mm.stolen_base == 0) @@ -191,8 +195,12 @@ int i915_gem_init_stolen(struct drm_device *dev) DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); + if (IS_VALLEYVIEW(dev)) + bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ + /* Basic memrange allocator for stolen space */ - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size); + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - + bios_reserved); return 0; } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a470103..a809a56 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -700,6 +700,7 @@ #define VLV_IIR (VLV_DISPLAY_BASE + 0x20a4) #define VLV_IMR (VLV_DISPLAY_BASE + 0x20a8) #define VLV_ISR (VLV_DISPLAY_BASE + 0x20ac) +#define VLV_PCBR (VLV_DISPLAY_BASE + 0x2120) #define I915_PIPE_CONTROL_NOTIFY_INTERRUPT (1<<18) #define I915_DISPLAY_PORT_INTERRUPT (1<<17) #define I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT (1<<15) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9b3e90e..e60cd3e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2562,6 +2562,11 @@ static void valleyview_disable_rps(struct drm_device *dev) spin_unlock_irq(&dev_priv->rps.lock); I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); + + if (dev_priv->vlv_pctx) { + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); + dev_priv->vlv_pctx = NULL; + } } int intel_enable_rc6(const struct drm_device *dev) @@ -2856,6 +2861,48 @@ static void vlv_rps_timer_work(struct work_struct *work) mutex_unlock(&dev_priv->rps.hw_lock); } +static void valleyview_setup_pctx(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *pctx; + unsigned long pctx_paddr; + u32 pcbr; + int pctx_size = 24*1024; + + pcbr = I915_READ(VLV_PCBR); + if (pcbr) { + /* BIOS set it up already, grab the pre-alloc'd space */ + int pcbr_offset; + + pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base; + pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev, + pcbr_offset, + pcbr_offset, + pctx_size); + goto out; + } + + /* + * From the Gunit register HAS: + * The Gfx driver is expected to program this register and ensure + * proper allocation within Gfx stolen memory. For example, this + * register should be programmed such than the PCBR range does not + * overlap with other ranges, such as the frame buffer, protected + * memory, or any other relevant ranges. + */ + pctx = i915_gem_object_create_stolen(dev, pctx_size); + if (!pctx) { + DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); + return; + } + + pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; + I915_WRITE(VLV_PCBR, pctx_paddr); + +out: + dev_priv->vlv_pctx = pctx; +} + static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2870,6 +2917,8 @@ static void valleyview_enable_rps(struct drm_device *dev) I915_WRITE(GTFIFODBG, gtfifodbg); } + valleyview_setup_pctx(dev); + gen6_gt_force_wake_get(dev_priv); I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
But we need to get the right stolen base and make pre-allocated objects for BIOS stuff so we don't clobber it. If the BIOS hasn't allocated a power context, we allocate one here too, from stolen space as required by the docs. v2: fix stolen to phys if ladder (Ben) keep BIOS reserved space out of allocator altogether (Ben) v3: fix mask of stolen base (Ben) v4: clean up preallocated object on unload (Ben) don't zero reg on unload (Jesse) fix mask harder (Jesse) v5: use unref for freeing stolen bits (Chris) move alloc/free to intel_pm.c (Chris) v6: NULL pctx at disable time so error paths work (Ben) Reviewed-by: Ben Widawsky <benjamin.widawsky@intel.com> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++-- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 2 deletions(-)