[Intel-gfx] [PATCH 2/4] drm/i915/gt: Check for arbitration after writing start seqno

[Chris Wilson]

Quoting Tvrtko Ursulin (2021-01-11 16:03:48)
> 
> On 11/01/2021 10:57, Chris Wilson wrote:
> > On the off chance that we need to arbitrate before launching the
> > payload, perform the check after we signal the request is ready to
> > start. Assuming instantaneous processing of the CS event, the request
> > will then be treated as having started when we make the decisions as to
> > how to process that CS event.
> 
> What is the wider context with this change?

Just thinking about the impact of MI_ARB_ONOFF. It's the next patch that
sparked the curiosity at that is trying to address a missed arbitration
point on the semaphore-wait miss.

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > index 2e36e0a9d8a6..9a182652a35e 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > @@ -361,19 +361,19 @@ int gen8_emit_init_breadcrumb(struct i915_request *rq)
> >       if (IS_ERR(cs))
> >               return PTR_ERR(cs);
> >   
> > +     *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
> > +     *cs++ = hwsp_offset(rq);
> > +     *cs++ = 0;
> > +     *cs++ = rq->fence.seqno - 1;
> > +
> 
> Strictly this movement even makes the existing comment (below) more correct.
> 
> >       /*
> >        * Check if we have been preempted before we even get started.
> >        *
> >        * After this point i915_request_started() reports true, even if
> >        * we get preempted and so are no longer running.
> >        */
> > -     *cs++ = MI_ARB_CHECK;
> >       *cs++ = MI_NOOP;
> > -
> > -     *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
> > -     *cs++ = hwsp_offset(rq);
> > -     *cs++ = 0;
> > -     *cs++ = rq->fence.seqno - 1;
> > +     *cs++ = MI_ARB_CHECK;
> 
> Special reason to have NOOP before MI_ARB_CHECK or would more common 
> NOOP padding at the end be suitable?

The so small it's barely a reason was to put as much distance (those
whole 6 cycles) between the store and the arbitration point.
-Chris