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

Submitted by Chris Wilson on April 14, 2015, 5:53 p.m.

Details

Message ID 20150414175341.GM16216@nuc-i3427.alporthouse.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Chris Wilson April 14, 2015, 5:53 p.m.
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:



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

Patch hide | download patch | download mbox

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) {

Comments

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 <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
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.

No r-b as an interim solution to move this patch series forward? Without
this we'll WARN.
-Daniel
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.

No r-b on this patch as an interim solution? Without it we'll WARN_ON
unfortunately. Merged all the previous patches meanwhile, thanks for your
review.
-Daniel
Daniel Vetter <daniel@ffwll.ch> writes:

> 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.
>
> No r-b on this patch as an interim solution? Without it we'll WARN_ON
> unfortunately. Merged all the previous patches meanwhile, thanks for your
> review.

Yes we need this for now.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch