Re: [Intel-gfx] [PATCH 13/18] drm/i915: Stop passing caller's num_dwords to engine->semaphore.signal()
On pe, 2016-07-22 at 09:30 +0100, Chris Wilson wrote: > On Fri, Jul 22, 2016 at 11:15:59AM +0300, Joonas Lahtinen wrote: > > > > On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote: > > > > > > Rather than pass in the num_dwords that the caller wishes to use after > > > the signal command packet, split the breadcrumb emission into two phases > > > and have both the signal and breadcrumb individiually acquire space on > > > the ring. This makes the interface simpler for the reader, and will > > > simplify for patches. > > > > > > Signed-off-by: Chris Wilson > > > --- > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 51 > > > ++--- > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-- > > > 2 files changed, 23 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > index 907d933d62aa..9c66745fc8d7 100644 > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > @@ -1308,10 +1308,8 @@ static void render_ring_cleanup(struct > > > intel_engine_cs *engine) > > > intel_fini_pipe_control(engine); > > > } > > > > > > -static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req, > > > - unsigned int num_dwords) > > > +static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req) > > > { > > > -#define MBOX_UPDATE_DWORDS 8 > > > struct intel_ring *signaller = signaller_req->ring; > > > struct drm_i915_private *dev_priv = signaller_req->i915; > > > struct intel_engine_cs *waiter; > > > @@ -1319,10 +1317,7 @@ static int gen8_rcs_signal(struct > > > drm_i915_gem_request *signaller_req, > > > int ret, num_rings; > > > > > > num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask); > > > - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS; > > > -#undef MBOX_UPDATE_DWORDS > > > - > > > - ret = intel_ring_begin(signaller_req, num_dwords); > > > + ret = intel_ring_begin(signaller_req, (num_rings-1) * 8); > > Magic number. Just make the defines GEN?_?CS_MBOX_UPDATE_DWORDS? > No. It is important that these are very clear as the reviewer is > required to check the number of dwords emitted. Actually noticed later in the series that now the counts are all in the same function as emitting, comments can be dismissed wrt those cases. Regards, Joonas > -Chris > -- Joonas Lahtinen Open Source Technology Center Intel Corporation ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/18] drm/i915: Stop passing caller's num_dwords to engine->semaphore.signal()
On Fri, Jul 22, 2016 at 11:15:59AM +0300, Joonas Lahtinen wrote: > On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote: > > Rather than pass in the num_dwords that the caller wishes to use after > > the signal command packet, split the breadcrumb emission into two phases > > and have both the signal and breadcrumb individiually acquire space on > > the ring. This makes the interface simpler for the reader, and will > > simplify for patches. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 51 > > ++--- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-- > > 2 files changed, 23 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > > b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index 907d933d62aa..9c66745fc8d7 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -1308,10 +1308,8 @@ static void render_ring_cleanup(struct > > intel_engine_cs *engine) > > intel_fini_pipe_control(engine); > > } > > > > -static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req, > > - unsigned int num_dwords) > > +static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req) > > { > > -#define MBOX_UPDATE_DWORDS 8 > > struct intel_ring *signaller = signaller_req->ring; > > struct drm_i915_private *dev_priv = signaller_req->i915; > > struct intel_engine_cs *waiter; > > @@ -1319,10 +1317,7 @@ static int gen8_rcs_signal(struct > > drm_i915_gem_request *signaller_req, > > int ret, num_rings; > > > > num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask); > > - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS; > > -#undef MBOX_UPDATE_DWORDS > > - > > - ret = intel_ring_begin(signaller_req, num_dwords); > > + ret = intel_ring_begin(signaller_req, (num_rings-1) * 8); > > Magic number. Just make the defines GEN?_?CS_MBOX_UPDATE_DWORDS? No. It is important that these are very clear as the reviewer is required to check the number of dwords emitted. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/18] drm/i915: Stop passing caller's num_dwords to engine->semaphore.signal()
On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote: > Rather than pass in the num_dwords that the caller wishes to use after > the signal command packet, split the breadcrumb emission into two phases > and have both the signal and breadcrumb individiually acquire space on > the ring. This makes the interface simpler for the reader, and will > simplify for patches. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 51 > ++--- > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-- > 2 files changed, 23 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 907d933d62aa..9c66745fc8d7 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1308,10 +1308,8 @@ static void render_ring_cleanup(struct intel_engine_cs > *engine) > intel_fini_pipe_control(engine); > } > > -static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req, > - unsigned int num_dwords) > +static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req) > { > -#define MBOX_UPDATE_DWORDS 8 > struct intel_ring *signaller = signaller_req->ring; > struct drm_i915_private *dev_priv = signaller_req->i915; > struct intel_engine_cs *waiter; > @@ -1319,10 +1317,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request > *signaller_req, > int ret, num_rings; > > num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask); > - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS; > -#undef MBOX_UPDATE_DWORDS > - > - ret = intel_ring_begin(signaller_req, num_dwords); > + ret = intel_ring_begin(signaller_req, (num_rings-1) * 8); Magic number. Just make the defines GEN?_?CS_MBOX_UPDATE_DWORDS? > if (ret) > return ret; > > @@ -1346,14 +1341,13 @@ static int gen8_rcs_signal(struct > drm_i915_gem_request *signaller_req, > MI_SEMAPHORE_TARGET(waiter->hw_id)); > intel_ring_emit(signaller, 0); > } > + intel_ring_advance(signaller); > > return 0; > } > > -static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req, > - unsigned int num_dwords) > +static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req) > { > -#define MBOX_UPDATE_DWORDS 6 > struct intel_ring *signaller = signaller_req->ring; > struct drm_i915_private *dev_priv = signaller_req->i915; > struct intel_engine_cs *waiter; > @@ -1361,10 +1355,7 @@ static int gen8_xcs_signal(struct drm_i915_gem_request > *signaller_req, > int ret, num_rings; > > num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask); > - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS; > -#undef MBOX_UPDATE_DWORDS > - > - ret = intel_ring_begin(signaller_req, num_dwords); > + ret = intel_ring_begin(signaller_req, (num_rings-1) * 6); Magic number, see above. > if (ret) > return ret; > > @@ -1386,12 +1377,12 @@ static int gen8_xcs_signal(struct > drm_i915_gem_request *signaller_req, > MI_SEMAPHORE_TARGET(waiter->hw_id)); > intel_ring_emit(signaller, 0); > } > + intel_ring_advance(signaller); > > return 0; > } > > -static int gen6_signal(struct drm_i915_gem_request *signaller_req, > - unsigned int num_dwords) > +static int gen6_signal(struct drm_i915_gem_request *signaller_req) > { > struct intel_ring *signaller = signaller_req->ring; > struct drm_i915_private *dev_priv = signaller_req->i915; > @@ -1399,12 +1390,8 @@ static int gen6_signal(struct drm_i915_gem_request > *signaller_req, > enum intel_engine_id id; > int ret, num_rings; > > -#define MBOX_UPDATE_DWORDS 3 > num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask); > - num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2); > -#undef MBOX_UPDATE_DWORDS > - > - ret = intel_ring_begin(signaller_req, num_dwords); > + ret = intel_ring_begin(signaller_req, round_up((num_rings-1) * 3, 2)); Magic. > if (ret) > return ret; > > @@ -1422,6 +1409,7 @@ static int gen6_signal(struct drm_i915_gem_request > *signaller_req, > /* If num_dwords was rounded, make sure the tail pointer is correct */ > if (num_rings % 2 == 0) > intel_ring_emit(signaller, MI_NOOP); > + intel_ring_advance(signaller); > > return 0; > } > @@ -1439,11 +1427,13 @@ static int gen6_emit_request(struct > drm_i915_gem_request *req) > struct intel_ring *ring = req->ring; > int ret; > > - if (req->engine->semaphore.signal) > - ret = req->engine->semaphore.signal(req, 4); > - else > - ret = intel_ring_begin(req, 4); > + if (req->engine->semaphore.signal) { > + ret = req->engine->semap