| Message ID | 1395134764-5599-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 18, 2014 at 10:26:04AM +0100, Daniel Vetter wrote: > Extract all this logic into a new helper function > semaphore_wait_to_signaller_ring because: > > - The current code has way too much magic. > > - The current code doesn't look at bi16, which encodes VECS signallers > on HSW. Those are just added after the fact, so can't be encoded in > a neat formula. > > - It blindly trust the hardware for the dev_priv->ring array > derefence, which given that we have a gpu hang at hand is scary. The > current logic can't blow up since it limits its value range > sufficiently, but that's a bit too tricky to rely on in my opinion. > Especially when we start to add bdw support. No, it does not blindly trust. What you just mean here is that to extend it for hsw+ it is simpler to reimplement differently. > - I'm not a big fan of the explicit ring->semaphore_register list, but > I think it's more robust to use the same mapping both when > constructing the semaphore commands and when decoding them. > > - Finally add a FIXME comment about lack of broadwell support here, > like in the earlier ipehr semaphore cmd detection function. > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Ben Widawsky <ben@bwidawsk.net> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Other than the digression above, where did MI_SEMAPHORE_SYNC_MASK spring from? Early patch? -Chris
On Wed, Mar 19, 2014 at 4:05 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Mar 18, 2014 at 10:26:04AM +0100, Daniel Vetter wrote: >> Extract all this logic into a new helper function >> semaphore_wait_to_signaller_ring because: >> >> - The current code has way too much magic. >> >> - The current code doesn't look at bi16, which encodes VECS signallers >> on HSW. Those are just added after the fact, so can't be encoded in >> a neat formula. >> >> - It blindly trust the hardware for the dev_priv->ring array >> derefence, which given that we have a gpu hang at hand is scary. The >> current logic can't blow up since it limits its value range >> sufficiently, but that's a bit too tricky to rely on in my opinion. >> Especially when we start to add bdw support. > > No, it does not blindly trust. What you just mean here is that to extend > it for hsw+ it is simpler to reimplement differently. Note that I say that the current logic can't blow up due to the limited value range ... I can drop the "blindly trust" and leave it at "too tricky" when merging. >> - I'm not a big fan of the explicit ring->semaphore_register list, but >> I think it's more robust to use the same mapping both when >> constructing the semaphore commands and when decoding them. >> >> - Finally add a FIXME comment about lack of broadwell support here, >> like in the earlier ipehr semaphore cmd detection function. >> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> Cc: Ben Widawsky <ben@bwidawsk.net> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Other than the digression above, where did MI_SEMAPHORE_SYNC_MASK spring > from? Early patch? Yeah, the two patches this one here should be in-reply-to have that #define. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 0f3a6d791502..98f95ca77246 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2543,6 +2543,39 @@ ipehr_is_semaphore_wait(struct drm_device *dev, u32 ipehr) } static struct intel_ring_buffer * +semaphore_wait_to_signaller_ring(struct intel_ring_buffer *ring, u32 ipehr) +{ + struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct intel_ring_buffer *signaller; + int i; + + if (INTEL_INFO(dev_priv->dev)->gen >= 8) { + /* + * FIXME: gen8 semaphore support - currently we don't emit + * semaphores on bdw anyway, but this needs to be addressed when + * we merge that code. + */ + return NULL; + } else { + u32 sync_bits = ipehr & MI_SEMAPHORE_SYNC_MASK; + + for_each_ring(signaller, dev_priv, i) { + if(ring == signaller) + continue; + + if (sync_bits == + signaller->semaphore_register[ring->id]) + return signaller; + } + } + + DRM_ERROR("No signaller ring found for ring %i, ipehr 0x%08x\n", + ring->id, ipehr); + + return NULL; +} + +static struct intel_ring_buffer * semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno) { struct drm_i915_private *dev_priv = ring->dev->dev_private; @@ -2582,7 +2615,7 @@ semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno) return NULL; *seqno = ioread32(ring->virtual_start + head + 4) + 1; - return &dev_priv->ring[(ring->id + (((ipehr >> 17) & 1) + 1)) % 3]; + return semaphore_wait_to_signaller_ring(ring, ipehr); } static int semaphore_passed(struct intel_ring_buffer *ring)
Extract all this logic into a new helper function semaphore_wait_to_signaller_ring because: - The current code has way too much magic. - The current code doesn't look at bi16, which encodes VECS signallers on HSW. Those are just added after the fact, so can't be encoded in a neat formula. - It blindly trust the hardware for the dev_priv->ring array derefence, which given that we have a gpu hang at hand is scary. The current logic can't blow up since it limits its value range sufficiently, but that's a bit too tricky to rely on in my opinion. Especially when we start to add bdw support. - I'm not a big fan of the explicit ring->semaphore_register list, but I think it's more robust to use the same mapping both when constructing the semaphore commands and when decoding them. - Finally add a FIXME comment about lack of broadwell support here, like in the earlier ipehr semaphore cmd detection function. Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Ben Widawsky <ben@bwidawsk.net> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)