On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx> (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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
