[PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.

PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored.  I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.

All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, "gen7 is different than gen8." I've
given up, and am adding this little bit of code to make it in sync with
the docs.

v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)

v3: Rebased after ppgtt clean-up rules, and apply only for render
ring. This is needed to boot to desktop with full ppgtt in legacy mode
(without execlists).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3)
---
 drivers/gpu/drm/i915/i915_gem_context.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a5221d8..faebbf3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -522,6 +522,8 @@ static int do_switch(struct intel_engine_cs *ring,
 	struct intel_context *from = ring->last_context;
 	u32 hw_flags = 0;
 	bool uninitialized = false;
+	bool needs_pd_load_rcs = (INTEL_INFO(ring->dev)->gen < 8) && to->ppgtt;
+	bool needs_pd_load_xcs = (ring != &dev_priv->ring[RCS]) && to->ppgtt;
 	int ret, i;
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -547,7 +549,11 @@ static int do_switch(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (to->ppgtt) {
+	if (needs_pd_load_rcs || needs_pd_load_xcs) {
+		/* Older GENs and non render rings still want the load first,
+		 * "PP_DCLV followed by PP_DIR_BASE register through Load
+		 * Register Immediate commands in Ring Buffer before submitting
+		 * a context."*/
 		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
 		if (ret)
 			goto unpin_out;
@@ -577,13 +583,34 @@ static int do_switch(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
 		hw_flags |= MI_RESTORE_INHIBIT;
+		needs_pd_load_rcs = to->ppgtt && IS_GEN8(ring->dev);
+	}
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
 		goto unpin_out;
 
+	/* GEN8 does *not* require an explicit reload if the PDPs have been
+	 * setup, and we do not wish to move them.
+	 *
+	 * XXX: If we implemented page directory eviction code, this
+	 * optimization needs to be removed.
+	 */
+	if (needs_pd_load_rcs) {
+		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
+		/* 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;
-- 
2.0.3

Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[Ville Syrjälä]

On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> The simple explanation is, the docs say to do this for GEN8. Perhaps we
> want to do this for GEN7 too, I am not certain.
> 
> PDPs are saved and restored with context. Contexts (without execlists)
> only exist on the render ring. The docs say that PDPs are not power
> context save/restored.  I've learned that this actually means something
> which SW doesn't care about. So pretend the statement doesn't exist.
> For non RCS, nothing changes.
> 
> All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
> for the initialization of the context. I do this because the docs say to
> do it, and frankly, I cannot reason why it is necessary. I've thought
> about it a lot, and tried, without success, to get a reason from design.
> The answer I got more or less says, "gen7 is different than gen8." I've
> given up, and am adding this little bit of code to make it in sync with
> the docs.
> 
> v2: Completely rewritten commit message that addresses the requests
> Ville made for v1
> Only load PDPs for initial context load (Ville)
> 
> v3: Rebased after ppgtt clean-up rules, and apply only for render
> ring. This is needed to boot to desktop with full ppgtt in legacy mode
> (without execlists).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3)
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5221d8..faebbf3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -522,6 +522,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	struct intel_context *from = ring->last_context;
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
> +	bool needs_pd_load_rcs = (INTEL_INFO(ring->dev)->gen < 8) && to->ppgtt;
> +	bool needs_pd_load_xcs = (ring != &dev_priv->ring[RCS]) && to->ppgtt;
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -547,7 +549,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = ring->last_context;
>  
> -	if (to->ppgtt) {
> +	if (needs_pd_load_rcs || needs_pd_load_xcs) {
> +		/* Older GENs and non render rings still want the load first,
> +		 * "PP_DCLV followed by PP_DIR_BASE register through Load
> +		 * Register Immediate commands in Ring Buffer before submitting
> +		 * a context."*/
>  		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>  		if (ret)
>  			goto unpin_out;
> @@ -577,13 +583,34 @@ static int do_switch(struct intel_engine_cs *ring,
>  		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
>  	}
>  
> -	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
> +	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
>  		hw_flags |= MI_RESTORE_INHIBIT;
> +		needs_pd_load_rcs = to->ppgtt && IS_GEN8(ring->dev);
> +	}
>  
>  	ret = mi_set_context(ring, to, hw_flags);
>  	if (ret)
>  		goto unpin_out;
>  
> +	/* GEN8 does *not* require an explicit reload if the PDPs have been
> +	 * setup, and we do not wish to move them.
> +	 *
> +	 * XXX: If we implemented page directory eviction code, this
> +	 * optimization needs to be removed.
> +	 */

The comment seems a bit misplaced. Would seem more appropriate around
where we derive needs_pd_load_rcs.

> +	if (needs_pd_load_rcs) {
> +		ret = to->ppgtt->switch_mm(to->ppgtt, ring);

Aren't we now loading both before _and_ after on <=gen7 (except when
uninitialized or default ctx is used)?

Maybe the variables should be called needs_pd_load_pre and
needs_pd_load_post or something? The current approach just
seems somehow confusing to me.

> +		/* 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;
> -- 
> 2.0.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[Chris Wilson]

On Fri, Sep 12, 2014 at 06:35:10PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
> > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > 
> > The simple explanation is, the docs say to do this for GEN8. Perhaps we
> > want to do this for GEN7 too, I am not certain.
> > 
> > PDPs are saved and restored with context. Contexts (without execlists)
> > only exist on the render ring. The docs say that PDPs are not power
> > context save/restored.  I've learned that this actually means something
> > which SW doesn't care about. So pretend the statement doesn't exist.
> > For non RCS, nothing changes.

Hang on. This is exactly what I was worried about... Which PDPs are
saved with context? Why doesn't this mean that we aren't overwitting
PDPs in use on other rings when RCS loads a new context?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[Michel Thierry]

On 9/12/2014 4:40 PM, Chris Wilson wrote:
> On Fri, Sep 12, 2014 at 06:35:10PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
>>> From: Ben Widawsky <benjamin.widawsky@intel.com>
>>>
>>> The simple explanation is, the docs say to do this for GEN8. Perhaps we
>>> want to do this for GEN7 too, I am not certain.
>>>
>>> PDPs are saved and restored with context. Contexts (without execlists)
>>> only exist on the render ring. The docs say that PDPs are not power
>>> context save/restored.  I've learned that this actually means something
>>> which SW doesn't care about. So pretend the statement doesn't exist.
>>> For non RCS, nothing changes.
>
> Hang on. This is exactly what I was worried about... Which PDPs are
> saved with context? Why doesn't this mean that we aren't overwitting
> PDPs in use on other rings when RCS loads a new context?
> -Chris
>

I was wondering the same when I read Ben's original comment. Could this 
be GEN8 specific?

Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context

[Ville Syrjälä]

On Fri, Sep 12, 2014 at 05:56:30PM +0100, Michel Thierry wrote:
> On 9/12/2014 4:40 PM, Chris Wilson wrote:
> > On Fri, Sep 12, 2014 at 06:35:10PM +0300, Ville Syrjälä wrote:
> >> On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
> >>> From: Ben Widawsky <benjamin.widawsky@intel.com>
> >>>
> >>> The simple explanation is, the docs say to do this for GEN8. Perhaps we
> >>> want to do this for GEN7 too, I am not certain.
> >>>
> >>> PDPs are saved and restored with context. Contexts (without execlists)
> >>> only exist on the render ring. The docs say that PDPs are not power
> >>> context save/restored.  I've learned that this actually means something
> >>> which SW doesn't care about. So pretend the statement doesn't exist.
> >>> For non RCS, nothing changes.
> >
> > Hang on. This is exactly what I was worried about... Which PDPs are
> > saved with context? Why doesn't this mean that we aren't overwitting
> > PDPs in use on other rings when RCS loads a new context?
> > -Chris
> >
> 
> I was wondering the same when I read Ben's original comment. Could this 
> be GEN8 specific?

Yes. For earlier hardware it's stated that the RCS PP_DIR registers are
saved in the power context when in ring buffer mode. For gen8 it's very
unclear from the docs.

If you look at the context image it shows the RCS PDPs in the execlist
context. But if it's saving the execlist context even in ring buffer
mode (which the earlier platforms didn't do) then it means that our
GEN8_CXT_TOTAL_SIZE is wrong since it doesn't include the execlist
context. I don't think anyone did for gen8 what I did for gen6/7
and actually look at the context after the hardware had saved it.
That's how I figured out which parts actually get saved.

As far as the other engines go, I think they must still save their
PDPs in some power context since the execlist context only contains
the RCS PDPs. Well, assuming the execlist context is really where
this stuff is saved for RCS.

-- 
Ville Syrjälä
Intel OTC

[PATCH v4] drm/i915/ppgtt: Load address space after mi_set_context

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.

PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored.  I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.

All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, "gen7 is different than gen8." I've
given up, and am adding this little bit of code to make it in sync with
the docs.

v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)

v3: Rebased after ppgtt clean-up rules, and apply only for render
ring. This is needed to boot to desktop with full ppgtt in legacy mode
(without execlists).

v4: Clean up the pre/post load logic (Ville).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3-4)
---
 drivers/gpu/drm/i915/i915_gem_context.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a5221d8..7b73b36 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -522,6 +522,9 @@ static int do_switch(struct intel_engine_cs *ring,
 	struct intel_context *from = ring->last_context;
 	u32 hw_flags = 0;
 	bool uninitialized = false;
+	bool needs_pd_load_pre = ((INTEL_INFO(ring->dev)->gen < 8) ||
+			(ring != &dev_priv->ring[RCS])) && to->ppgtt;
+	bool needs_pd_load_post = false;
 	int ret, i;
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -547,7 +550,11 @@ static int do_switch(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (to->ppgtt) {
+	if (needs_pd_load_pre) {
+		/* Older GENs and non render rings still want the load first,
+		 * "PP_DCLV followed by PP_DIR_BASE register through Load
+		 * Register Immediate commands in Ring Buffer before submitting
+		 * a context."*/
 		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
 		if (ret)
 			goto unpin_out;
@@ -577,13 +584,34 @@ static int do_switch(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+	/* GEN8 does *not* require an explicit reload if the PDPs have been
+	 * setup, and we do not wish to move them.
+	 *
+	 * XXX: If we implemented page directory eviction code, this
+	 * optimization needs to be removed.
+	 */
+	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
 		hw_flags |= MI_RESTORE_INHIBIT;
+		needs_pd_load_post = to->ppgtt && IS_GEN8(ring->dev);
+	}
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
 		goto unpin_out;
 
+	if (needs_pd_load_post) {
+		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
+		/* 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;
-- 
2.0.3

Re: [PATCH v4] drm/i915/ppgtt: Load address space after mi_set_context

[Chris Wilson]

On Mon, Sep 15, 2014 at 10:29:47AM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> The simple explanation is, the docs say to do this for GEN8. Perhaps we
> want to do this for GEN7 too, I am not certain.
> 
> PDPs are saved and restored with context. Contexts (without execlists)
> only exist on the render ring. The docs say that PDPs are not power
> context save/restored.  I've learned that this actually means something
> which SW doesn't care about. So pretend the statement doesn't exist.
> For non RCS, nothing changes.
> 
> All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
> for the initialization of the context. I do this because the docs say to
> do it, and frankly, I cannot reason why it is necessary. I've thought
> about it a lot, and tried, without success, to get a reason from design.
> The answer I got more or less says, "gen7 is different than gen8." I've
> given up, and am adding this little bit of code to make it in sync with
> the docs.
> 
> v2: Completely rewritten commit message that addresses the requests
> Ville made for v1
> Only load PDPs for initial context load (Ville)
> 
> v3: Rebased after ppgtt clean-up rules, and apply only for render
> ring. This is needed to boot to desktop with full ppgtt in legacy mode
> (without execlists).
> 
> v4: Clean up the pre/post load logic (Ville).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3-4)
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5221d8..7b73b36 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -522,6 +522,9 @@ static int do_switch(struct intel_engine_cs *ring,
>  	struct intel_context *from = ring->last_context;
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
> +	bool needs_pd_load_pre = ((INTEL_INFO(ring->dev)->gen < 8) ||
> +			(ring != &dev_priv->ring[RCS])) && to->ppgtt;
> +	bool needs_pd_load_post = false;
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -547,7 +550,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = ring->last_context;
>  
> -	if (to->ppgtt) {
> +	if (needs_pd_load_pre) {
> +		/* Older GENs and non render rings still want the load first,
> +		 * "PP_DCLV followed by PP_DIR_BASE register through Load
> +		 * Register Immediate commands in Ring Buffer before submitting
> +		 * a context."*/

What's the reference for this? It works if you load the ppgtt after the
set_context on ivb/byt/hsw, so do you have a specific recommendation in
mind? And the set-context even provides the barrier required for the lri.

>  		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>  		if (ret)
>  			goto unpin_out;
> @@ -577,13 +584,34 @@ static int do_switch(struct intel_engine_cs *ring,
>  		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
>  	}
>  
> -	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
> +	/* GEN8 does *not* require an explicit reload if the PDPs have been
> +	 * setup, and we do not wish to move them.
> +	 *
> +	 * XXX: If we implemented page directory eviction code, this
> +	 * optimization needs to be removed.
> +	 */

What is the cost of the pde invalidate on top of the context restore
anyway? As this will be a secondary path in future, is optimizing
worthwhile at all?

> +	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
>  		hw_flags |= MI_RESTORE_INHIBIT;
> +		needs_pd_load_post = to->ppgtt && IS_GEN8(ring->dev);
> +	}
>  
>  	ret = mi_set_context(ring, to, hw_flags);
>  	if (ret)
>  		goto unpin_out;
>  
> +	if (needs_pd_load_post) {
> +		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
> +		/* 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.
> +		 */

Imagine if we only had patches posted already to fix all of this up.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4] drm/i915/ppgtt: Load address space after mi_set_context

[Zhi Wang]

Hi Michel:
     I'm doing a task which require LRIs to load root pointer around RCS 
context switch.My BDW CPU is C stepping. And what I found is 
MI_SET_CONTEXT instruction will save/restore PDP0/1/2/3. It's weired 
that as part of EXECLIST context, the "EXECLIST PPGTT base" context also 
appear in the dump generated by MI_SET_CONTEXT.

And I found LRI after MI_SET_CONTEXT will make ring not idle: 0x8000[0] 
!= 1. By the way, if MI_SET_CONTEXT is issued very frequently. I also 
found that will make ring not idle: INSTDONE register shows VFE and TSG 
not done.

Do you met these issues? Thanks.

Thanks,
Zhi.

> -------- Original Message --------
> Subject: [Intel-gfx] [PATCH v4] drm/i915/ppgtt: Load address space after
> mi_set_context
> Date: Mon, 15 Sep 2014 10:29:47 +0100
> From: Michel Thierry <michel.thierry@intel.com>
> To: <intel-gfx@lists.freedesktop.org>
>
> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> The simple explanation is, the docs say to do this for GEN8. Perhaps we
> want to do this for GEN7 too, I am not certain.
>
> PDPs are saved and restored with context. Contexts (without execlists)
> only exist on the render ring. The docs say that PDPs are not power
> context save/restored.  I've learned that this actually means something
> which SW doesn't care about. So pretend the statement doesn't exist.
> For non RCS, nothing changes.
>
> All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
> for the initialization of the context. I do this because the docs say to
> do it, and frankly, I cannot reason why it is necessary. I've thought
> about it a lot, and tried, without success, to get a reason from design.
> The answer I got more or less says, "gen7 is different than gen8." I've
> given up, and am adding this little bit of code to make it in sync with
> the docs.
>
> v2: Completely rewritten commit message that addresses the requests
> Ville made for v1
> Only load PDPs for initial context load (Ville)
>
> v3: Rebased after ppgtt clean-up rules, and apply only for render
> ring. This is needed to boot to desktop with full ppgtt in legacy mode
> (without execlists).
>
> v4: Clean up the pre/post load logic (Ville).
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3-4)
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 32
> ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5221d8..7b73b36 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -522,6 +522,9 @@ static int do_switch(struct intel_engine_cs *ring,
>       struct intel_context *from = ring->last_context;
>       u32 hw_flags = 0;
>       bool uninitialized = false;
> +    bool needs_pd_load_pre = ((INTEL_INFO(ring->dev)->gen < 8) ||
> +            (ring != &dev_priv->ring[RCS])) && to->ppgtt;
> +    bool needs_pd_load_post = false;
>       int ret, i;
>
>       if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -547,7 +550,11 @@ static int do_switch(struct intel_engine_cs *ring,
>        */
>       from = ring->last_context;
>
> -    if (to->ppgtt) {
> +    if (needs_pd_load_pre) {
> +        /* Older GENs and non render rings still want the load first,
> +         * "PP_DCLV followed by PP_DIR_BASE register through Load
> +         * Register Immediate commands in Ring Buffer before submitting
> +         * a context."*/
>           ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>           if (ret)
>               goto unpin_out;
> @@ -577,13 +584,34 @@ static int do_switch(struct intel_engine_cs *ring,
>           vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level,
> GLOBAL_BIND);
>       }
>
> -    if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
> +    /* GEN8 does *not* require an explicit reload if the PDPs have been
> +     * setup, and we do not wish to move them.
> +     *
> +     * XXX: If we implemented page directory eviction code, this
> +     * optimization needs to be removed.
> +     */
> +    if (!to->legacy_hw_ctx.initialized ||
> i915_gem_context_is_default(to)) {
>           hw_flags |= MI_RESTORE_INHIBIT;
> +        needs_pd_load_post = to->ppgtt && IS_GEN8(ring->dev);
> +    }
>
>       ret = mi_set_context(ring, to, hw_flags);
>       if (ret)
>           goto unpin_out;
>
> +    if (needs_pd_load_post) {
> +        ret = to->ppgtt->switch_mm(to->ppgtt, ring);
> +        /* 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;