[PATCH 0/4] drm/i915: Avoid early timeout due to wait_for_atomic

[PATCH 0/4] drm/i915: Avoid early timeout due to wait_for_atomic

[Imre Deak]

If called from non-atomic context wait_for_atomic{,_us} can fail even
though the given condition becomes true before the timeout passes. I
noticed this via sporadic timeouts during PLL locking on BXT (fixed by
patch 1). The WARN in wait_for_atomic would be also triggered, alas I
didn't have extra debugging enabled.

host2guc_action() has the same issue. I left that as-is since it seems
to be on a critical path where the overhead of the jiffies based
wait_for can be considerable. Due to the relatively long 10ms timeout
there this doesn't lead to early timeouts in any case.

CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Imre Deak (4):
  drm/i915/bxt: Avoid early timeout during PLL enable
  drm/i915/lpt: Avoid early timeout during FDI PHY reset
  drm/i915/hsw: Avoid early timeout during LCPLL disable/restore
  drm/i915: Avoid early timeout during AUX transfers

 drivers/gpu/drm/i915/intel_display.c  | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_dp.c       |  2 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.c |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.5.0

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

[PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Imre Deak]

Since wait_for_atomic doesn't re-check the wait-for condition after
expiry of the timeout it can fail when called from non-atomic context
even if the condition is set correctly before the expiry. Fix this by
using the non-atomic wait_for instead.

I noticed this via the PLL locking timing out incorrectly, with this fix
I couldn't reproduce the problem.

Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index c0eff15..e130c3e 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
 	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
 
