[Intel-gfx] [PATCH v6] drm/i915: Emit to ringbuffer directly

[Chris Wilson]

On Thu, Feb 09, 2017 at 11:49:28AM +0000, Tvrtko Ursulin wrote:
> 
> On 09/02/2017 10:37, Mika Kuoppala wrote:
> >Chris Wilson <chris at chris-wilson.co.uk> writes:
> >
> >>On Thu, Feb 09, 2017 at 10:00:35AM +0200, Joonas Lahtinen wrote:
> >>>I expressed my concern in the previous iteration of this series months
> >>>ago, and here goes again; Lets try to keep the writes easily greppable.
> >>>
> >>>So intel_ring_emit (or better name) could remain as a wrapper
> >>>
> >>>#define (something something)_emit(x, y) *(x)++ = (y)
> >>
> >>My concern with intel_ring_emit() remaining is that we are no longer
> >>operating on the ring. The pointer to use for emitting is retrieved from
> >>the request, so I think pointer = i915_gem_request_emit(rq, num_dwords)
> >>is what we want in the near future.
> >>
> >>I suppose if that was
> >>
> >>	ring = i915_gem_request_emit(rq, num_dwords);
> >>	intel_ring_emit(ring, blah)
> >>	intel_ring_advance(rq, ring); /* this still needs polish */
> >>
> >
> >Going through request feels right. For ring_emit
> >we could use shorter:
> >
> >cs_emit and cs_advance.
> >
> >They are rings but for users at this level the distinction
> >feels unimportant.
> >
> >Just my few bikesheds.
> 
> If the main concern is "grepability" then I am not sure that there
> is a problem with "*out++", be it out, batch, or something (just not
> ring!). There are no other such statements in the code base at the
> moment. So It would be completely unique for ring emission.
> 
> On top of that intel_ring_begin is also a current grep marker when
> looking for ring emission. Only exception is I think breadcrumb
> emission.
> 
> I can see one argument about helper being future proof, that if one
> day we decide to allow wrap or something, there would be no need to
> change it all back. It would mean having it as "out =
> intel_ring_emit(rq, out, dw)" though, so still a churny patch.
> 
> If the lasts thing is something we want to consider then OK,
> otherwise I am ambiguous whether we need a helper.
> 
> I am also not sure macro which increments the argument is that great.
> 
> 1.
> *out++ = MI_USER_INTERURPT;
> *out++ = MI_NOOP;

I am quite happy with just a normal pointer assignment, and share
concern over the simplest macro(2).

out = ring_emit(out, MI_USER_INTERRUPT);

if pushed, but the magic should never be in ring_emit itself but in the
preparation of the CS mapping for the request. I think ring_emit() is a
distraction.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre