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