Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: > On Thu, Apr 06, 2017 at 11:44:19AM +0300, Mika Kuoppala wrote: >> Replace the handcrafter loop when checking for fifo slots >> with atomic wait for. This brings this wait in line with >> the other waits on register access. We also get a readable >> timeout constraint, so make it to fail after 10ms. >> >> Chris suggested that we should fail silently as the fifo debug >> handler, now attached to unclaimed mmio handling, will take care of the >> possible errors at later stage. >> >> Note that the decision to wait was changed so that we avoid >> allocating the first reserved entry. Nor do we reduce the count >> if we fail the wait, removing the possiblity to wrap the >> count if the hw fifo returned zero. > > Otoh, we don't abort the write so the slot is still taken. Nor does it > update the last known fifo_count along that path. > > However, we leave it set to a value that will cause us to reread the > counter on the next call, so it comes out in the wash. > >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=100247 >> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_uncore.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index a129a73..df744a8 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -29,6 +29,7 @@ >> #include <linux/pm_runtime.h> >> >> #define FORCEWAKE_ACK_TIMEOUT_MS 50 >> +#define GT_FIFO_TIMEOUT_MS 10 >> >> #define __raw_posting_read(dev_priv__, reg__) (void)__raw_i915_read32((dev_priv__), (reg__)) >> >> @@ -181,28 +182,27 @@ static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv) >> >> static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv) >> { >> - int ret = 0; >> + u32 n; >> >> /* On VLV, FIFO will be shared by both SW and HW. >> * So, we need to read the FREE_ENTRIES everytime */ >> if (IS_VALLEYVIEW(dev_priv)) >> - dev_priv->uncore.fifo_count = fifo_free_entries(dev_priv); >> - >> - if (dev_priv->uncore.fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) { >> - int loop = 500; >> - u32 fifo = fifo_free_entries(dev_priv); >> - >> - while (fifo <= GT_FIFO_NUM_RESERVED_ENTRIES && loop--) { >> - udelay(10); >> - fifo = fifo_free_entries(dev_priv); >> + n = fifo_free_entries(dev_priv); >> + else >> + n = dev_priv->uncore.fifo_count; >> + >> + if (n <= GT_FIFO_NUM_RESERVED_ENTRIES) { >> + if (wait_for_atomic((n = fifo_free_entries(dev_priv)) > >> + GT_FIFO_NUM_RESERVED_ENTRIES, >> + GT_FIFO_TIMEOUT_MS)) { >> + DRM_DEBUG("GT_FIFO timeout, entries: %u, unclaimed: %d\n", n, >> + intel_uncore_unclaimed_mmio(dev_priv)); > > What's the connection here between unclaimed mmio and the fifo count? > i.e. what information do we glean? Espcially as this information is part > of the generic mmio framework already. > To trigger fifodbg check and clean. And to get a trace from this exact spot. So this battles against the commit message to leave the warn into the unclaimed handling. I will remove it. -Mika >> + return -EBUSY; >> } >> - if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES)) >> - ++ret; >> - dev_priv->uncore.fifo_count = fifo; >> } >> - dev_priv->uncore.fifo_count--; > > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
