Re: [PATCH 2/2] drm/i915: Use wait_for_atomic_us when waiting for gt fifo

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> 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@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.
>

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@chris-wilson.co.uk>
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx