On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > > > that's all that's needed. Note that this only blows up when we're
> > > > using the allocate_va_range funcs and not the special-purpose ones
> > > > used. With this change we can get rid of that duplication.
> > >
> > > Honestly, I would prefer the test to be rewritten so that the
> > > information on which vm was being used was passed along and not that
> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> > > what vm it bound the objects into, and yet towards the end we decide to
> > > guess again. Also, I would rather the execbuffer test be moved to
> > > i915_gem_context since it is scattering internal knowledge about.
> >
> > Yeah I agree that there's more room for cleanup. I've done this minimal
> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> > gen6/7 pt alloc code in the next patch. Since it's bogus.
>
> How about:
Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
switch logic and failed, and I wasn't fully convinced that the gen7 one
won't WARN if we actually enable full ppgtt. Given all that I decided to
go with the most minimal patch and just removed the offending bogus WARN.
But Mika/Michel promised to hang around for eventual cleanups?
-Daniel
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ed9232126644..be0f475ee1e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -638,22 +638,14 @@ mi_set_context(struct intel_engine_cs *ring,
>
> static inline bool should_skip_switch(struct intel_engine_cs *ring,
> struct intel_context *from,
> - struct intel_context *to)
> + struct intel_context *to,
> + struct i915_hw_ppgtt *ppgtt)
> {
> - struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
> if (to->remap_slice)
> return false;
>
> - if (to->ppgtt) {
> - if (from == to && !test_bit(ring->id,
> - &to->ppgtt->pd_dirty_rings))
> - return true;
> - } else if (dev_priv->mm.aliasing_ppgtt) {
> - if (from == to && !test_bit(ring->id,
> - &dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
> - return true;
> - }
> + if (from == to && !test_bit(ring->id, &ppgtt->pd_dirty_rings))
> + return true;
>
> return false;
> }
> @@ -661,15 +653,13 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
> static bool
> needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
> {
> - struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
> if (!to->ppgtt)
> return false;
>
> if (INTEL_INFO(ring->dev)->gen < 8)
> return true;
>
> - if (ring != &dev_priv->ring[RCS])
> + if (ring->id != RCS)
> return true;
>
> return false;
> @@ -679,15 +669,13 @@ static bool
> needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to,
> u32 hw_flags)
> {
> - struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
> if (!to->ppgtt)
> return false;
>
> if (!IS_GEN8(ring->dev))
> return false;
>
> - if (ring != &dev_priv->ring[RCS])
> + if (ring->id != RCS)
> return false;
>
> if (hw_flags & MI_RESTORE_INHIBIT)
> @@ -701,14 +689,15 @@ static int do_switch(struct intel_engine_cs *ring,
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
> struct intel_context *from = ring->last_context;
> + struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: dev_priv->mm.aliasing_ppgtt;
> u32 hw_flags = 0;
> bool uninitialized = false;
> int ret, i;
>
> - if (from != NULL && ring == &dev_priv->ring[RCS])
> + if (from != NULL && ring->id == RCS)
> BUG_ON(from->legacy_hw_ctx.rcs_vma == NULL);
>
> - if (should_skip_switch(ring, from, to))
> + if (should_skip_switch(ring, from, to, ppgtt))
> return 0;
>
> /* Trying to pin first makes error handling easier. */
> @@ -851,6 +840,9 @@ done:
> i915_gem_context_reference(to);
> ring->last_context = to;
>
> + WARN(ppgtt->pd_dirty_rings & (1<<ring->id),
> + "%s didn't clear reload\n", ring->name);
> +
> if (uninitialized) {
> if (ring->init_context) {
> ret = ring->init_context(ring, to);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ef644e7eaac0..595daecb3283 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1385,13 +1385,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> if (ret)
> goto error;
>
> - if (ctx->ppgtt)
> - WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
> - "%s didn't clear reload\n", ring->name);
> - else if (dev_priv->mm.aliasing_ppgtt)
> - WARN(dev_priv->mm.aliasing_ppgtt->pd_dirty_rings &
> - (1<<ring->id), "%s didn't clear reload\n", ring->name);
> -
> instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> instp_mask = I915_EXEC_CONSTANTS_MASK;
> switch (instp_mode) {
>
>
> It gets rid of the internal knowledge of address spaces from the execbuf
> code, and keeps the symmetry between handling aliasing_ppgtt and
> full_ppgtt. (The former should just be a degenerate case where the PD
> are all preloaded).
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
