[Intel-gfx] [PATCH 2/2] drm/i915: Use wait_for_atomic_us when waiting for gt fifo

[Chris Wilson]

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 at intel.com>
> ---
>  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.

> +			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 at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre