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

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Chris Wilson <chris@chris-wilson.co.uk>Daniel Vetter
	<daniel@ffwll.ch>, Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
Date: Fri, 17 Apr 2015 16:49:18 +0300	[thread overview]
Message-ID: <87siby27c1.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <20150415104429.GZ6092@phenom.ffwll.local>

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.

-Mika

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