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

[Chris Wilson]

On Thu, Apr 23, 2015 at 08:56:40PM +0200, Daniel Vetter wrote:
> 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 at 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.

This performance regression is still unfixed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre