| Message ID | 1405600520-31173-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 17, 2014 at 05:35:20AM -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; > */ > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index 9faebbc..36a7960 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,14 @@ 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) { > + for_each_ring(to, dev_priv, i) { > u16 signal_offset = > (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > u32 *tmp = error->semaphore_obj->pages[0]; > + int idx = intel_ring_sync_index(ring, to); > > - ering->semaphore_mboxes[i] = tmp[signal_offset]; > - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; > + ering->semaphore_mboxes[idx] = tmp[signal_offset]; > + ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx]; > } > } > Actually, this patch does the opposite of my original intent. Since the semaphore object should have the proper spacing, my original intent was what your first patch was: Fix semaphore_seqno and semaphore_mboxes sizes (with probably a comment in the error state why we do this). That patch is simpler and gives potentially more useful information (like if we write to the wrong offset in the semaphore page, we won't see it here). UNFORTUNATELY it does not seem to correspond with how we actually print out the error state. So I think unless you want to fix up the other spots, your other patch is incorrect. This patch looks correct to me, and should fix the bug with the least amount of churn. In addition, assuming we have no bugs, it shows things more concisely in the error state. Perhaps just add my concern about missing capture certain offsets within the semaphore object in the commit message and call it good. Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
On Thu, Jul 17, 2014 at 02:31:14PM -0700, Ben Widawsky wrote: > > - for_each_ring(useless, dev_priv, i) { > > + for_each_ring(to, dev_priv, i) { > > u16 signal_offset = > > (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; > > u32 *tmp = error->semaphore_obj->pages[0]; > > + int idx = intel_ring_sync_index(ring, to); > > > > - ering->semaphore_mboxes[i] = tmp[signal_offset]; > > - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; > > + ering->semaphore_mboxes[idx] = tmp[signal_offset]; > > + ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx]; > > } > > } > > > This patch looks correct to me. We're still looping over all the rings aren't we? we'll override the array when ring == to?
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 9faebbc..36a7960 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,14 @@ 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) { + for_each_ring(to, dev_priv, i) { u16 signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4; u32 *tmp = error->semaphore_obj->pages[0]; + int idx = intel_ring_sync_index(ring, to); - ering->semaphore_mboxes[i] = tmp[signal_offset]; - ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i]; + 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; */ Cc: Ben Widawsky <benjamin.widawsky@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)