Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 23, 2015 at 04:43:52PM +0100, Chris Wilson wrote:
> On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> > 
> > > 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?
> > 
> > Yes. There is more to come after this series.
> > I can include Chris's suggestion.
> 
> Looking at this WARN that fires continually on gen6/gen7, (it's nice to
> have such a blatant WARN to explain the perf decrease), don't we need
> 
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a1a9f5dad541..006d4a2610f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1562,6 +1562,8 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>                         ret = ppgtt->switch_mm(ppgtt, ring);
>                         if (ret != 0)
>                                 return ret;
> +
> +                       ppgtt->pd_dirty_rings &= ~(1 << i);
>                 }
>         }

Yeah, but I didn't really see the point in the debug infrastructure for
aliasing ppgtt. The pde loading is done somewhere completely else than for
full ppgtt. And if we forget to load the pdes the effects are obvious,
whereas failing to reload when changes happen with full ppgtt is much
harder to spot quickly. So I opted to undo those checks again, they have
been defunct before my changes anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx