From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
Imre Deak <imre.deak@intel.com>,
intel-gfx@lists.freedesktop.org,
Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Subject: Re: [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable
Date: Tue, 28 Jun 2016 12:05:42 +0100 [thread overview]
Message-ID: <57725A06.7040609@linux.intel.com> (raw)
In-Reply-To: <20160628104838.GB10917@nuc-i3427.alporthouse.com>
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
next prev parent reply other threads:[~2016-06-28 11:05 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-28 10:37 [PATCH 0/4] drm/i915: Avoid early timeout due to wait_for_atomic Imre Deak
2016-06-28 10:37 ` [PATCH 1/4] drm/i915/bxt: Avoid early timeout during PLL enable Imre Deak
2016-06-28 10:48 ` Chris Wilson
2016-06-28 11:00 ` Imre Deak
2016-06-28 11:05 ` Tvrtko Ursulin [this message]
2016-06-28 11:11 ` Chris Wilson
2016-06-28 11:16 ` Imre Deak
2016-06-28 11:21 ` Tvrtko Ursulin
2016-06-28 11:26 ` Tvrtko Ursulin
2016-06-28 10:37 ` [PATCH 2/4] drm/i915/lpt: Avoid early timeout during FDI PHY reset Imre Deak
2016-06-28 10:50 ` Chris Wilson
2016-06-28 11:03 ` Imre Deak
2016-06-28 11:12 ` Tvrtko Ursulin
2016-06-28 10:37 ` [PATCH 3/4] drm/i915/hsw: Avoid early timeout during LCPLL disable/restore Imre Deak
2016-06-28 11:17 ` Tvrtko Ursulin
2016-06-28 10:37 ` [PATCH 4/4] drm/i915: Avoid early timeout during AUX transfers Imre Deak
2016-06-28 11:19 ` Tvrtko Ursulin
2016-06-28 11:00 ` ✓ Ro.CI.BAT: success for drm/i915: Avoid early timeout due to wait_for_atomic Patchwork
2016-06-28 19:17 ` Imre Deak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57725A06.7040609@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=tvrtko.ursulin@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.