-	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
-			PORT_PLL_LOCK), 200))
+	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
+			200))
 		DRM_ERROR("PLL %d not locked\n", port);
 
 	/*
-- 
2.5.0

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

[PATCH 2/4] drm/i915/lpt: Avoid early timeout during FDI PHY reset

[Imre Deak]

Since wait_for_atomic doesn't re-check the wait-for condition after
expiry of the timeout it can fail when called from non-atomic context
even if the condition is set correctly before the expiry. Fix this by
using the non-atomic wait_for instead.

Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3b5dc8..0312472 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8537,16 +8537,16 @@ static void lpt_reset_fdi_mphy(struct drm_i915_private *dev_priv)
 	tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
 	I915_WRITE(SOUTH_CHICKEN2, tmp);
 
-	if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
-			       FDI_MPHY_IOSFSB_RESET_STATUS, 100))
+	if (wait_for_us(I915_READ(SOUTH_CHICKEN2) &
+			FDI_MPHY_IOSFSB_RESET_STATUS, 100))
 		DRM_ERROR("FDI mPHY reset assert timeout\n");
 
 	tmp = I915_READ(SOUTH_CHICKEN2);
 	tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
 	I915_WRITE(SOUTH_CHICKEN2, tmp);
 
-	if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
-				FDI_MPHY_IOSFSB_RESET_STATUS) == 0, 100))
+	if (wait_for_us((I915_READ(SOUTH_CHICKEN2) &
+			 FDI_MPHY_IOSFSB_RESET_STATUS) == 0, 100))
 		DRM_ERROR("FDI mPHY reset de-assert timeout\n");
 }
 
-- 
2.5.0

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

[PATCH 3/4] drm/i915/hsw: Avoid early timeout during LCPLL disable/restore

[Imre Deak]

Since wait_for_atomic doesn't re-check the wait-for condition after
expiry of the timeout it can fail when called from non-atomic context
even if the condition is set correctly before the expiry. Fix this by
using the non-atomic wait_for instead.

Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0312472..d902a70 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9534,8 +9534,8 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 		val |= LCPLL_CD_SOURCE_FCLK;
 		I915_WRITE(LCPLL_CTL, val);
 
-		if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
-				       LCPLL_CD_SOURCE_FCLK_DONE, 1))
+		if (wait_for_us(I915_READ(LCPLL_CTL) &
+				LCPLL_CD_SOURCE_FCLK_DONE, 1))
 			DRM_ERROR("Switching to FCLK failed\n");
 
 		val = I915_READ(LCPLL_CTL);
@@ -9608,8 +9608,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		val &= ~LCPLL_CD_SOURCE_FCLK;
 		I915_WRITE(LCPLL_CTL, val);
 
-		if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
-					LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
+		if (wait_for_us((I915_READ(LCPLL_CTL) &
+				 LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
 
-- 
2.5.0

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

[PATCH 4/4] drm/i915: Avoid early timeout during AUX transfers

[Imre Deak]

Since wait_for_atomic doesn't re-check the wait-for condition after
expiry of the timeout it can fail when called from non-atomic context
even if the condition is set correctly before the expiry. Fix this by
using the non-atomic wait_for instead.

Due to the relatively long 10ms timeout, probably this didn't cause any
real problems, but fix it in any case for consistency.

Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c3742a0..6d586b7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -727,7 +727,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
 					  msecs_to_jiffies_timeout(10));
 	else
-		done = wait_for_atomic(C, 10) == 0;
+		done = wait_for(C, 10) == 0;
 	if (!done)
 		DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
 			  has_aux_irq);
-- 
2.5.0

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Chris Wilson]

On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.

wait_for_atomic is indeed only safe to be called from atomic context.
Likewise, wait_for is only safe to called from !atomic context.

> I noticed this via the PLL locking timing out incorrectly, with this fix
> I couldn't reproduce the problem.
> 
> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")

The bug would be using wait_for_atomic from non-atomic context, and so
older.


> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c0eff15..e130c3e 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
>  	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
>  
> -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
> -			PORT_PLL_LOCK), 200))
> +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
> +			200))

Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?
-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 2/4] drm/i915/lpt: Avoid early timeout during FDI PHY reset

[Chris Wilson]

On Tue, Jun 28, 2016 at 01:37:31PM +0300, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.

Same point as patch 1. If the bug is using the wrong function, ...
The only question is whether you do have an atomic context, in which
case we need to reinspect the notion of "there is only at most 3
non-preemptible instructions between the COND and the timeout"...
-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 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Imre Deak]

On ti, 2016-06-28 at 11:48 +0100, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
> > Since wait_for_atomic doesn't re-check the wait-for condition after
> > expiry of the timeout it can fail when called from non-atomic
> > context
> > even if the condition is set correctly before the expiry. Fix this
> > by
> > using the non-atomic wait_for instead.
> 
> wait_for_atomic is indeed only safe to be called from atomic context.
> Likewise, wait_for is only safe to called from !atomic context.
> 
> > I noticed this via the PLL locking timing out incorrectly, with
> > this fix
> > I couldn't reproduce the problem.
> > 
> > Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout
> > granularity")
> 
> The bug would be using wait_for_atomic from non-atomic context, and
> so older.

I agree that calling wait_for_atomic() wasn't correct even before, but
only because of busy waiting when we could just sleep. The condition
was rechecked after expiry so the function didn't fail in the above
case.

> 
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index c0eff15..e130c3e 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >  	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
> >  	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
> >  
> > -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
> > -			PORT_PLL_LOCK), 200))
> > +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
> > +			200))
> 
> Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?

Yes, I have CONFIG_DEBUG_ATOMIC_SLEEP enabled and AFAICS
CONFIG_I915_DEBUG shouldn't matter for the changed code. I'll enable
now also the latter although it'll trigger for the GuC path at least.

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

✓ Ro.CI.BAT: success for drm/i915: Avoid early timeout due to wait_for_atomic

[Patchwork]

== Series Details ==

Series: drm/i915: Avoid early timeout due to wait_for_atomic
URL   : https://patchwork.freedesktop.org/series/9224/
State : success

== Summary ==

Series 9224v1 drm/i915: Avoid early timeout due to wait_for_atomic
http://patchwork.freedesktop.org/api/1.0/series/9224/revisions/1/mbox


fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-kbl-qkkr      total:83   pass:62   dwarn:1   dfail:0   fail:0   skip:19 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:4   dfail:1   fail:2   skip:20 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1317/

13bc84e drm-intel-nightly: 2016y-06m-27d-21h-27m-04s UTC integration manifest
3e235ec drm/i915: Avoid early timeout during AUX transfers
45c3bee drm/i915/hsw: Avoid early timeout during LCPLL disable/restore
c6850bf drm/i915/lpt: Avoid early timeout during FDI PHY reset
bbf86d1 drm/i915/bxt: Avoid early timeout during PLL enable

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

Re: [PATCH 2/4] drm/i915/lpt: Avoid early timeout during FDI PHY reset

[Imre Deak]

On ti, 2016-06-28 at 11:50 +0100, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 01:37:31PM +0300, Imre Deak wrote:
> > Since wait_for_atomic doesn't re-check the wait-for condition after
> > expiry of the timeout it can fail when called from non-atomic
> > context
> > even if the condition is set correctly before the expiry. Fix this
> > by
> > using the non-atomic wait_for instead.
> 
> Same point as patch 1. If the bug is using the wrong function, ...
> The only question is whether you do have an atomic context, in which
> case we need to reinspect the notion of "there is only at most 3
> non-preemptible instructions between the COND and the timeout"...

These functions are only called from non-atomic context.

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Tvrtko Ursulin]

On 28/06/16 11:48, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
>> Since wait_for_atomic doesn't re-check the wait-for condition after
>> expiry of the timeout it can fail when called from non-atomic context
>> even if the condition is set correctly before the expiry. Fix this by
>> using the non-atomic wait_for instead.
>
> wait_for_atomic is indeed only safe to be called from atomic context.
> Likewise, wait_for is only safe to called from !atomic context.
>
>> I noticed this via the PLL locking timing out incorrectly, with this fix
>> I couldn't reproduce the problem.
>>
>> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
>
> The bug would be using wait_for_atomic from non-atomic context, and so
> older.
>
>
>> CC: Chris Wilson <chris@chris-wilson.co.uk>
>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index c0eff15..e130c3e 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>   	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
>>   	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
>>
>> -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
>> -			PORT_PLL_LOCK), 200))
>> +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
>> +			200))
>
> Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?

CONFIG_PREEMPT_COUNT is also required.

There were a bunch of these WARNs triggering in various places. I think 
I had patches to fix them but at the same time Mika had a more 
comprehensive work in progress for the whole area. I suppose that just 
got delayed to much.

AFAIR the meat of the discussion was what is more important - sleep 
granularity or timeout accuracy. I preferred the former to avoid waiting 
for too long for operations which are normally much quicker than a 
jiffie and normally succeed.

Another issue if wait_for_us for sleeps < 10us is not the most efficient 
implementation. So another idea I had is to implement those via the 
wait_for_atomic but without the in_atomic WARN. And obviously now after 
Imre found this with the extra cond check as well.

So I think Imre's patches are good in principle, should go in, and 
probably afterwards we can talk about improving wait_for_us for timeouts 
under 10us and potentially the timeout precision as well.

Regards,

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Chris Wilson]

On Tue, Jun 28, 2016 at 12:05:42PM +0100, Tvrtko Ursulin wrote:
> 
> On 28/06/16 11:48, Chris Wilson wrote:
> >On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
> >>Since wait_for_atomic doesn't re-check the wait-for condition after
> >>expiry of the timeout it can fail when called from non-atomic context
> >>even if the condition is set correctly before the expiry. Fix this by
> >>using the non-atomic wait_for instead.
> >
> >wait_for_atomic is indeed only safe to be called from atomic context.
> >Likewise, wait_for is only safe to called from !atomic context.
> >
> >>I noticed this via the PLL locking timing out incorrectly, with this fix
> >>I couldn't reproduce the problem.
> >>
> >>Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> >
> >The bug would be using wait_for_atomic from non-atomic context, and so
> >older.
> >
> >
> >>CC: Chris Wilson <chris@chris-wilson.co.uk>
> >>CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >>index c0eff15..e130c3e 100644
> >>--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >>+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >>@@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >>  	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
> >>  	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
> >>
> >>-	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
> >>-			PORT_PLL_LOCK), 200))
> >>+	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
> >>+			200))
> >
> >Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?
> 
> CONFIG_PREEMPT_COUNT is also required.

Maybe add a select to CONFIG_I915_DEBUG?
-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 2/4] drm/i915/lpt: Avoid early timeout during FDI PHY reset

[Tvrtko Ursulin]

On 28/06/16 11:37, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.
>
> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c3b5dc8..0312472 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8537,16 +8537,16 @@ static void lpt_reset_fdi_mphy(struct drm_i915_private *dev_priv)
>   	tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
>   	I915_WRITE(SOUTH_CHICKEN2, tmp);
>
> -	if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
> -			       FDI_MPHY_IOSFSB_RESET_STATUS, 100))
> +	if (wait_for_us(I915_READ(SOUTH_CHICKEN2) &
> +			FDI_MPHY_IOSFSB_RESET_STATUS, 100))
>   		DRM_ERROR("FDI mPHY reset assert timeout\n");
>
>   	tmp = I915_READ(SOUTH_CHICKEN2);
>   	tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
>   	I915_WRITE(SOUTH_CHICKEN2, tmp);
>
> -	if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
> -				FDI_MPHY_IOSFSB_RESET_STATUS) == 0, 100))
> +	if (wait_for_us((I915_READ(SOUTH_CHICKEN2) &
> +			 FDI_MPHY_IOSFSB_RESET_STATUS) == 0, 100))
>   		DRM_ERROR("FDI mPHY reset de-assert timeout\n");
>   }

This one was easy to find non-atomic context is correct.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Imre Deak]

On ti, 2016-06-28 at 12:05 +0100, Tvrtko Ursulin wrote:
> On 28/06/16 11:48, Chris Wilson wrote:
> > On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
> > > Since wait_for_atomic doesn't re-check the wait-for condition after
> > > expiry of the timeout it can fail when called from non-atomic context
> > > even if the condition is set correctly before the expiry. Fix this by
> > > using the non-atomic wait_for instead.
> > 
> > wait_for_atomic is indeed only safe to be called from atomic context.
> > Likewise, wait_for is only safe to called from !atomic context.
> > 
> > > I noticed this via the PLL locking timing out incorrectly, with this fix
> > > I couldn't reproduce the problem.
> > > 
> > > Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> > 
> > The bug would be using wait_for_atomic from non-atomic context, and so
> > older.
> > 
> > 
> > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index c0eff15..e130c3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > >   	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
> > >   	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
> > > 
> > > -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
> > > -			PORT_PLL_LOCK), 200))
> > > +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
> > > +			200))
> > 
> > Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?
> 
> CONFIG_PREEMPT_COUNT is also required.
> 
> There were a bunch of these WARNs triggering in various places. I think 
> I had patches to fix them but at the same time Mika had a more 
> comprehensive work in progress for the whole area. I suppose that just 
> got delayed to much.
> 
> AFAIR the meat of the discussion was what is more important - sleep 
> granularity or timeout accuracy. I preferred the former to avoid waiting 
> for too long for operations which are normally much quicker than a 
> jiffie and normally succeed.
> 
> Another issue if wait_for_us for sleeps < 10us is not the most efficient 
> implementation. So another idea I had is to implement those via the 
> wait_for_atomic but without the in_atomic WARN. And obviously now after 
> Imre found this with the extra cond check as well.

For that kind of optimization, the comment at cpu_clock() could be
interesting when comparing cpu_clock(i) wrt. cpu_clock(j) and i!=j. I
couldn't see any backward jumps between such timestamps, but I'm not
sure if that comment can be disregarded. Maybe on Intel/TSC it can.

> So I think Imre's patches are good in principle, should go in, and 
> probably afterwards we can talk about improving wait_for_us for timeouts 
> under 10us and potentially the timeout precision as well.
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/hsw: Avoid early timeout during LCPLL disable/restore

[Tvrtko Ursulin]

On 28/06/16 11:37, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.
>
> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0312472..d902a70 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9534,8 +9534,8 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>   		val |= LCPLL_CD_SOURCE_FCLK;
>   		I915_WRITE(LCPLL_CTL, val);
>
> -		if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> -				       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> +		if (wait_for_us(I915_READ(LCPLL_CTL) &
> +				LCPLL_CD_SOURCE_FCLK_DONE, 1))
>   			DRM_ERROR("Switching to FCLK failed\n");
>
>   		val = I915_READ(LCPLL_CTL);
> @@ -9608,8 +9608,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>   		val &= ~LCPLL_CD_SOURCE_FCLK;
>   		I915_WRITE(LCPLL_CTL, val);
>
> -		if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
> -					LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> +		if (wait_for_us((I915_READ(LCPLL_CTL) &
> +				 LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
>   			DRM_ERROR("Switching back to LCPLL failed\n");
>   	}
>
>

This one is also on a mutex-taking path so looks correct to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Since it just a 1us timeout, as I wrote in another reply I can follow up 
with a patch to implement those more efficiently. Does not make anything 
worse in the meantime unless operation times out which is not critical.

Regards,

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

Re: [PATCH 4/4] drm/i915: Avoid early timeout during AUX transfers

[Tvrtko Ursulin]

On 28/06/16 11:37, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.
>
> Due to the relatively long 10ms timeout, probably this didn't cause any
> real problems, but fix it in any case for consistency.
>
> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c3742a0..6d586b7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -727,7 +727,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>   		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
>   					  msecs_to_jiffies_timeout(10));
>   	else
> -		done = wait_for_atomic(C, 10) == 0;
> +		done = wait_for(C, 10) == 0;
>   	if (!done)
>   		DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
>   			  has_aux_irq);
>

This one looks obvious;

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Tvrtko Ursulin]

On 28/06/16 12:16, Imre Deak wrote:
> On ti, 2016-06-28 at 12:05 +0100, Tvrtko Ursulin wrote:
>> On 28/06/16 11:48, Chris Wilson wrote:
>>> On Tue, Jun 28, 2016 at 01:37:30PM +0300, Imre Deak wrote:
>>>> Since wait_for_atomic doesn't re-check the wait-for condition after
>>>> expiry of the timeout it can fail when called from non-atomic context
>>>> even if the condition is set correctly before the expiry. Fix this by
>>>> using the non-atomic wait_for instead.
>>>
>>> wait_for_atomic is indeed only safe to be called from atomic context.
>>> Likewise, wait_for is only safe to called from !atomic context.
>>>
>>>> I noticed this via the PLL locking timing out incorrectly, with this fix
>>>> I couldn't reproduce the problem.
>>>>
>>>> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
>>>
>>> The bug would be using wait_for_atomic from non-atomic context, and so
>>> older.
>>>
>>>
>>>> CC: Chris Wilson <chris@chris-wilson.co.uk>
>>>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>>> index c0eff15..e130c3e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>>>> @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>>>    	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
>>>>    	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
>>>>
>>>> -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
>>>> -			PORT_PLL_LOCK), 200))
>>>> +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
>>>> +			200))
>>>
>>> Does this work with CONFIG_I915_DEBUG and CONFIG_DEBUG_ATOMIC_SLEEP ?
>>
>> CONFIG_PREEMPT_COUNT is also required.
>>
>> There were a bunch of these WARNs triggering in various places. I think
>> I had patches to fix them but at the same time Mika had a more
>> comprehensive work in progress for the whole area. I suppose that just
>> got delayed to much.
>>
>> AFAIR the meat of the discussion was what is more important - sleep
>> granularity or timeout accuracy. I preferred the former to avoid waiting
>> for too long for operations which are normally much quicker than a
>> jiffie and normally succeed.
>>
>> Another issue if wait_for_us for sleeps < 10us is not the most efficient
>> implementation. So another idea I had is to implement those via the
>> wait_for_atomic but without the in_atomic WARN. And obviously now after
>> Imre found this with the extra cond check as well.
>
> For that kind of optimization, the comment at cpu_clock() could be
> interesting when comparing cpu_clock(i) wrt. cpu_clock(j) and i!=j. I
> couldn't see any backward jumps between such timestamps, but I'm not
> sure if that comment can be disregarded. Maybe on Intel/TSC it can.

You are right, to bad. Perhaps disabling preemption would do the trick 
in that case.

Regards,

Tvrtko

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

Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable

[Tvrtko Ursulin]

On 28/06/16 11:37, Imre Deak wrote:
> Since wait_for_atomic doesn't re-check the wait-for condition after
> expiry of the timeout it can fail when called from non-atomic context
> even if the condition is set correctly before the expiry. Fix this by
> using the non-atomic wait_for instead.
>
> I noticed this via the PLL locking timing out incorrectly, with this fix
> I couldn't reproduce the problem.
>
> Fixes: 0351b93992aa ("drm/i915: Do not lie about atomic timeout granularity")
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c0eff15..e130c3e 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1374,8 +1374,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>   	I915_WRITE(BXT_PORT_PLL_ENABLE(port), temp);
>   	POSTING_READ(BXT_PORT_PLL_ENABLE(port));
>
> -	if (wait_for_atomic_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) &
> -			PORT_PLL_LOCK), 200))
> +	if (wait_for_us((I915_READ(BXT_PORT_PLL_ENABLE(port)) & PORT_PLL_LOCK),
> +			200))
>   		DRM_ERROR("PLL %d not locked\n", port);
>
>   	/*
>

This one was a bit more work to verify but is definitely a non-atomic 
path as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: ✓ Ro.CI.BAT: success for drm/i915: Avoid early timeout due to wait_for_atomic

[Imre Deak]

On Tue, 2016-06-28 at 11:00 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Avoid early timeout due to wait_for_atomic
> URL   : https://patchwork.freedesktop.org/series/9224/
> State : success

Thanks for the reviews, I pushed the patchset to -dinq. I also added
CC: drm-intel-fixes.

> 
> == Summary ==
> 
> Series 9224v1 drm/i915: Avoid early timeout due to wait_for_atomic
> http://patchwork.freedesktop.org/api/1.0/series/9224/revisions/1/mbox
> 
> 
> fi-hsw-i7-
> 4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
> fi-kbl-
> qkkr      total:83   pass:62   dwarn:1   dfail:0   fail:0   skip:19 
> fi-skl-i5-
> 6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
> fi-skl-i7-
> 6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
> fi-snb-i7-
> 2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
> ro-bdw-i5-
> 5250u  total:229  pass:202  dwarn:4   dfail:1   fail:2   skip:20 
> ro-bdw-i7-
> 5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
> ro-bdw-i7-
> 5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
> ro-bsw-
> n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
> ro-byt-
> n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
> ro-hsw-i3-
> 4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
> ro-hsw-i7-
> 4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
> ro-ilk-i7-
> 620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
> ro-ilk1-i5-
> 650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
> ro-ivb-i7-
> 3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
> ro-ivb2-i7-
> 3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
> ro-skl3-i5-6260u
> total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
> ro-snb-i7-
> 2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
> 
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1317/
> 
> 13bc84e drm-intel-nightly: 2016y-06m-27d-21h-27m-04s UTC integration
> manifest
> 3e235ec drm/i915: Avoid early timeout during AUX transfers
> 45c3bee drm/i915/hsw: Avoid early timeout during LCPLL
> disable/restore
> c6850bf drm/i915/lpt: Avoid early timeout during FDI PHY reset
> bbf86d1 drm/i915/bxt: Avoid early timeout during PLL enable
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx