| Message ID | 1405675180-12374-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 18, 2014 at 02:19:40AM -0700, Rodrigo Vivi wrote: > semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings. > This optimization is to remove the ring itself from the list and the logic to do that > is at intel_ring_sync_index as below: > > /* > * 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; > */ > > v2: Skip when from == to (Damien). > v3: avoid computing idx when from == to (Damien). > use ring == to instead of ring->id == to->id (Damien). > use continue instead of return (Rodrigo). > v4: avoid all unecessary computation (Damien). > reduce idx to loop scope (Damien). > > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index 9faebbc..0b3f694 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, > struct intel_engine_cs *ring, > struct drm_i915_error_ring *ering) > { > - struct intel_engine_cs *useless; > + struct intel_engine_cs *to; > int i; > > if (!i915_semaphore_is_enabled(dev_priv->dev)) > @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, > dev_priv->semaphore_obj, > &dev_priv->gtt.base); > > - for_each_ring(useless, dev_priv, i) { > - u16 signal_offset = > - (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > - u32 *tmp = error->semaphore_obj->pages[0]; > + for_each_ring(to, dev_priv, i) { > + int idx; > + u16 signal_offset; > + u32 *tmp; > > - ering->semaphore_mboxes[i] = tmp[signal_offset]; > - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; > + if (ring == to) > + continue; > + > + signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > + tmp = error->semaphore_obj->pages[0]; > + idx = intel_ring_sync_index(ring, to); > + > + ering->semaphore_mboxes[idx] = tmp[signal_offset]; > + ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx]; > } > } > Just elaborate on previous email from your v1, now that you've fixed the error state printing, I would have rather not skip the check and instead expand the array. This would help us find stray, or unexpected writes with either future bugs, or buggy hardware. I'm not asking for a v5 (but I did ask you to make a note of what we miss in the commit message when I responded to v1... but that's okay too). I am simply explaining the earlier mail in case it was unclear. If a v5 *does* need to happen anyway, that is still my preference, but I don't care too much. (Also, I think v2 in your commit message was (Ben), not (Damien) but perhaps I missed a conversation somewhere) Reviewed-by: Ben Widawsky <ben@bwidawsk.net> P.S. I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in intel_ring_sync_index().
On Fri, Jul 18, 2014 at 07:00:40PM -0700, Ben Widawsky wrote: > On Fri, Jul 18, 2014 at 02:19:40AM -0700, Rodrigo Vivi wrote: > > semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings. > > This optimization is to remove the ring itself from the list and the logic to do that > > is at intel_ring_sync_index as below: > > > > /* > > * 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; > > */ > > > > v2: Skip when from == to (Damien). > > v3: avoid computing idx when from == to (Damien). > > use ring == to instead of ring->id == to->id (Damien). > > use continue instead of return (Rodrigo). > > v4: avoid all unecessary computation (Damien). > > reduce idx to loop scope (Damien). > > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > > index 9faebbc..0b3f694 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, > > struct intel_engine_cs *ring, > > struct drm_i915_error_ring *ering) > > { > > - struct intel_engine_cs *useless; > > + struct intel_engine_cs *to; > > int i; > > > > if (!i915_semaphore_is_enabled(dev_priv->dev)) > > @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, > > dev_priv->semaphore_obj, > > &dev_priv->gtt.base); > > > > - for_each_ring(useless, dev_priv, i) { > > - u16 signal_offset = > > - (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > > - u32 *tmp = error->semaphore_obj->pages[0]; > > + for_each_ring(to, dev_priv, i) { > > + int idx; > > + u16 signal_offset; > > + u32 *tmp; > > > > - ering->semaphore_mboxes[i] = tmp[signal_offset]; > > - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; > > + if (ring == to) > > + continue; > > + > > + signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > > + tmp = error->semaphore_obj->pages[0]; > > + idx = intel_ring_sync_index(ring, to); > > + > > + ering->semaphore_mboxes[idx] = tmp[signal_offset]; > > + ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx]; > > } > > } > > > > Just elaborate on previous email from your v1, now that you've fixed the > error state printing, I would have rather not skip the check and instead > expand the array. This would help us find stray, or unexpected writes > with either future bugs, or buggy hardware. > > I'm not asking for a v5 (but I did ask you to make a note of what we > miss in the commit message when I responded to v1... but that's okay > too). I am simply explaining the earlier mail in case it was unclear. If > a v5 *does* need to happen anyway, that is still my preference, but I > don't care too much. > > (Also, I think v2 in your commit message was (Ben), not (Damien) but > perhaps I missed a conversation somewhere) We could do this as a follow-up, merged the current version to dinq. > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > P.S. > I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in > intel_ring_sync_index(). BUG_ON considered harmful, please use WARN_ON instead. But not sure how much this is worth it here really. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 9faebbc..0b3f694 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, struct intel_engine_cs *ring, struct drm_i915_error_ring *ering) { - struct intel_engine_cs *useless; + struct intel_engine_cs *to; int i; if (!i915_semaphore_is_enabled(dev_priv->dev)) @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv, dev_priv->semaphore_obj, &dev_priv->gtt.base); - for_each_ring(useless, dev_priv, i) { - u16 signal_offset = - (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; - u32 *tmp = error->semaphore_obj->pages[0]; + for_each_ring(to, dev_priv, i) { + int idx; + u16 signal_offset; + u32 *tmp; - ering->semaphore_mboxes[i] = tmp[signal_offset]; - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; + if (ring == to) + continue; + + signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; + tmp = error->semaphore_obj->pages[0]; + idx = intel_ring_sync_index(ring, to); + + ering->semaphore_mboxes[idx] = tmp[signal_offset]; + ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx]; } }
semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings. This optimization is to remove the ring itself from the list and the logic to do that is at intel_ring_sync_index as below: /* * 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; */ v2: Skip when from == to (Damien). v3: avoid computing idx when from == to (Damien). use ring == to instead of ring->id == to->id (Damien). use continue instead of return (Rodrigo). v4: avoid all unecessary computation (Damien). reduce idx to loop scope (Damien). Cc: Damien Lespiau <damien.lespiau@intel.com> Cc: Ben Widawsky <benjamin.widawsky@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)