drm/i915/bdw: Use scratch page table for GEN8 PPGTT

PATCH In review Ben Widawsky 4 participants 9 messages Mar 8, 2014 – Mar 11, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Ben Widawsky

author

2014-03-08 19:58 UTC #0 lore raw

PATCH 1/2 drm/i915/bdw: Use scratch page table for GEN8 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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem_gtt.c +1 -1

index 5427d6d..0f39090 100644
@@ -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);

Signature · 1 line

1.9.0

Ben Widawsky

author

2014-03-08 19:58 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem_gtt.c +1 -1

index 0f39090..68f55c4 100644
@@ -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 =

Signature · 1 line

1.9.0

Chris Wilson

Reviewed-by on v1 2/2

2014-03-11 12:21 UTC #2 lore raw

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

Quoted 4 lines · 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.

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

Quoted 1 line

Signed-off-by Ben Widawsky <[email protected]>

Reviewed-by Chris Wilson <[email protected]>

-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

2014-03-11 20:06 UTC #3 lore raw

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

Quoted 6 lines · Chris Wilson

Quoted 4 lines · 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.

Quoted 7 lines

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).
Quoted 1 line
Signed-off-by Ben Widawsky <[email protected]>
Reviewed-by Chris Wilson <[email protected]>

Both merged, thanks for patches&review.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Ben Widawsky

2014-03-08 19:59 UTC #4 lore raw

In reply to Ben Widawsky

Quoted 8 lines · 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.

Wait NVM... that wasn't gen8. I can't associate this one with a bug.

Quoted 28 lines

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem_gtt.c +1 -1

index 5427d6d..0f39090 100644
@@ -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);

Signature · 1 line

1.9.0

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Chris Wilson

2014-03-11 12:24 UTC #5 lore raw

Quoted 11 lines · Ben Widawsky

Quoted 8 lines · 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.
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

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Ben Widawsky

author

2014-03-11 16:39 UTC #6 lore raw

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ben Widawsky
Quoted 8 lines · 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.

Quoted 6 lines

Quoted 1 line
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?

Chris Wilson

Reviewed-by

2014-03-11 16:47 UTC #7 lore raw

Quoted 14 lines · Ben Widawsky

Quoted 13 lines · Chris Wilson
Quoted 11 lines · Ben Widawsky
Quoted 8 lines · 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.
Wait NVM... that wasn't gen8. I can't associate this one with a bug.

Quoted 7 lines

Quoted 4 lines
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 <[email protected]>

-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Ben Widawsky

author

2014-03-11 16:56 UTC #8 lore raw

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.

Quoted 35 lines · Chris Wilson

Quoted 14 lines · Ben Widawsky
Quoted 13 lines · Chris Wilson
Quoted 11 lines · Ben Widawsky
Quoted 8 lines · 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.
Wait NVM... that wasn't gen8. I can't associate this one with a bug.
Quoted 7 lines
Quoted 4 lines
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 <[email protected]>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre