All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/bdw: Enable full PPGTT
@ 2014-04-01 15:55 Ben Widawsky
  2014-04-01 23:12 ` Ben Widawsky
  0 siblings, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2014-04-01 15:55 UTC (permalink / raw)
  To: Intel GFX

Broadwell is perfectly capable of full PPGTT. I've been using it for
some time, and seen no especially ill effects.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ab7a75e..95477f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1830,8 +1830,7 @@ struct drm_i915_cmd_table {
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
-#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) \
-				 && !IS_BROADWELL(dev))
+#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev))
 #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
 #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/bdw: Enable full PPGTT
  2014-04-01 15:55 [PATCH] drm/i915/bdw: Enable full PPGTT Ben Widawsky
@ 2014-04-01 23:12 ` Ben Widawsky
  2014-04-02  0:37   ` [PATCH 1/3] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky
  0 siblings, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2014-04-01 23:12 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: Intel GFX

On Tue, Apr 01, 2014 at 08:55:14AM -0700, Ben Widawsky wrote:
> Broadwell is perfectly capable of full PPGTT. I've been using it for
> some time, and seen no especially ill effects.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Nak. I spoke too soon. 1 context + default works fine. More than that
gets ugly.

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ab7a75e..95477f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1830,8 +1830,7 @@ struct drm_i915_cmd_table {
>  
>  #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
>  #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> -#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) \
> -				 && !IS_BROADWELL(dev))
> +#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev))
>  #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
>  #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH 1/3] drm/i915/ppgtt: Load address space after mi_set_context
  2014-04-01 23:12 ` Ben Widawsky
@ 2014-04-02  0:37   ` Ben Widawsky
  2014-04-02  0:37     ` [PATCH 2/3] drm/i915/bdw: Render ring PDP is restored with context Ben Widawsky
  2014-04-02  0:37     ` [PATCH 3/3] drm/i915/bdw: Enable full PPGTT Ben Widawsky
  0 siblings, 2 replies; 7+ messages in thread
From: Ben Widawsky @ 2014-04-02  0:37 UTC (permalink / raw)
  To: Intel GFX

On GEN8 the PDPs are saved and restored with context, which means we
must set them after the context switch has occurred. If we do not do
this, we end up saving the new PDPs for the old context.

The next patch will go one step further and actually only load the PDPs
once.

Example of a problem
LRI PDPs 1
MI_SET_CONTEXT bar
LRI_PDPs 2
MI_SET_CONTEXT foo // save PDPs 2 to bar's context
		  //  load foos PDPs
LRI PDPs 1
MI_SET_CONTEXT bar // save PDPs 1 to foo's context

It's all wacky. This should allow full PPGTT on Broadwell to work.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8fe7d72..0e8fd1e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -667,13 +667,12 @@ static int do_switch(struct intel_ring_buffer *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
-		ret = ppgtt->switch_mm(ppgtt, ring, false);
-		if (ret)
-			goto unpin_out;
-	}
-
 	if (ring != &dev_priv->ring[RCS]) {
+		if (USES_FULL_PPGTT(ring->dev)) {
+			ret = ppgtt->switch_mm(ppgtt, ring, false);
+			if (ret)
+				goto unpin_out;
+		}
 		if (from)
 			i915_gem_context_unreference(from);
 		goto done;
@@ -711,6 +710,19 @@ static int do_switch(struct intel_ring_buffer *ring,
 	if (ret)
 		goto unpin_out;
 
+	if (USES_FULL_PPGTT(ring->dev)) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		/* The hardware context switch is emitted, but we haven't
+		 * actually changed the state - so it's probably safe to bail
+		 * here. Still, let the user know something dangerous has
+		 * happened.
+		 */
+		if (ret) {
+			DRM_ERROR("Failed to change address space on context switch\n");
+			goto unpin_out;
+		}
+	}
+
 	for (i = 0; i < MAX_L3_SLICES; i++) {
 		if (!(to->remap_slice & (1<<i)))
 			continue;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 2/3] drm/i915/bdw: Render ring PDP is restored with context
  2014-04-02  0:37   ` [PATCH 1/3] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky
