[Intel-gfx] [PATCH] drm/i915: Make wa_tail_dwords flexible for future platforms.

[Chris Wilson]

On Tue, Jan 26, 2016 at 01:51:19PM +0000, Rodrigo Vivi wrote:
>    On Tue, Jan 26, 2016 at 12:30 AM Chris Wilson
>    <[1]chris at chris-wilson.co.uk> wrote:
> 
>      On Mon, Jan 25, 2016 at 09:17:15PM +0000, Chris Wilson wrote:
>      > On Mon, Jan 25, 2016 at 11:29:19AM -0800, Rodrigo Vivi wrote:
>      > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>      > > @@ -764,18 +764,18 @@ intel_logical_ring_advance_and_submit(struct
>      drm_i915_gem_request *request)
>      > >  {
>      > >     struct intel_ringbuffer *ringbuf = request->ringbuf;
>      > >     struct drm_i915_private *dev_priv = request->i915;
>      > > +   int i;
>      > >
>      > >     intel_logical_ring_advance(ringbuf);
>      > >     request->tail = ringbuf->tail;
>      > >
>      > >     /*
>      > > -    * Here we add two extra NOOPs as padding to avoid
>      > > +    * Here we add extra NOOPs as padding to avoid
>      > >      * lite restore of a context with HEAD==TAIL.
>      > > -    *
>      > > -    * Caller must reserve WA_TAIL_DWORDS for us!
>      > >      */
>      > > -   intel_logical_ring_emit(ringbuf, MI_NOOP);
>      > > -   intel_logical_ring_emit(ringbuf, MI_NOOP);
>      > > +   for (i = 0; i < ringbuf->wa_tail_dwords; i++)
>      > > +           intel_logical_ring_emit(ringbuf, MI_NOOP);
>      > > +
>      > >     intel_logical_ring_advance(ringbuf);
>      > >
>      > >     if (intel_ring_stopped(request->ring))
>      > > @@ -876,6 +876,16 @@ int intel_logical_ring_begin(struct
>      drm_i915_gem_request *req, int num_dwords)
>      > >     if (ret)
>      > >             return ret;
>      > >
>      > > +   if (IS_GEN8(req->ring->dev) || IS_GEN9(req->ring->dev))
>      >
>      > req->i915
>      >
>      > This is attrocious. Just allocate the extra space when required.
> 
>    by this logic I should just emit the mi_noops when required as well,
>    right?

Yes, I didn't like the placement of the wa_tail but I went with that to
avoid the code duplication.

>      Slightly less grumpy this morning.
> 
>    thanks 
> 
>      1. This is duplicating the reserved-space mechanism, by open-coding the
>      requirements for execlists. Fine-tuning the reserved space per ring may
>      be worth it, but probably not. Over reserving space is not a hung issue
>      (it just effectively reduces the size of the ring), and the granularity
>      is the size of the average request.
> 
>    forgive this clueless mind here, but I don't see how I'm duplicating the
>    reserved-space... 

You are extending every begin by the overallocation required to emit
the tail dwords. We already extend every begin by the overallocation
required to emit the request (until we come to emit the request, where
there is no more overallocation applied).

>      2. You are hiding how much space is actually used during request
>      emission. This makes review impossible, and we depend upon review to
>      verify that the intel_ring_begin() matches the number of dwords emitted.
> 
>    but the mi_noops are hidden on the submit and advance... shouldn't we move
>    it back to the places that allocates it.

Hence why I stressed that in the comments - but it is a tail call, just
read it as one function. The important sequence is that

intel_ring_begin(count)
...
count x intel_ring_emit
...
intel_ring_advance()

is clear to the reader. Yes, this breaks that rule by replacing
intel_ring_advance() with a custom lr_ring_advance_and_submit() and
perhaps it would be clearer to add lr_ring_begin_for_submit() or
something to stress the slight discrepancy, but still make the pairing
clear.

>      3. Is this even the right mechanism considering the number of other ways
>      of automatically emitting instructions between batches and contexts? We
>      cannot answer that as this patch is out of context.
> 
>    yeap, sorry again, I was just going to the easiest path to be able to
>    avoid the nulls per platform without adding 3 ifs..
>    But I wonder if you mean on comment "1." that we can live with
>    WA_TAIL_DWORDS 2 and avoid only the NULLs when needed... Is this the case?

If you want more dwords in the add_request callback, we need to add
those to the MIN_SPACE_FOR_ADD_REQUEST. If we need to add a lot, then
making it variable seems fine - but it should just hook into the common
mechanism i.e. the minimum space should be computed during engine
initialisation and the reservation applied at i915_gem_eequest_alloc().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre