<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jan 26, 2016 at 12:30 AM Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jan 25, 2016 at 09:17:15PM +0000, Chris Wilson wrote:<br>
> On Mon, Jan 25, 2016 at 11:29:19AM -0800, Rodrigo Vivi wrote:<br>
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c<br>
> > @@ -764,18 +764,18 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)<br>
> >  {<br>
> >     struct intel_ringbuffer *ringbuf = request->ringbuf;<br>
> >     struct drm_i915_private *dev_priv = request->i915;<br>
> > +   int i;<br>
> ><br>
> >     intel_logical_ring_advance(ringbuf);<br>
> >     request->tail = ringbuf->tail;<br>
> ><br>
> >     /*<br>
> > -    * Here we add two extra NOOPs as padding to avoid<br>
> > +    * Here we add extra NOOPs as padding to avoid<br>
> >      * lite restore of a context with HEAD==TAIL.<br>
> > -    *<br>
> > -    * Caller must reserve WA_TAIL_DWORDS for us!<br>
> >      */<br>
> > -   intel_logical_ring_emit(ringbuf, MI_NOOP);<br>
> > -   intel_logical_ring_emit(ringbuf, MI_NOOP);<br>
> > +   for (i = 0; i < ringbuf->wa_tail_dwords; i++)<br>
> > +           intel_logical_ring_emit(ringbuf, MI_NOOP);<br>
> > +<br>
> >     intel_logical_ring_advance(ringbuf);<br>
> ><br>
> >     if (intel_ring_stopped(request->ring))<br>
> > @@ -876,6 +876,16 @@ int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)<br>
> >     if (ret)<br>
> >             return ret;<br>
> ><br>
> > +   if (IS_GEN8(req->ring->dev) || IS_GEN9(req->ring->dev))<br>
><br>
> req->i915<br>
><br>
> This is attrocious. Just allocate the extra space when required.<br>
<br></blockquote><div><br></div><div>by this logic I should just emit the mi_noops when required as well, right?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Slightly less grumpy this morning.<br></blockquote><div><br></div><div>thanks </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
1. This is duplicating the reserved-space mechanism, by open-coding the<br>
requirements for execlists. Fine-tuning the reserved space per ring may<br>
be worth it, but probably not. Over reserving space is not a hung issue<br>
(it just effectively reduces the size of the ring), and the granularity<br>
is the size of the average request.<br></blockquote><div><br></div><div>forgive this clueless mind here, but I don't see how I'm duplicating the reserved-space... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2. You are hiding how much space is actually used during request<br>
emission. This makes review impossible, and we depend upon review to<br>
verify that the intel_ring_begin() matches the number of dwords emitted.<br></blockquote><div><br></div><div>but the mi_noops are hidden on the submit and advance... shouldn't we move it back to the places that allocates it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3. Is this even the right mechanism considering the number of other ways<br>
of automatically emitting instructions between batches and contexts? We<br>
cannot answer that as this patch is out of context.<br></blockquote><div><br></div><div>yeap, sorry again, I was just going to the easiest path to be able to avoid the nulls per platform without adding 3 ifs..</div><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-Chris<br>
<br>
--<br>
Chris Wilson, Intel Open Source Technology Centre<br>
_______________________________________________<br>
Intel-gfx mailing list<br>
<a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
</blockquote></div></div>