| Message ID | 1394308697-3683-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 08, 2014 at 11:58:16AM -0800, Ben Widawsky wrote: > I'm not clear if the hardware is still subject to the same prefetching > issues that made us use a scratch page in the first place. In either > case, we're using garbage with the current code (we will end up using > offset 0). > > This may be the cause of our current gem_cpu_reloc regression with > PPGTT. I cannot test it at the moment. > Wait NVM... that wasn't gen8. I can't associate this one with a bug. > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 5427d6d..0f39090 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1169,7 +1169,6 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > ppgtt->base.clear_range = gen6_ppgtt_clear_range; > ppgtt->base.insert_entries = gen6_ppgtt_insert_entries; > ppgtt->base.cleanup = gen6_ppgtt_cleanup; > - ppgtt->base.scratch = dev_priv->gtt.base.scratch; > ppgtt->base.start = 0; > ppgtt->base.total = GEN6_PPGTT_PD_ENTRIES * I915_PPGTT_PT_ENTRIES * PAGE_SIZE; > ppgtt->debug_dump = gen6_dump_ppgtt; > @@ -1192,6 +1191,7 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > int ret = 0; > > ppgtt->base.dev = dev; > + ppgtt->base.scratch = dev_priv->gtt.base.scratch; > > if (INTEL_INFO(dev)->gen < 8) > ret = gen6_ppgtt_init(ppgtt); > -- > 1.9.0 >
On Sat, Mar 08, 2014 at 11:59:42AM -0800, Ben Widawsky wrote: > On Sat, Mar 08, 2014 at 11:58:16AM -0800, Ben Widawsky wrote: > > I'm not clear if the hardware is still subject to the same prefetching > > issues that made us use a scratch page in the first place. In either > > case, we're using garbage with the current code (we will end up using > > offset 0). > > > > This may be the cause of our current gem_cpu_reloc regression with > > PPGTT. I cannot test it at the moment. > > > > Wait NVM... that wasn't gen8. I can't associate this one with a bug. Yeah, this doesn't appear to achieve anything. ppgtt->base.scratch is only used by ppgtt->base.clear_range() and there is no caller between i915_gem_init_ppgtt() and ppgtt->base.scratch initialisation in gen6_ppgtt_init(). -Chris
On Tue, Mar 11, 2014 at 5:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Mar 08, 2014 at 11:59:42AM -0800, Ben Widawsky wrote: >> On Sat, Mar 08, 2014 at 11:58:16AM -0800, Ben Widawsky wrote: >> > I'm not clear if the hardware is still subject to the same prefetching >> > issues that made us use a scratch page in the first place. In either >> > case, we're using garbage with the current code (we will end up using >> > offset 0). >> > >> > This may be the cause of our current gem_cpu_reloc regression with >> > PPGTT. I cannot test it at the moment. >> > >> >> Wait NVM... that wasn't gen8. I can't associate this one with a bug. > > Yeah, this doesn't appear to achieve anything. ppgtt->base.scratch is > only used by ppgtt->base.clear_range() and there is no caller between > i915_gem_init_ppgtt() and ppgtt->base.scratch initialisation in > gen6_ppgtt_init(). Still the right thing to do for gen8 though, right?
On Tue, Mar 11, 2014 at 09:39:30AM -0700, Ben Widawsky wrote: > On Tue, Mar 11, 2014 at 5:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Mar 08, 2014 at 11:59:42AM -0800, Ben Widawsky wrote: > >> On Sat, Mar 08, 2014 at 11:58:16AM -0800, Ben Widawsky wrote: > >> > I'm not clear if the hardware is still subject to the same prefetching > >> > issues that made us use a scratch page in the first place. In either > >> > case, we're using garbage with the current code (we will end up using > >> > offset 0). > >> > > >> > This may be the cause of our current gem_cpu_reloc regression with > >> > PPGTT. I cannot test it at the moment. > >> > > >> > >> Wait NVM... that wasn't gen8. I can't associate this one with a bug. > > > > Yeah, this doesn't appear to achieve anything. ppgtt->base.scratch is > > only used by ppgtt->base.clear_range() and there is no caller between > > i915_gem_init_ppgtt() and ppgtt->base.scratch initialisation in > > gen6_ppgtt_init(). > > > Still the right thing to do for gen8 though, right? Likewise vm->scratch.addr is only used by gen8_ppgtt_clear_range()... Except that it is never initialized to point to the scratch page in gen8_ppgtt_init(). So yes, wrt gen8 it is the right thing to do. There is more common code you could refactor if you so desired though... My bad for not realising this was to fix the gen8 bug, I was looking for something broken in the gen6 init sequence. So, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Way more coming in terms of sharing code. If you feel like looking into the future: http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=dynamic_pt_alloc I've hoped, and continue to hope to kill insert/clear_entires entirely. Still debugging some gen7 crap though for now. On Tue, Mar 11, 2014 at 9:46 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Mar 11, 2014 at 09:39:30AM -0700, Ben Widawsky wrote: >> On Tue, Mar 11, 2014 at 5:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Sat, Mar 08, 2014 at 11:59:42AM -0800, Ben Widawsky wrote: >> >> On Sat, Mar 08, 2014 at 11:58:16AM -0800, Ben Widawsky wrote: >> >> > I'm not clear if the hardware is still subject to the same prefetching >> >> > issues that made us use a scratch page in the first place. In either >> >> > case, we're using garbage with the current code (we will end up using >> >> > offset 0). >> >> > >> >> > This may be the cause of our current gem_cpu_reloc regression with >> >> > PPGTT. I cannot test it at the moment. >> >> > >> >> >> >> Wait NVM... that wasn't gen8. I can't associate this one with a bug. >> > >> > Yeah, this doesn't appear to achieve anything. ppgtt->base.scratch is >> > only used by ppgtt->base.clear_range() and there is no caller between >> > i915_gem_init_ppgtt() and ppgtt->base.scratch initialisation in >> > gen6_ppgtt_init(). >> >> >> Still the right thing to do for gen8 though, right? > > Likewise vm->scratch.addr is only used by gen8_ppgtt_clear_range()... > Except that it is never initialized to point to the scratch page in > gen8_ppgtt_init(). So yes, wrt gen8 it is the right thing to do. There > is more common code you could refactor if you so desired though... > > My bad for not realising this was to fix the gen8 bug, I was looking for > something broken in the gen6 init sequence. So, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 5427d6d..0f39090 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1169,7 +1169,6 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt->base.clear_range = gen6_ppgtt_clear_range; ppgtt->base.insert_entries = gen6_ppgtt_insert_entries; ppgtt->base.cleanup = gen6_ppgtt_cleanup; - ppgtt->base.scratch = dev_priv->gtt.base.scratch; ppgtt->base.start = 0; ppgtt->base.total = GEN6_PPGTT_PD_ENTRIES * I915_PPGTT_PT_ENTRIES * PAGE_SIZE; ppgtt->debug_dump = gen6_dump_ppgtt; @@ -1192,6 +1191,7 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) int ret = 0; ppgtt->base.dev = dev; + ppgtt->base.scratch = dev_priv->gtt.base.scratch; if (INTEL_INFO(dev)->gen < 8) ret = gen6_ppgtt_init(ppgtt);
I'm not clear if the hardware is still subject to the same prefetching issues that made us use a scratch page in the first place. In either case, we're using garbage with the current code (we will end up using offset 0). This may be the cause of our current gem_cpu_reloc regression with PPGTT. I cannot test it at the moment. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)