On Tue, Jan 26, 2016 at 12:30 AM Chris Wilson <chris@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?


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


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


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


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>