[PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Ben Widawsky]

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(-)

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

[PATCH 2/2] drm/i915: Correct PPGTT total size

[Ben Widawsky]

Our code allows have a PPGTT that is smaller than the maximum size for
GEN6-GEN7. Though I don't think this actually ever occurs, the code may
as well work properly and more importantly look correct by using the
variable size instead of the HW max.

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 0f39090..68f55c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1170,7 +1170,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.start = 0;
-	ppgtt->base.total = GEN6_PPGTT_PD_ENTRIES * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
+	ppgtt->base.total =  ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
 	ppgtt->debug_dump = gen6_dump_ppgtt;
 
 	ppgtt->pd_offset =
-- 
1.9.0

Re: [PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Ben Widawsky]

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
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Correct PPGTT total size

[Chris Wilson]

On Sat, Mar 08, 2014 at 11:58:17AM -0800, Ben Widawsky wrote:
> Our code allows have a PPGTT that is smaller than the maximum size for
> GEN6-GEN7. Though I don't think this actually ever occurs, the code may
> as well work properly and more importantly look correct by using the
> variable size instead of the HW max.

Might mention that the num_pd_entries is set during allocation of the
page directories, and so here we just compute the size for the drm_mm
range manager (and the variables only exist for consistency with legacy
GGTT interfaces).
 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Ben Widawsky]

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?

Re: [PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Chris Wilson]

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

Re: [PATCH 1/2] drm/i915/bdw: Use scratch page table for GEN8 PPGTT

[Ben Widawsky]

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

Re: [PATCH 2/2] drm/i915: Correct PPGTT total size

[Daniel Vetter]

On Tue, Mar 11, 2014 at 12:21:12PM +0000, Chris Wilson wrote:
> On Sat, Mar 08, 2014 at 11:58:17AM -0800, Ben Widawsky wrote:
> > Our code allows have a PPGTT that is smaller than the maximum size for
> > GEN6-GEN7. Though I don't think this actually ever occurs, the code may
> > as well work properly and more importantly look correct by using the
> > variable size instead of the HW max.
> 
> Might mention that the num_pd_entries is set during allocation of the
> page directories, and so here we just compute the size for the drm_mm
> range manager (and the variables only exist for consistency with legacy
> GGTT interfaces).
>  
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both merged, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch