[PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[Chris Wilson]

We have only 8bits of precise timestamps in which to complete our
upper/load reads, along with the switch between precision. This is not
always enough time to read the upper counter twice within the same time
slice, leading to hard lockups. Limit the number of times to prevent
an inifite loop (my fault for assuming we would have no trouble doing
the write + reads fast enough).

Fixes: 47c21d9a1a7b ("drm/i915: Extend vlv/chv residency resolution")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100377
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aece0ff88a5d..63ce70329e6e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8355,6 +8355,7 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv,
 			     const i915_reg_t reg)
 {
 	u32 lower, upper, tmp;
+	int loop = 2;
 
 	/* The register accessed do not need forcewake. We borrow
 	 * uncore lock to prevent concurrent access to range reg.
@@ -8383,7 +8384,7 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv,
 		I915_WRITE_FW(VLV_COUNTER_CONTROL,
 			      _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH));
 		upper = I915_READ_FW(reg);
-	} while (upper != tmp);
+	} while (upper != tmp && --loop);
 
 	/* Everywhere else we always use VLV_COUNTER_CONTROL with the
 	 * VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Limit number of reads to stabilize rc6 counter reads

[Patchwork]

== Series Details ==

Series: drm/i915: Limit number of reads to stabilize rc6 counter reads
URL   : https://patchwork.freedesktop.org/series/21843/
State : success

== Summary ==

Series 21843v1 drm/i915: Limit number of reads to stabilize rc6 counter reads
https://patchwork.freedesktop.org/api/1.0/series/21843/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 459s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 457s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 539s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 577s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 515s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 431s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 511s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 513s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 484s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 483s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 598s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 496s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 515s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 457s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 545s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 429s
fi-byt-n2820 failed to collect. IGT log at Patchwork_4297/fi-byt-n2820/igt.log

286b64375ffcd1fdae8427f6fdae7af6e043a641 drm-tip: 2017y-03m-24d-15h-59m-46s UTC integration manifest
42dcfbb drm/i915: Limit number of reads to stabilize rc6 counter reads

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4297/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We have only 8bits of precise timestamps in which to complete our
> upper/load reads, along with the switch between precision. This is not
> always enough time to read the upper counter twice within the same time
> slice, leading to hard lockups. Limit the number of times to prevent
> an inifite loop (my fault for assuming we would have no trouble doing
> the write + reads fast enough).
>

We get here only with kasan enabled? Or even without?


> Fixes: 47c21d9a1a7b ("drm/i915: Extend vlv/chv residency resolution")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100377
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aece0ff88a5d..63ce70329e6e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8355,6 +8355,7 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv,
>  			     const i915_reg_t reg)
>  {
>  	u32 lower, upper, tmp;
> +	int loop = 2;

loop could be larger, like 4 but as we very seldom
get a less than usec error, I am fine with this too.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  
>  	/* The register accessed do not need forcewake. We borrow
>  	 * uncore lock to prevent concurrent access to range reg.
> @@ -8383,7 +8384,7 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv,
>  		I915_WRITE_FW(VLV_COUNTER_CONTROL,
>  			      _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH));
>  		upper = I915_READ_FW(reg);
> -	} while (upper != tmp);
> +	} while (upper != tmp && --loop);
>  
>  	/* Everywhere else we always use VLV_COUNTER_CONTROL with the
>  	 * VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[Chris Wilson]

On Mon, Mar 27, 2017 at 01:07:58PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We have only 8bits of precise timestamps in which to complete our
> > upper/load reads, along with the switch between precision. This is not
> > always enough time to read the upper counter twice within the same time
> > slice, leading to hard lockups. Limit the number of times to prevent
> > an inifite loop (my fault for assuming we would have no trouble doing
> > the write + reads fast enough).
> >
> 
> We get here only with kasan enabled? Or even without?

It quite possibly is just a kasan artefact. Though we are chasing severe
latencies (>400us) due to mmio reads elsewhere (skl gt4e).
-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

Re: [PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Mar 27, 2017 at 01:07:58PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > We have only 8bits of precise timestamps in which to complete our
>> > upper/load reads, along with the switch between precision. This is not
>> > always enough time to read the upper counter twice within the same time
>> > slice, leading to hard lockups. Limit the number of times to prevent
>> > an inifite loop (my fault for assuming we would have no trouble doing
>> > the write + reads fast enough).
>> >
>> 
>> We get here only with kasan enabled? Or even without?
>
> It quite possibly is just a kasan artefact. Though we are chasing severe
> latencies (>400us) due to mmio reads elsewhere (skl gt4e).

And with byt, even inside local_irq_disable/enable. I read that
there has been some pstate messups in the intel_idle. I just
hope that would be remedy, haven't tested yet.

-Mika


> -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

Re: [PATCH] drm/i915: Limit number of reads to stabilize rc6 counter reads

[Chris Wilson]

On Mon, Mar 27, 2017 at 01:07:58PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We have only 8bits of precise timestamps in which to complete our
> > upper/load reads, along with the switch between precision. This is not
> > always enough time to read the upper counter twice within the same time
> > slice, leading to hard lockups. Limit the number of times to prevent
> > an inifite loop (my fault for assuming we would have no trouble doing
> > the write + reads fast enough).
> >
> 
> We get here only with kasan enabled? Or even without?
> 
> 
> > Fixes: 47c21d9a1a7b ("drm/i915: Extend vlv/chv residency resolution")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100377
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index aece0ff88a5d..63ce70329e6e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8355,6 +8355,7 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv,
> >  			     const i915_reg_t reg)
> >  {
> >  	u32 lower, upper, tmp;
> > +	int loop = 2;
> 
> loop could be larger, like 4 but as we very seldom
> get a less than usec error, I am fine with this too.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Pushed with max loops set to 2. I didn't feel the usecase merited trying
harder to be sure the read was as accurate as we could make it, beyond
the first attempt to prevent the wrap affecting the result.
-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