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@xxxxxxxxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
