| Message ID | 1404147071-2911-3-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 30, 2014 at 09:51:11AM -0700, Rodrigo Vivi wrote: > ring index calculation table was out of date after other rings were added, > although the formula is flexible and scale when adding new rings. > > So this patch just update the comments and add a brief explanation > why to use sync_seqno[ring index]. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++--- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f6d1238..e85c85c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > idx = intel_ring_sync_index(from, to); > > seqno = obj->last_read_seqno; > + /* Optimization: Avoid semaphore sync when we are sure we already > + * waited for an object with higher seqno */ > if (seqno <= from->semaphore.sync_seqno[idx]) > return 0; > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index e72017b..2e8b516 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring, > int idx; > > /* > - * cs -> 0 = vcs, 1 = bcs > - * vcs -> 0 = bcs, 1 = cs, > - * bcs -> 0 = cs, 1 = vcs. > + * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2; > + * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs; > + * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs; > + * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs; > + * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs; > */ I'd be a favor of dropping this table, and instead explaining the goal of the math (to save the dword) > > idx = (other - ring) - 1; I'm guessing this hunk is from your private branch? In any event, the topmost comment is a nice addition: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
On Mon, Jun 30, 2014 at 06:16:50PM -0700, Ben Widawsky wrote: > On Mon, Jun 30, 2014 at 09:51:11AM -0700, Rodrigo Vivi wrote: > > ring index calculation table was out of date after other rings were added, > > although the formula is flexible and scale when adding new rings. > > > > So this patch just update the comments and add a brief explanation > > why to use sync_seqno[ring index]. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++--- > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index f6d1238..e85c85c 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > > idx = intel_ring_sync_index(from, to); > > > > seqno = obj->last_read_seqno; > > + /* Optimization: Avoid semaphore sync when we are sure we already > > + * waited for an object with higher seqno */ > > if (seqno <= from->semaphore.sync_seqno[idx]) > > return 0; > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index e72017b..2e8b516 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring, > > int idx; > > > > /* > > - * cs -> 0 = vcs, 1 = bcs > > - * vcs -> 0 = bcs, 1 = cs, > > - * bcs -> 0 = cs, 1 = vcs. > > + * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2; > > + * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs; > > + * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs; > > + * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs; > > + * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs; > > */ > > I'd be a favor of dropping this table, and instead explaining the goal > of the math (to save the dword) tbh I don't mind either way ... > > > > idx = (other - ring) - 1; > > I'm guessing this hunk is from your private branch? Applied here without fuzz ... > In any event, the topmost comment is a nice addition: Indeed. > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f6d1238..e85c85c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, idx = intel_ring_sync_index(from, to); seqno = obj->last_read_seqno; + /* Optimization: Avoid semaphore sync when we are sure we already + * waited for an object with higher seqno */ if (seqno <= from->semaphore.sync_seqno[idx]) return 0; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index e72017b..2e8b516 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring, int idx; /* - * cs -> 0 = vcs, 1 = bcs - * vcs -> 0 = bcs, 1 = cs, - * bcs -> 0 = cs, 1 = vcs. + * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2; + * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs; + * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs; + * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs; + * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs; */ idx = (other - ring) - 1;
ring index calculation table was out of date after other rings were added, although the formula is flexible and scale when adding new rings. So this patch just update the comments and add a brief explanation why to use sync_seqno[ring index]. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-)