@ 2014-04-02  0:37     ` Ben Widawsky
  2014-04-02  0:43       ` [PATCH 2/3] [v2] " Ben Widawsky
  2014-04-02  0:37     ` [PATCH 3/3] drm/i915/bdw: Enable full PPGTT Ben Widawsky
  1 sibling, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2014-04-02  0:37 UTC (permalink / raw)
  To: Intel GFX

So let's not do it every time we switch.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0e8fd1e..743fc42 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -710,7 +710,7 @@ static int do_switch(struct intel_ring_buffer *ring,
 	if (ret)
 		goto unpin_out;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
+	if (USES_FULL_PPGTT(ring->dev) && !to->is_initialized) {
 		ret = ppgtt->switch_mm(ppgtt, ring, false);
 		/* The hardware context switch is emitted, but we haven't
 		 * actually changed the state - so it's probably safe to bail
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 3/3] drm/i915/bdw: Enable full PPGTT
  2014-04-02  0:37   ` [PATCH 1/3] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky
  2014-04-02  0:37     ` [PATCH 2/3] drm/i915/bdw: Render ring PDP is restored with context Ben Widawsky
@ 2014-04-02  0:37     ` Ben Widawsky
  1 sibling, 0 replies; 7+ messages in thread
From: Ben Widawsky @ 2014-04-02  0:37 UTC (permalink / raw)
  To: Intel GFX

Broadwell is perfectly capable of full PPGTT. I've been using it for
some time, and seen no especially ill effects.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9a8cfe0..33bc85a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1830,8 +1830,7 @@ struct drm_i915_cmd_table {
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
-#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) \
-				 && !IS_BROADWELL(dev))
+#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev))
 #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
 #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 2/3] [v2] drm/i915/bdw: Render ring PDP is restored with context
  2014-04-02  0:37     ` [PATCH 2/3] drm/i915/bdw: Render ring PDP is restored with context Ben Widawsky
@ 2014-04-02  0:43       ` Ben Widawsky
  2014-04-02  3:35         ` Ben Widawsky
  0 siblings, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2014-04-02  0:43 UTC (permalink / raw)
  To: Intel GFX

So let's not do it every time we switch.

v2: v1 skipped the switch for all gens. We still need it pre-gen8.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0e8fd1e..3b5b8ab 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -710,7 +710,8 @@ static int do_switch(struct intel_ring_buffer *ring,
 	if (ret)
 		goto unpin_out;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
+	if (USES_FULL_PPGTT(ring->dev) &&
+	    (!IS_GEN8(ring->dev) || !to->is_initialized)) {
 		ret = ppgtt->switch_mm(ppgtt, ring, false);
 		/* The hardware context switch is emitted, but we haven't
 		 * actually changed the state - so it's probably safe to bail
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] [v2] drm/i915/bdw: Render ring PDP is restored with context
  2014-04-02  0:43       ` [PATCH 2/3] [v2] " Ben Widawsky
@ 2014-04-02  3:35         ` Ben Widawsky
  0 siblings, 0 replies; 7+ messages in thread
From: Ben Widawsky @ 2014-04-02  3:35 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: Intel GFX

On Tue, Apr 01, 2014 at 05:43:06PM -0700, Ben Widawsky wrote:
> So let's not do it every time we switch.
> 
> v2: v1 skipped the switch for all gens. We still need it pre-gen8.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Please ignore this patch. I actually need to rework some stuff. 1 and 3
are still good though.

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0e8fd1e..3b5b8ab 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -710,7 +710,8 @@ static int do_switch(struct intel_ring_buffer *ring,
>  	if (ret)
>  		goto unpin_out;
>  
> -	if (USES_FULL_PPGTT(ring->dev)) {
> +	if (USES_FULL_PPGTT(ring->dev) &&
> +	    (!IS_GEN8(ring->dev) || !to->is_initialized)) {
>  		ret = ppgtt->switch_mm(ppgtt, ring, false);
>  		/* The hardware context switch is emitted, but we haven't
>  		 * actually changed the state - so it's probably safe to bail
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-04-02  3:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-01 15:55 [PATCH] drm/i915/bdw: Enable full PPGTT Ben Widawsky
2014-04-01 23:12 ` Ben Widawsky
2014-04-02  0:37   ` [PATCH 1/3] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky
2014-04-02  0:37     ` [PATCH 2/3] drm/i915/bdw: Render ring PDP is restored with context Ben Widawsky
2014-04-02  0:43       ` [PATCH 2/3] [v2] " Ben Widawsky
2014-04-02  3:35         ` Ben Widawsky
2014-04-02  0:37     ` [PATCH 3/3] drm/i915/bdw: Enable full PPGTT Ben Widawsky

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.