All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Avoid busy-spinning on VLV_GLTC_PW_STATUS mmio
Date: Fri, 21 Apr 2017 16:38:49 +0300	[thread overview]
Message-ID: <20170421133849.GM30290@intel.com> (raw)
In-Reply-To: <20170421130040.5450-1-chris@chris-wilson.co.uk>

On Fri, Apr 21, 2017 at 02:00:40PM +0100, Chris Wilson wrote:
> The busy-spin, as the first stage of intel_wait_for_register, is
> currently under suspicion for causing:
> 
> [   62.034926] NMI watchdog: Watchdog detected hard LOCKUP on cpu 1
> [   62.034928] Modules linked in: i2c_dev i915 intel_gtt drm_kms_helper prime_numbers
> [   62.034932] CPU: 1 PID: 183 Comm: kworker/1:2 Not tainted 4.11.0-rc7+ #471
> [   62.034933] Hardware name:                  /        , BIOS PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
> [   62.034934] Workqueue: pm pm_runtime_work
> [   62.034936] task: ffff880275a04ec0 task.stack: ffffc900002d8000
> [   62.034936] RIP: 0010:__intel_wait_for_register_fw+0x77/0x1a0 [i915]
> [   62.034937] RSP: 0018:ffffc900002dbc38 EFLAGS: 00000082
> [   62.034939] RAX: ffffc90003530094 RBX: 0000000000130094 RCX: 0000000000000001
> [   62.034940] RDX: 00000000000000a1 RSI: ffff88027fd15e58 RDI: 0000000000000000
> [   62.034941] RBP: ffffc900002dbc78 R08: 0000000000000002 R09: 0000000000000000
> [   62.034942] R10: ffffc900002dbc18 R11: ffff880276429dd0 R12: ffff8802707c0000
> [   62.034943] R13: 00000000000000a0 R14: 0000000000000000 R15: 00000000fffefc10
> [   62.034945] FS:  0000000000000000(0000) GS:ffff88027fd00000(0000) knlGS:0000000000000000
> [   62.034945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   62.034947] CR2: 00007ffd3cd98ff8 CR3: 0000000274c19000 CR4: 00000000001006e0
> [   62.034947] Call Trace:
> [   62.034948]  intel_wait_for_register+0x77/0x140 [i915]
> [   62.034949]  vlv_suspend_complete+0x23/0x5b0 [i915]
> [   62.034950]  intel_runtime_suspend+0x16c/0x2a0 [i915]
> [   62.034950]  pci_pm_runtime_suspend+0x50/0x180
> [   62.034951]  ? pci_pm_runtime_resume+0xa0/0xa0
> [   62.034952]  __rpm_callback+0xc5/0x210
> [   62.034953]  rpm_callback+0x1f/0x80
> [   62.034953]  ? pci_pm_runtime_resume+0xa0/0xa0
> [   62.034954]  rpm_suspend+0x118/0x580
> [   62.034955]  pm_runtime_work+0x64/0x90
> [   62.034956]  process_one_work+0x1bb/0x3e0
> [   62.034956]  worker_thread+0x46/0x4f0
> [   62.034957]  ? __schedule+0x18b/0x610
> [   62.034958]  kthread+0xff/0x140
> [   62.034958]  ? process_one_work+0x3e0/0x3e0
> [   62.034959]  ? kthread_create_on_node+
> 
> and related hard lockups in CI for byt and bsw.
> 
> Note this effectively reverts commits 41ce405e6894 and b27366958869
> ("drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register()")
> 
> Fixes: 41ce405e6894 ("drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register()")
> Fixes: b27366958869 ("drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register()")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100718
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e4f902f61e57..89b517c478e8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2179,6 +2179,17 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
>  }
>  
> +static int vlv_wait_for_pw_status(struct drm_i915_private *dev_priv,
> +				  u32 mask, u32 val)
> +{
> +	/* The HW does not like us polling for PW_STATUS frequently, so
> +	 * use the sleeping loop rather than risk the busy spin within
> +	 * intel_wait_for_register().
> +	 */
> +	return wait_for((I915_READ_NOTRACE(VLV_GTLC_PW_STATUS) & mask) == val,
> +			3);
> +}
> +
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  {
>  	u32 val;
> @@ -2208,7 +2219,7 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  {
>  	u32 val;
> -	int err = 0;
> +	int err;
>  
>  	val = I915_READ(VLV_GTLC_WAKE_CTRL);
>  	val &= ~VLV_GTLC_ALLOWWAKEREQ;
> @@ -2217,45 +2228,29 @@ static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
>  	POSTING_READ(VLV_GTLC_WAKE_CTRL);
>  
> -	err = intel_wait_for_register(dev_priv,
> -				      VLV_GTLC_PW_STATUS,
> -				      VLV_GTLC_ALLOWWAKEACK,
> -				      allow,
> -				      1);
> +	err = vlv_wait_for_pw_status(dev_priv, VLV_GTLC_ALLOWWAKEACK, allow);

Looks a bit funny to wait for a boolean. Maybe 'allow ? VLV_GTLC_ALLOWWAKEACK : 0'
to make this a little less confusing? 

>  	if (err)
>  		DRM_ERROR("timeout disabling GT waking\n");
>  
>  	return err;
>  }
>  
> -static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> -				 bool wait_for_on)
> +static void vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +				  bool wait_for_on)
>  {
>  	u32 mask;
>  	u32 val;
> -	int err;
>  
>  	mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
>  	val = wait_for_on ? mask : 0;
> -	if ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> -		return 0;
> -
> -	DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> -		      onoff(wait_for_on),
> -		      I915_READ(VLV_GTLC_PW_STATUS));
>  
>  	/*
>  	 * RC6 transitioning can be delayed up to 2 msec (see
>  	 * valleyview_enable_rps), use 3 msec for safety.
>  	 */

Maybe move the comment into vlv_wait_for_pw_status() as well since that's
where the timeout is specified now?

Haven't verfied the hardware issue myself, but the change looks sane enough, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -	err = intel_wait_for_register(dev_priv,
> -				      VLV_GTLC_PW_STATUS, mask, val,
> -				      3);
> -	if (err)
> +	if (vlv_wait_for_pw_status(dev_priv, mask, val))
>  		DRM_ERROR("timeout waiting for GT wells to go %s\n",
>  			  onoff(wait_for_on));
> -
> -	return err;
>  }
>  
>  static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> @@ -2276,7 +2271,7 @@ static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
>  	 * Bspec defines the following GT well on flags as debug only, so
>  	 * don't treat them as hard failures.
>  	 */
> -	(void)vlv_wait_for_gt_wells(dev_priv, false);
> +	vlv_wait_for_gt_wells(dev_priv, false);
>  
>  	mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
>  	WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2017-04-21 13:38 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-21 13:00 [PATCH] drm/i915: Avoid busy-spinning on VLV_GLTC_PW_STATUS mmio Chris Wilson
2017-04-21 13:32 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-04-21 13:38 ` Ville Syrjälä [this message]
2017-04-21 13:58 ` [PATCH v2] " Chris Wilson
2017-04-21 14:26 ` ✓ Fi.CI.BAT: success for drm/i915: Avoid busy-spinning on VLV_GLTC_PW_STATUS mmio (rev2) Patchwork
2017-04-21 15:42   ` Chris Wilson

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=20170421133849.GM30290@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=tomi.p.sarvela@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.