Re: [Intel-gfx] [PATCH 7/7] drm/i915/execlists: Remove preempt-to-busy roundtrip delay

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Daniele Ceraolo Spurio (2020-02-12 01:08:30)
> 
> 
> On 2/10/20 12:57 PM, Chris Wilson wrote:
> > @@ -1934,6 +2002,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >   
> >                               return;
> >                       }
> > +
> > +                     last = skip_lite_restore(engine, last, &submit);
> >               }
> >       }
> >   
> > @@ -2155,10 +2225,11 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >               WRITE_ONCE(execlists->yield, -1);
> >               execlists_submit_ports(engine);
> >               set_preempt_timeout(engine);
> > -     } else {
> > +     }
> > +
> > +     if (intel_engine_has_tail_lrm(engine) || !submit)
> 
> Why do we skip here if intel_engine_has_tail_lrm is true? I see that if 
> we have work pending we either take the skip_lite_restore() or the 
> submit path above, but I can't see why we need to explicitly skip 
> re-starting the ring.

You mean if !has_lrm. We have to delay letting the RING_TAIL move past
the end of the request until the HW has acknowledged the preemption
request. This is required to avoid the ELSP submission from trying to
move the RING_TAIL backwards.

As it turns out, I can't special case has_lrm here since if we read the
new RING_TAIL before the ELSP event, we end up submitting the same
RING_TAIL again and trigging the HW bug.

> 
> >   skip_submit:
> >               ring_set_paused(engine, 0);
> > -     }
> >   }
> >   
> >   static void
> > @@ -2325,7 +2396,8 @@ static void process_csb(struct intel_engine_cs *engine)
> >   
> >                       GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
> >   
> > -                     ring_set_paused(engine, 0);
> > +                     if (!intel_engine_has_tail_lrm(engine))
> > +                             ring_set_paused(engine, 0);
> >   
> 
> here as well, although I'm assuming it has the same explanation as the 
> one above.

For has_lrm, it will have already seen the new RING_TAIL at the end of
the request regardless of the preempting ELSP.

However, as noted without only setting wa_tail in the context-image and
LRM normal tail, we end up hitting WaIdleLiteRestore and killing the HW.
Bleugh.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx