Re: [PATCH] drm/i915: Make wa_tail_dwords flexible for future platforms.

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

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.

Slightly less grumpy this morning.

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.

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.

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