All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
@ 2018-03-08 12:02 Maarten Lankhorst
  2018-03-08 12:20 ` ✓ Fi.CI.BAT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-08 12:02 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Changes since v1:
- Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
Changes since v2:
- Make crc functions for !DEBUGFS case inline.
- Pass intel_crtc to crc functions.
- Add comments to callsites.
Changes since v3:
- Cache selected source to pipe_crc->source.
- Set pipe_crc->skipped to MIN_INT during disable to close a race condition.
Changes since v4:
- Handle fallout from setting pipe_crc->source in irq handler.

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c       |  4 +--
 drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
 drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ce16003ef048..7807488084fe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1626,7 +1626,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 	int head, tail;
 
 	spin_lock(&pipe_crc->lock);
-	if (pipe_crc->source) {
+	if (pipe_crc->source && !crtc->base.crc.opened) {
 		if (!pipe_crc->entries) {
 			spin_unlock(&pipe_crc->lock);
 			DRM_DEBUG_KMS("spurious interrupt\n");
@@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 		 * On GEN8+ sometimes the second CRC is bonkers as well, so
 		 * don't trust that one either.
 		 */
-		if (pipe_crc->skipped == 0 ||
+		if (pipe_crc->skipped <= 0 ||
 		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
 			pipe_crc->skipped++;
 			spin_unlock(&pipe_crc->lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..2933ad38094f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+
+		/* vblanks work again, re-enable pipe CRC. */
+		intel_crtc_enable_pipe_crc(intel_crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..fce5d3072d97 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
+{
+}
+
+static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
+{
+}
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..4f367c16e9e5 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				uint32_t *val)
+				uint32_t *val,
+				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PF;
@@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if ((IS_HASWELL(dev_priv) ||
+		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
 
@@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val)
+			       enum intel_pipe_crc_source *source, u32 *val,
+			       bool set_wa)
 {
 	if (IS_GEN2(dev_priv))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
@@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
+	pipe_crc->source = source;
 	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
 
@@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
+		return;
+
+	/* Don't need pipe_crc->lock here, IRQs are not generated. */
+	pipe_crc->skipped = 0;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+
+	/* Swallow crc's until we stop generating them. */
+	spin_lock_irq(&pipe_crc->lock);
+	pipe_crc->skipped = INT_MIN;
+	spin_unlock_irq(&pipe_crc->lock);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6)
  2018-03-08 12:02 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
@ 2018-03-08 12:20 ` Patchwork
  2018-03-08 15:22 ` [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Ville Syrjälä
  2018-03-08 15:25 ` ✓ Fi.CI.IGT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
  2 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2018-03-08 12:20 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6)
URL   : https://patchwork.freedesktop.org/series/39508/
State : success

== Summary ==

Series 39508v6 drm/i915: Handle pipe CRC around enabling/disabling pipe.
https://patchwork.freedesktop.org/api/1.0/series/39508/revisions/6/mbox/

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:424s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:502s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:286s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:494s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:495s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:486s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:471s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:403s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:583s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:294s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:520s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:399s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:412s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:418s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:469s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:591s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:427s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:524s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:534s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:499s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:481s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:423s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:428s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:524s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:402s
Blacklisted hosts:
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:19  time:516s
fi-cfl-u failed to connect after reboot

e4a2b905cbe1a129e1252b6f80a97add0dfbff40 drm-tip: 2018y-03m-08d-10h-27m-13s UTC integration manifest
fb8d3205c6d9 drm/i915: Handle pipe CRC around enabling/disabling pipe.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8271/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-08 12:02 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
  2018-03-08 12:20 ` ✓ Fi.CI.BAT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
@ 2018-03-08 15:22 ` Ville Syrjälä
  2018-03-08 16:02   ` Maarten Lankhorst
  2018-03-08 15:25 ` ✓ Fi.CI.IGT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
  2 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-03-08 15:22 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Thu, Mar 08, 2018 at 01:02:02PM +0100, Maarten Lankhorst wrote:
> This will get rid of the following error:
> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
> [   74.730382] Call Trace:
> [   74.730385]  <IRQ>
> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
> [   74.730401]  drm_update_vblank_count+0x64/0x240
> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
> [   74.730561]  handle_irq_event+0x2f/0x50
> [   74.730566]  handle_edge_irq+0xe4/0x1b0
> [   74.730572]  handle_irq+0x11/0x20
> [   74.730576]  do_IRQ+0x5e/0x120
> [   74.730584]  common_interrupt+0x84/0x84
> [   74.730586]  </IRQ>
> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
> [   74.730629]  do_idle+0x188/0x1d0
> [   74.730636]  cpu_startup_entry+0x14/0x20
> [   74.730641]  start_secondary+0x129/0x160
> [   74.730646]  secondary_startup_64+0xa5/0xb0
> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
> [   74.730754] ---[ end trace 14b1345705b68565 ]---
> 
> Changes since v1:
> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
> Changes since v2:
> - Make crc functions for !DEBUGFS case inline.
> - Pass intel_crtc to crc functions.
> - Add comments to callsites.
> Changes since v3:
> - Cache selected source to pipe_crc->source.
> - Set pipe_crc->skipped to MIN_INT during disable to close a race condition.
> Changes since v4:
> - Handle fallout from setting pipe_crc->source in irq handler.
> 
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c       |  4 +--
>  drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
>  drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
>  4 files changed, 67 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ce16003ef048..7807488084fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1626,7 +1626,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  	int head, tail;
>  
>  	spin_lock(&pipe_crc->lock);
> -	if (pipe_crc->source) {
> +	if (pipe_crc->source && !crtc->base.crc.opened) {

Hmm. This is starting to get confusing. Seeing as we now lose the
capability to get all the crcs across the modeset anyway I think I'll
have to revoke my objection to nuking the old interface. Consider
that old nugget acked.

>  		if (!pipe_crc->entries) {
>  			spin_unlock(&pipe_crc->lock);
>  			DRM_DEBUG_KMS("spurious interrupt\n");
> @@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  		 * On GEN8+ sometimes the second CRC is bonkers as well, so
>  		 * don't trust that one either.
>  		 */
> -		if (pipe_crc->skipped == 0 ||
> +		if (pipe_crc->skipped <= 0 ||
>  		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
>  			pipe_crc->skipped++;
>  			spin_unlock(&pipe_crc->lock);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 331084082545..2933ad38094f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	if (modeset) {
>  		update_scanline_offset(intel_crtc);
>  		dev_priv->display.crtc_enable(pipe_config, state);
> +
> +		/* vblanks work again, re-enable pipe CRC. */
> +		intel_crtc_enable_pipe_crc(intel_crtc);
>  	} else {
>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>  				       pipe_config);
> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  		if (old_crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> +
> +			/*
> +			 * We need to disable pipe CRC before disabling the pipe,
> +			 * or we race against vblank off.
> +			 */
> +			intel_crtc_disable_pipe_crc(intel_crtc);
> +
>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>  			intel_crtc->active = false;
>  			intel_fbc_disable(intel_crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d4368589b355..fce5d3072d97 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>  #ifdef CONFIG_DEBUG_FS
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt);
> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>  #else
>  #define intel_crtc_set_crc_source NULL
> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
> +{
> +}
> +
> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
> +{
> +}
>  #endif
>  extern const struct file_operations i915_display_crc_ctl_fops;
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 1f5cd572a7ff..4f367c16e9e5 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				enum pipe pipe,
>  				enum intel_pipe_crc_source *source,
> -				uint32_t *val)
> +				uint32_t *val,
> +				bool set_wa)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if ((IS_HASWELL(dev_priv) ||
> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>  			hsw_pipe_A_crc_wa(dev_priv, true);
>  
> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
> -			       enum intel_pipe_crc_source *source, u32 *val)
> +			       enum intel_pipe_crc_source *source, u32 *val,
> +			       bool set_wa)
>  {
>  	if (IS_GEN2(dev_priv))
>  		return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>  		return ilk_pipe_crc_ctl_reg(source, val);
>  	else
> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>  }
>  
>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt)
>  {
> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>  	enum intel_display_power_domain power_domain;
>  	enum intel_pipe_crc_source source;
> @@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> +	pipe_crc->source = source;
>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>  
> @@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  
>  	return ret;
>  }
> +
> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> +	u32 val = 0;
> +
> +	if (!crtc->crc.opened)
> +		return;
> +
> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
> +		return;
> +
> +	/* Don't need pipe_crc->lock here, IRQs are not generated. */
> +	pipe_crc->skipped = 0;
> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +}
> +
> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> +
> +	/* Swallow crc's until we stop generating them. */
> +	spin_lock_irq(&pipe_crc->lock);
> +	pipe_crc->skipped = INT_MIN;
> +	spin_unlock_irq(&pipe_crc->lock);

Does this mean the hw can still generate a crc interrupt after the
PIPE_CRC_CTL has been cleared?

> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +	synchronize_irq(dev_priv->drm.irq);
> +}
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6)
  2018-03-08 12:02 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
  2018-03-08 12:20 ` ✓ Fi.CI.BAT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
  2018-03-08 15:22 ` [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Ville Syrjälä
@ 2018-03-08 15:25 ` Patchwork
  2 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2018-03-08 15:25 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6)
URL   : https://patchwork.freedesktop.org/series/39508/
State : success

== Summary ==

---- Known issues:

Test gem_eio:
        Subgroup in-flight-contexts:
                incomplete -> PASS       (shard-apl) fdo#105341
Test gem_softpin:
        Subgroup noreloc-s3:
                skip       -> PASS       (shard-snb) fdo#103375
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-bottom-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test kms_flip:
        Subgroup flip-vs-expired-vblank:
                fail       -> PASS       (shard-hsw) fdo#102887
Test kms_frontbuffer_tracking:
        Subgroup fbc-suspend:
                fail       -> PASS       (shard-apl) fdo#101623

fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-apl        total:3467 pass:1825 dwarn:1   dfail:0   fail:8   skip:1632 time:12544s
shard-hsw        total:3467 pass:1773 dwarn:1   dfail:0   fail:1   skip:1691 time:11906s
shard-snb        total:3467 pass:1364 dwarn:1   dfail:0   fail:2   skip:2100 time:7075s
Blacklisted hosts:
shard-kbl        total:3381 pass:1900 dwarn:1   dfail:0   fail:9   skip:1470 time:9172s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8271/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-08 15:22 ` [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Ville Syrjälä
@ 2018-03-08 16:02   ` Maarten Lankhorst
  2018-03-08 16:21     ` Ville Syrjälä
  0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-08 16:02 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Op 08-03-18 om 16:22 schreef Ville Syrjälä:
> On Thu, Mar 08, 2018 at 01:02:02PM +0100, Maarten Lankhorst wrote:
>> This will get rid of the following error:
>> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
>> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
>> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
>> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
>> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
>> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
>> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
>> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
>> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
>> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
>> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
>> [   74.730382] Call Trace:
>> [   74.730385]  <IRQ>
>> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
>> [   74.730401]  drm_update_vblank_count+0x64/0x240
>> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
>> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
>> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
>> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
>> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
>> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
>> [   74.730561]  handle_irq_event+0x2f/0x50
>> [   74.730566]  handle_edge_irq+0xe4/0x1b0
>> [   74.730572]  handle_irq+0x11/0x20
>> [   74.730576]  do_IRQ+0x5e/0x120
>> [   74.730584]  common_interrupt+0x84/0x84
>> [   74.730586]  </IRQ>
>> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
>> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
>> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
>> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
>> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
>> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
>> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
>> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
>> [   74.730629]  do_idle+0x188/0x1d0
>> [   74.730636]  cpu_startup_entry+0x14/0x20
>> [   74.730641]  start_secondary+0x129/0x160
>> [   74.730646]  secondary_startup_64+0xa5/0xb0
>> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
>> [   74.730754] ---[ end trace 14b1345705b68565 ]---
>>
>> Changes since v1:
>> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
>> Changes since v2:
>> - Make crc functions for !DEBUGFS case inline.
>> - Pass intel_crtc to crc functions.
>> - Add comments to callsites.
>> Changes since v3:
>> - Cache selected source to pipe_crc->source.
>> - Set pipe_crc->skipped to MIN_INT during disable to close a race condition.
>> Changes since v4:
>> - Handle fallout from setting pipe_crc->source in irq handler.
>>
>> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
>> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c       |  4 +--
>>  drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
>>  drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
>>  drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
>>  4 files changed, 67 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index ce16003ef048..7807488084fe 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1626,7 +1626,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>  	int head, tail;
>>  
>>  	spin_lock(&pipe_crc->lock);
>> -	if (pipe_crc->source) {
>> +	if (pipe_crc->source && !crtc->base.crc.opened) {
> Hmm. This is starting to get confusing. Seeing as we now lose the
> capability to get all the crcs across the modeset anyway I think I'll
> have to revoke my objection to nuking the old interface. Consider
> that old nugget acked.
Ok thanks. :)
>>  		if (!pipe_crc->entries) {
>>  			spin_unlock(&pipe_crc->lock);
>>  			DRM_DEBUG_KMS("spurious interrupt\n");
>> @@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>  		 * On GEN8+ sometimes the second CRC is bonkers as well, so
>>  		 * don't trust that one either.
>>  		 */
>> -		if (pipe_crc->skipped == 0 ||
>> +		if (pipe_crc->skipped <= 0 ||
>>  		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
>>  			pipe_crc->skipped++;
>>  			spin_unlock(&pipe_crc->lock);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 331084082545..2933ad38094f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>>  	if (modeset) {
>>  		update_scanline_offset(intel_crtc);
>>  		dev_priv->display.crtc_enable(pipe_config, state);
>> +
>> +		/* vblanks work again, re-enable pipe CRC. */
>> +		intel_crtc_enable_pipe_crc(intel_crtc);
>>  	} else {
>>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>>  				       pipe_config);
>> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  
>>  		if (old_crtc_state->active) {
>>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
>> +
>> +			/*
>> +			 * We need to disable pipe CRC before disabling the pipe,
>> +			 * or we race against vblank off.
>> +			 */
>> +			intel_crtc_disable_pipe_crc(intel_crtc);
>> +
>>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>>  			intel_crtc->active = false;
>>  			intel_fbc_disable(intel_crtc);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d4368589b355..fce5d3072d97 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>>  #ifdef CONFIG_DEBUG_FS
>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  			      size_t *values_cnt);
>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>>  #else
>>  #define intel_crtc_set_crc_source NULL
>> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
>> +{
>> +}
>> +
>> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
>> +{
>> +}
>>  #endif
>>  extern const struct file_operations i915_display_crc_ctl_fops;
>>  #endif /* __INTEL_DRV_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> index 1f5cd572a7ff..4f367c16e9e5 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  				enum pipe pipe,
>>  				enum intel_pipe_crc_source *source,
>> -				uint32_t *val)
>> +				uint32_t *val,
>> +				bool set_wa)
>>  {
>>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
>> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>>  		break;
>>  	case INTEL_PIPE_CRC_SOURCE_PF:
>> -		if ((IS_HASWELL(dev_priv) ||
>> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>>  			hsw_pipe_A_crc_wa(dev_priv, true);
>>  
>> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  
>>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  			       enum pipe pipe,
>> -			       enum intel_pipe_crc_source *source, u32 *val)
>> +			       enum intel_pipe_crc_source *source, u32 *val,
>> +			       bool set_wa)
>>  {
>>  	if (IS_GEN2(dev_priv))
>>  		return i8xx_pipe_crc_ctl_reg(source, val);
>> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>>  		return ilk_pipe_crc_ctl_reg(source, val);
>>  	else
>> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>>  }
>>  
>>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>>  		return -EIO;
>>  	}
>>  
>> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
>> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>>  	if (ret != 0)
>>  		goto out;
>>  
>> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  			      size_t *values_cnt)
>>  {
>> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>>  	enum intel_display_power_domain power_domain;
>>  	enum intel_pipe_crc_source source;
>> @@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  		return -EIO;
>>  	}
>>  
>> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
>> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>>  	if (ret != 0)
>>  		goto out;
>>  
>> +	pipe_crc->source = source;
>>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>>  
>> @@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  
>>  	return ret;
>>  }
>> +
>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
>> +{
>> +	struct drm_crtc *crtc = &intel_crtc->base;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>> +	u32 val = 0;
>> +
>> +	if (!crtc->crc.opened)
>> +		return;
>> +
>> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
>> +		return;
>> +
>> +	/* Don't need pipe_crc->lock here, IRQs are not generated. */
>> +	pipe_crc->skipped = 0;
>> +
>> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>> +}
>> +
>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
>> +{
>> +	struct drm_crtc *crtc = &intel_crtc->base;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>> +
>> +	/* Swallow crc's until we stop generating them. */
>> +	spin_lock_irq(&pipe_crc->lock);
>> +	pipe_crc->skipped = INT_MIN;
>> +	spin_unlock_irq(&pipe_crc->lock);
> Does this mean the hw can still generate a crc interrupt after the
> PIPE_CRC_CTL has been cleared?
I think at most just 1, but if modeset disable is fast enough we could theoretically hit it after vblank off probably.
Hence the extra paranoia of setting skipped before synchronize_irq, at that point we know for sure that any
CRC will be swallowed after disable_pipe_crc returns, even on a crazy -rt kernel.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-08 16:02   ` Maarten Lankhorst
@ 2018-03-08 16:21     ` Ville Syrjälä
  2018-03-08 16:38       ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-03-08 16:21 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Thu, Mar 08, 2018 at 05:02:38PM +0100, Maarten Lankhorst wrote:
> Op 08-03-18 om 16:22 schreef Ville Syrjälä:
> > On Thu, Mar 08, 2018 at 01:02:02PM +0100, Maarten Lankhorst wrote:
> >> This will get rid of the following error:
> >> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> >> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
> >> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
> >> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
> >> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> >> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
> >> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
> >> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
> >> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
> >> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
> >> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
> >> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
> >> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
> >> [   74.730382] Call Trace:
> >> [   74.730385]  <IRQ>
> >> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
> >> [   74.730401]  drm_update_vblank_count+0x64/0x240
> >> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
> >> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
> >> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
> >> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
> >> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
> >> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
> >> [   74.730561]  handle_irq_event+0x2f/0x50
> >> [   74.730566]  handle_edge_irq+0xe4/0x1b0
> >> [   74.730572]  handle_irq+0x11/0x20
> >> [   74.730576]  do_IRQ+0x5e/0x120
> >> [   74.730584]  common_interrupt+0x84/0x84
> >> [   74.730586]  </IRQ>
> >> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
> >> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
> >> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
> >> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
> >> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
> >> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
> >> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
> >> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
> >> [   74.730629]  do_idle+0x188/0x1d0
> >> [   74.730636]  cpu_startup_entry+0x14/0x20
> >> [   74.730641]  start_secondary+0x129/0x160
> >> [   74.730646]  secondary_startup_64+0xa5/0xb0
> >> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
> >> [   74.730754] ---[ end trace 14b1345705b68565 ]---
> >>
> >> Changes since v1:
> >> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
> >> Changes since v2:
> >> - Make crc functions for !DEBUGFS case inline.
> >> - Pass intel_crtc to crc functions.
> >> - Add comments to callsites.
> >> Changes since v3:
> >> - Cache selected source to pipe_crc->source.
> >> - Set pipe_crc->skipped to MIN_INT during disable to close a race condition.
> >> Changes since v4:
> >> - Handle fallout from setting pipe_crc->source in irq handler.
> >>
> >> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> >> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c       |  4 +--
> >>  drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
> >>  drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
> >>  drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
> >>  4 files changed, 67 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index ce16003ef048..7807488084fe 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -1626,7 +1626,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >>  	int head, tail;
> >>  
> >>  	spin_lock(&pipe_crc->lock);
> >> -	if (pipe_crc->source) {
> >> +	if (pipe_crc->source && !crtc->base.crc.opened) {
> > Hmm. This is starting to get confusing. Seeing as we now lose the
> > capability to get all the crcs across the modeset anyway I think I'll
> > have to revoke my objection to nuking the old interface. Consider
> > that old nugget acked.
> Ok thanks. :)
> >>  		if (!pipe_crc->entries) {
> >>  			spin_unlock(&pipe_crc->lock);
> >>  			DRM_DEBUG_KMS("spurious interrupt\n");
> >> @@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >>  		 * On GEN8+ sometimes the second CRC is bonkers as well, so
> >>  		 * don't trust that one either.
> >>  		 */
> >> -		if (pipe_crc->skipped == 0 ||
> >> +		if (pipe_crc->skipped <= 0 ||
> >>  		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
> >>  			pipe_crc->skipped++;
> >>  			spin_unlock(&pipe_crc->lock);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 331084082545..2933ad38094f 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> >>  	if (modeset) {
> >>  		update_scanline_offset(intel_crtc);
> >>  		dev_priv->display.crtc_enable(pipe_config, state);
> >> +
> >> +		/* vblanks work again, re-enable pipe CRC. */
> >> +		intel_crtc_enable_pipe_crc(intel_crtc);
> >>  	} else {
> >>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
> >>  				       pipe_config);
> >> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >>  
> >>  		if (old_crtc_state->active) {
> >>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> >> +
> >> +			/*
> >> +			 * We need to disable pipe CRC before disabling the pipe,
> >> +			 * or we race against vblank off.
> >> +			 */
> >> +			intel_crtc_disable_pipe_crc(intel_crtc);
> >> +
> >>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
> >>  			intel_crtc->active = false;
> >>  			intel_fbc_disable(intel_crtc);
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index d4368589b355..fce5d3072d97 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
> >>  #ifdef CONFIG_DEBUG_FS
> >>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >>  			      size_t *values_cnt);
> >> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
> >> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
> >>  #else
> >>  #define intel_crtc_set_crc_source NULL
> >> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
> >> +{
> >> +}
> >> +
> >> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
> >> +{
> >> +}
> >>  #endif
> >>  extern const struct file_operations i915_display_crc_ctl_fops;
> >>  #endif /* __INTEL_DRV_H__ */
> >> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> index 1f5cd572a7ff..4f367c16e9e5 100644
> >> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> >> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> >>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >>  				enum pipe pipe,
> >>  				enum intel_pipe_crc_source *source,
> >> -				uint32_t *val)
> >> +				uint32_t *val,
> >> +				bool set_wa)
> >>  {
> >>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> >>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
> >> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> >>  		break;
> >>  	case INTEL_PIPE_CRC_SOURCE_PF:
> >> -		if ((IS_HASWELL(dev_priv) ||
> >> +		if (set_wa && (IS_HASWELL(dev_priv) ||
> >>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> >>  			hsw_pipe_A_crc_wa(dev_priv, true);
> >>  
> >> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >>  
> >>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >>  			       enum pipe pipe,
> >> -			       enum intel_pipe_crc_source *source, u32 *val)
> >> +			       enum intel_pipe_crc_source *source, u32 *val,
> >> +			       bool set_wa)
> >>  {
> >>  	if (IS_GEN2(dev_priv))
> >>  		return i8xx_pipe_crc_ctl_reg(source, val);
> >> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
> >>  		return ilk_pipe_crc_ctl_reg(source, val);
> >>  	else
> >> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> >> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> >>  }
> >>  
> >>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >>  		return -EIO;
> >>  	}
> >>  
> >> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> >> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
> >>  	if (ret != 0)
> >>  		goto out;
> >>  
> >> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
> >>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >>  			      size_t *values_cnt)
> >>  {
> >> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> >>  	enum intel_display_power_domain power_domain;
> >>  	enum intel_pipe_crc_source source;
> >> @@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >>  		return -EIO;
> >>  	}
> >>  
> >> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> >> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
> >>  	if (ret != 0)
> >>  		goto out;
> >>  
> >> +	pipe_crc->source = source;
> >>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> >>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> >>  
> >> @@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >>  
> >>  	return ret;
> >>  }
> >> +
> >> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
> >> +{
> >> +	struct drm_crtc *crtc = &intel_crtc->base;
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> >> +	u32 val = 0;
> >> +
> >> +	if (!crtc->crc.opened)
> >> +		return;
> >> +
> >> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
> >> +		return;
> >> +
> >> +	/* Don't need pipe_crc->lock here, IRQs are not generated. */
> >> +	pipe_crc->skipped = 0;
> >> +
> >> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> >> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> >> +}
> >> +
> >> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
> >> +{
> >> +	struct drm_crtc *crtc = &intel_crtc->base;
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> >> +
> >> +	/* Swallow crc's until we stop generating them. */
> >> +	spin_lock_irq(&pipe_crc->lock);
> >> +	pipe_crc->skipped = INT_MIN;
> >> +	spin_unlock_irq(&pipe_crc->lock);
> > Does this mean the hw can still generate a crc interrupt after the
> > PIPE_CRC_CTL has been cleared?
> I think at most just 1, but if modeset disable is fast enough we could theoretically hit it after vblank off probably.
> Hence the extra paranoia of setting skipped before synchronize_irq, at that point we know for sure that any
> CRC will be swallowed after disable_pipe_crc returns, even on a crazy -rt kernel.

OK. So I guess we're not entirely sure then. I guess being paranoid
doesn't hurt though

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
still holds. Not that it did much good on the previous iteration :/

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-08 16:21     ` Ville Syrjälä
@ 2018-03-08 16:38       ` Maarten Lankhorst
  0 siblings, 0 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-08 16:38 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Op 08-03-18 om 17:21 schreef Ville Syrjälä:
> On Thu, Mar 08, 2018 at 05:02:38PM +0100, Maarten Lankhorst wrote:
>> Op 08-03-18 om 16:22 schreef Ville Syrjälä:
>>> On Thu, Mar 08, 2018 at 01:02:02PM +0100, Maarten Lankhorst wrote:
>>>> This will get rid of the following error:
>>>> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>>>> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
>>>> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
>>>> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
>>>> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>>>> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
>>>> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
>>>> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
>>>> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
>>>> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
>>>> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
>>>> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
>>>> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
>>>> [   74.730382] Call Trace:
>>>> [   74.730385]  <IRQ>
>>>> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
>>>> [   74.730401]  drm_update_vblank_count+0x64/0x240
>>>> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
>>>> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
>>>> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
>>>> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
>>>> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
>>>> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
>>>> [   74.730561]  handle_irq_event+0x2f/0x50
>>>> [   74.730566]  handle_edge_irq+0xe4/0x1b0
>>>> [   74.730572]  handle_irq+0x11/0x20
>>>> [   74.730576]  do_IRQ+0x5e/0x120
>>>> [   74.730584]  common_interrupt+0x84/0x84
>>>> [   74.730586]  </IRQ>
>>>> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
>>>> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
>>>> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
>>>> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
>>>> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
>>>> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
>>>> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
>>>> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
>>>> [   74.730629]  do_idle+0x188/0x1d0
>>>> [   74.730636]  cpu_startup_entry+0x14/0x20
>>>> [   74.730641]  start_secondary+0x129/0x160
>>>> [   74.730646]  secondary_startup_64+0xa5/0xb0
>>>> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
>>>> [   74.730754] ---[ end trace 14b1345705b68565 ]---
>>>>
>>>> Changes since v1:
>>>> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
>>>> Changes since v2:
>>>> - Make crc functions for !DEBUGFS case inline.
>>>> - Pass intel_crtc to crc functions.
>>>> - Add comments to callsites.
>>>> Changes since v3:
>>>> - Cache selected source to pipe_crc->source.
>>>> - Set pipe_crc->skipped to MIN_INT during disable to close a race condition.
>>>> Changes since v4:
>>>> - Handle fallout from setting pipe_crc->source in irq handler.
>>>>
>>>> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
>>>> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_irq.c       |  4 +--
>>>>  drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
>>>>  drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
>>>>  4 files changed, 67 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index ce16003ef048..7807488084fe 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -1626,7 +1626,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>>>  	int head, tail;
>>>>  
>>>>  	spin_lock(&pipe_crc->lock);
>>>> -	if (pipe_crc->source) {
>>>> +	if (pipe_crc->source && !crtc->base.crc.opened) {
>>> Hmm. This is starting to get confusing. Seeing as we now lose the
>>> capability to get all the crcs across the modeset anyway I think I'll
>>> have to revoke my objection to nuking the old interface. Consider
>>> that old nugget acked.
>> Ok thanks. :)
>>>>  		if (!pipe_crc->entries) {
>>>>  			spin_unlock(&pipe_crc->lock);
>>>>  			DRM_DEBUG_KMS("spurious interrupt\n");
>>>> @@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>>>>  		 * On GEN8+ sometimes the second CRC is bonkers as well, so
>>>>  		 * don't trust that one either.
>>>>  		 */
>>>> -		if (pipe_crc->skipped == 0 ||
>>>> +		if (pipe_crc->skipped <= 0 ||
>>>>  		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
>>>>  			pipe_crc->skipped++;
>>>>  			spin_unlock(&pipe_crc->lock);
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 331084082545..2933ad38094f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>>>>  	if (modeset) {
>>>>  		update_scanline_offset(intel_crtc);
>>>>  		dev_priv->display.crtc_enable(pipe_config, state);
>>>> +
>>>> +		/* vblanks work again, re-enable pipe CRC. */
>>>> +		intel_crtc_enable_pipe_crc(intel_crtc);
>>>>  	} else {
>>>>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>>>>  				       pipe_config);
>>>> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>>>  
>>>>  		if (old_crtc_state->active) {
>>>>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
>>>> +
>>>> +			/*
>>>> +			 * We need to disable pipe CRC before disabling the pipe,
>>>> +			 * or we race against vblank off.
>>>> +			 */
>>>> +			intel_crtc_disable_pipe_crc(intel_crtc);
>>>> +
>>>>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>>>>  			intel_crtc->active = false;
>>>>  			intel_fbc_disable(intel_crtc);
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index d4368589b355..fce5d3072d97 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>>>>  #ifdef CONFIG_DEBUG_FS
>>>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>>>  			      size_t *values_cnt);
>>>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
>>>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>>>>  #else
>>>>  #define intel_crtc_set_crc_source NULL
>>>> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
>>>> +{
>>>> +}
>>>> +
>>>> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
>>>> +{
>>>> +}
>>>>  #endif
>>>>  extern const struct file_operations i915_display_crc_ctl_fops;
>>>>  #endif /* __INTEL_DRV_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
>>>> index 1f5cd572a7ff..4f367c16e9e5 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>>>> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>>>>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>>>  				enum pipe pipe,
>>>>  				enum intel_pipe_crc_source *source,
>>>> -				uint32_t *val)
>>>> +				uint32_t *val,
>>>> +				bool set_wa)
>>>>  {
>>>>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>>>>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
>>>> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>>>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>>>>  		break;
>>>>  	case INTEL_PIPE_CRC_SOURCE_PF:
>>>> -		if ((IS_HASWELL(dev_priv) ||
>>>> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>>>>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>>>>  			hsw_pipe_A_crc_wa(dev_priv, true);
>>>>  
>>>> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>>>  
>>>>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>>>  			       enum pipe pipe,
>>>> -			       enum intel_pipe_crc_source *source, u32 *val)
>>>> +			       enum intel_pipe_crc_source *source, u32 *val,
>>>> +			       bool set_wa)
>>>>  {
>>>>  	if (IS_GEN2(dev_priv))
>>>>  		return i8xx_pipe_crc_ctl_reg(source, val);
>>>> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>>>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>>>>  		return ilk_pipe_crc_ctl_reg(source, val);
>>>>  	else
>>>> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>>>> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>>>>  }
>>>>  
>>>>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>>>> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>>>>  		return -EIO;
>>>>  	}
>>>>  
>>>> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
>>>> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>>>>  	if (ret != 0)
>>>>  		goto out;
>>>>  
>>>> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>>>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>>>  			      size_t *values_cnt)
>>>>  {
>>>> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>>>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>>>>  	enum intel_display_power_domain power_domain;
>>>>  	enum intel_pipe_crc_source source;
>>>> @@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>>>  		return -EIO;
>>>>  	}
>>>>  
>>>> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
>>>> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>>>>  	if (ret != 0)
>>>>  		goto out;
>>>>  
>>>> +	pipe_crc->source = source;
>>>>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>>>>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>>>>  
>>>> @@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>>>  
>>>>  	return ret;
>>>>  }
>>>> +
>>>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
>>>> +{
>>>> +	struct drm_crtc *crtc = &intel_crtc->base;
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>>> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>>>> +	u32 val = 0;
>>>> +
>>>> +	if (!crtc->crc.opened)
>>>> +		return;
>>>> +
>>>> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
>>>> +		return;
>>>> +
>>>> +	/* Don't need pipe_crc->lock here, IRQs are not generated. */
>>>> +	pipe_crc->skipped = 0;
>>>> +
>>>> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>>>> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
>>>> +}
>>>> +
>>>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
>>>> +{
>>>> +	struct drm_crtc *crtc = &intel_crtc->base;
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>>> +	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>>>> +
>>>> +	/* Swallow crc's until we stop generating them. */
>>>> +	spin_lock_irq(&pipe_crc->lock);
>>>> +	pipe_crc->skipped = INT_MIN;
>>>> +	spin_unlock_irq(&pipe_crc->lock);
>>> Does this mean the hw can still generate a crc interrupt after the
>>> PIPE_CRC_CTL has been cleared?
>> I think at most just 1, but if modeset disable is fast enough we could theoretically hit it after vblank off probably.
>> Hence the extra paranoia of setting skipped before synchronize_irq, at that point we know for sure that any
>> CRC will be swallowed after disable_pipe_crc returns, even on a crazy -rt kernel.
> OK. So I guess we're not entirely sure then. I guess being paranoid
> doesn't hurt though
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> still holds. Not that it did much good on the previous iteration :/
>
Yeah on second inspection it looks like revision 2 was working correctly and it fixed 105185, but I think the extra paranoia is good regardless.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-08  9:17 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
@ 2018-03-08  9:21 ` Maarten Lankhorst
  0 siblings, 0 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-08  9:21 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Changes since v1:
- Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
Changes since v2:
- Make crc functions for !DEBUGFS case inline.
- Pass intel_crtc to crc functions.
- Add comments to callsites.
Changes since v3:
- Cache pipe_crc->source.
- Set pipe_crc->skipped to MIN_INT during disable to close a race condition.

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Small compile fix for !DEBUGFS case.

 drivers/gpu/drm/i915/i915_irq.c       |  2 +-
 drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
 drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
 4 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ce16003ef048..316ba623c95a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 		 * On GEN8+ sometimes the second CRC is bonkers as well, so
 		 * don't trust that one either.
 		 */
-		if (pipe_crc->skipped == 0 ||
+		if (pipe_crc->skipped <= 0 ||
 		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
 			pipe_crc->skipped++;
 			spin_unlock(&pipe_crc->lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..2933ad38094f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+
+		/* vblanks work again, re-enable pipe CRC. */
+		intel_crtc_enable_pipe_crc(intel_crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..fce5d3072d97 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
+{
+}
+
+static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc)
+{
+}
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..4f367c16e9e5 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				uint32_t *val)
+				uint32_t *val,
+				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PF;
@@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if ((IS_HASWELL(dev_priv) ||
+		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
 
@@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val)
+			       enum intel_pipe_crc_source *source, u32 *val,
+			       bool set_wa)
 {
 	if (IS_GEN2(dev_priv))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
@@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
+	pipe_crc->source = source;
 	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
 
@@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
+		return;
+
+	/* Don't need pipe_crc->lock here, IRQs are not generated. */
+	pipe_crc->skipped = 0;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+
+	/* Swallow crc's until we stop generating them. */
+	spin_lock_irq(&pipe_crc->lock);
+	pipe_crc->skipped = INT_MIN;
+	spin_unlock_irq(&pipe_crc->lock);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
@ 2018-03-08  9:17 Maarten Lankhorst
  2018-03-08  9:21 ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-08  9:17 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Changes since v1:
- Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
Changes since v2:
- Make crc functions for !DEBUGFS case inline.
- Pass intel_crtc to crc functions.
- Add comments to callsites.
Changes since v3:
- Cache pipe_crc->source.
- Set pipe_crc->skipped to MIN_INT during disable to close a race condition.

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c       |  2 +-
 drivers/gpu/drm/i915/intel_display.c  | 10 +++++++
 drivers/gpu/drm/i915/intel_drv.h      |  9 ++++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 53 ++++++++++++++++++++++++++++++-----
 4 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ce16003ef048..316ba623c95a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1666,7 +1666,7 @@ static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 		 * On GEN8+ sometimes the second CRC is bonkers as well, so
 		 * don't trust that one either.
 		 */
-		if (pipe_crc->skipped == 0 ||
+		if (pipe_crc->skipped <= 0 ||
 		    (INTEL_GEN(dev_priv) >= 8 && pipe_crc->skipped == 1)) {
 			pipe_crc->skipped++;
 			spin_unlock(&pipe_crc->lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..2933ad38094f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+
+		/* vblanks work again, re-enable pipe CRC. */
+		intel_crtc_enable_pipe_crc(intel_crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..31f9859cddfc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
+{
+}
+
+static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
+{
+}
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..4f367c16e9e5 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				uint32_t *val)
+				uint32_t *val,
+				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PF;
@@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if ((IS_HASWELL(dev_priv) ||
+		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
 
@@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val)
+			       enum intel_pipe_crc_source *source, u32 *val,
+			       bool set_wa)
 {
 	if (IS_GEN2(dev_priv))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
@@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -934,10 +936,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
+	pipe_crc->source = source;
 	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
 
@@ -959,3 +962,39 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
+		return;
+
+	/* Don't need pipe_crc->lock here, IRQs are not generated. */
+	pipe_crc->skipped = 0;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
+
+	/* Swallow crc's until we stop generating them. */
+	spin_lock_irq(&pipe_crc->lock);
+	pipe_crc->skipped = INT_MIN;
+	spin_unlock_irq(&pipe_crc->lock);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-07 15:21       ` Ville Syrjälä
@ 2018-03-07 16:40         ` Maarten Lankhorst
  0 siblings, 0 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-07 16:40 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Op 07-03-18 om 16:21 schreef Ville Syrjälä:
> On Wed, Mar 07, 2018 at 04:04:32PM +0100, Maarten Lankhorst wrote:
>> This will get rid of the following error:
>> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
>> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
>> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
>> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
>> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
>> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
>> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
>> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
>> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
>> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
>> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
>> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
>> [   74.730382] Call Trace:
>> [   74.730385]  <IRQ>
>> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
>> [   74.730401]  drm_update_vblank_count+0x64/0x240
>> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
>> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
>> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
>> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
>> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
>> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
>> [   74.730561]  handle_irq_event+0x2f/0x50
>> [   74.730566]  handle_edge_irq+0xe4/0x1b0
>> [   74.730572]  handle_irq+0x11/0x20
>> [   74.730576]  do_IRQ+0x5e/0x120
>> [   74.730584]  common_interrupt+0x84/0x84
>> [   74.730586]  </IRQ>
>> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
>> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
>> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
>> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
>> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
>> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
>> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
>> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
>> [   74.730629]  do_idle+0x188/0x1d0
>> [   74.730636]  cpu_startup_entry+0x14/0x20
>> [   74.730641]  start_secondary+0x129/0x160
>> [   74.730646]  secondary_startup_64+0xa5/0xb0
>> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
>> [   74.730754] ---[ end trace 14b1345705b68565 ]---
>>
>> Changes since v1:
>> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
>> Changes since v2:
>> - Make crc functions for !DEBUGFS case inline.
>> - Pass intel_crtc to crc functions.
>> - Add comments to callsites.
>>
>> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
>> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c  | 10 ++++++++
>>  drivers/gpu/drm/i915/intel_drv.h      |  9 +++++++
>>  drivers/gpu/drm/i915/intel_pipe_crc.c | 46 +++++++++++++++++++++++++++++------
>>  3 files changed, 58 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 331084082545..2933ad38094f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>>  	if (modeset) {
>>  		update_scanline_offset(intel_crtc);
>>  		dev_priv->display.crtc_enable(pipe_config, state);
>> +
>> +		/* vblanks work again, re-enable pipe CRC. */
>> +		intel_crtc_enable_pipe_crc(intel_crtc);
>>  	} else {
>>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>>  				       pipe_config);
>> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  
>>  		if (old_crtc_state->active) {
>>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
>> +
>> +			/*
>> +			 * We need to disable pipe CRC before disabling the pipe,
>> +			 * or we race against vblank off.
>> +			 */
>> +			intel_crtc_disable_pipe_crc(intel_crtc);
>> +
>>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>>  			intel_crtc->active = false;
>>  			intel_fbc_disable(intel_crtc);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d4368589b355..31f9859cddfc 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>>  #ifdef CONFIG_DEBUG_FS
>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  			      size_t *values_cnt);
>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>>  #else
>>  #define intel_crtc_set_crc_source NULL
>> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
>> +{
>> +}
>> +
>> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>                                                                          ^
> Extra ';'
>
>> +{
>> +}
>>  #endif
>>  extern const struct file_operations i915_display_crc_ctl_fops;
>>  #endif /* __INTEL_DRV_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> index 1f5cd572a7ff..2c7a48b5a571 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  				enum pipe pipe,
>>  				enum intel_pipe_crc_source *source,
>> -				uint32_t *val)
>> +				uint32_t *val,
>> +				bool set_wa)
>>  {
>>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
>> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>>  		break;
>>  	case INTEL_PIPE_CRC_SOURCE_PF:
>> -		if ((IS_HASWELL(dev_priv) ||
>> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>>  			hsw_pipe_A_crc_wa(dev_priv, true);
>>  
>> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  
>>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  			       enum pipe pipe,
>> -			       enum intel_pipe_crc_source *source, u32 *val)
>> +			       enum intel_pipe_crc_source *source, u32 *val,
>> +			       bool set_wa)
>>  {
>>  	if (IS_GEN2(dev_priv))
>>  		return i8xx_pipe_crc_ctl_reg(source, val);
>> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>>  		return ilk_pipe_crc_ctl_reg(source, val);
>>  	else
>> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
>> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>>  }
>>  
>>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>>  		return -EIO;
>>  	}
>>  
>> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
>> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>>  	if (ret != 0)
>>  		goto out;
>>  
>> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  			      size_t *values_cnt)
>>  {
>> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>>  	enum intel_display_power_domain power_domain;
>>  	enum intel_pipe_crc_source source;
>> @@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  		return -EIO;
>>  	}
>>  
>> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
>> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>>  	if (ret != 0)
>>  		goto out;
>>  
>> @@ -959,3 +961,33 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>  
>>  	return ret;
>>  }
>> +
>> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
>> +{
>> +	struct drm_crtc *crtc = &intel_crtc->base;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +	enum intel_pipe_crc_source source;
>> +	u32 val = 0;
>> +
>> +	if (!crtc->crc.opened)
>> +		return;
>> +
>> +	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
>> +		return;
>> +
>> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
>> +		return;
>> +
>> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> I wonder what's going to happen with the scrambler reset stuff while the
> pipe is off. At least the power well going off on VLV/CHV could
> potentially clobber that state. But I guess we already have that
> problem so probably no point in worrying about it now.
Hm we don't explicitly disable the scrambler reset, but get_new_crc_ctl_reg
will re-apply the scrambler reset in intel_crtc_enable_pipe_crc,
so that part will work as previously. Will apply after CI results are in. :)
>> +}
>> +
>> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
>> +{
>> +	struct drm_crtc *crtc = &intel_crtc->base;
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +
>> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
> I wonder whether we should trust 0 to be "off". Should make use
> get_new_crc_ctl_reg() here as well? Dunno.
>
> Anyways, with the ';' fixed this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-07 15:04     ` Maarten Lankhorst
@ 2018-03-07 15:21       ` Ville Syrjälä
  2018-03-07 16:40         ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-03-07 15:21 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Mar 07, 2018 at 04:04:32PM +0100, Maarten Lankhorst wrote:
> This will get rid of the following error:
> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
> [   74.730382] Call Trace:
> [   74.730385]  <IRQ>
> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
> [   74.730401]  drm_update_vblank_count+0x64/0x240
> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
> [   74.730561]  handle_irq_event+0x2f/0x50
> [   74.730566]  handle_edge_irq+0xe4/0x1b0
> [   74.730572]  handle_irq+0x11/0x20
> [   74.730576]  do_IRQ+0x5e/0x120
> [   74.730584]  common_interrupt+0x84/0x84
> [   74.730586]  </IRQ>
> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
> [   74.730629]  do_idle+0x188/0x1d0
> [   74.730636]  cpu_startup_entry+0x14/0x20
> [   74.730641]  start_secondary+0x129/0x160
> [   74.730646]  secondary_startup_64+0xa5/0xb0
> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
> [   74.730754] ---[ end trace 14b1345705b68565 ]---
> 
> Changes since v1:
> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
> Changes since v2:
> - Make crc functions for !DEBUGFS case inline.
> - Pass intel_crtc to crc functions.
> - Add comments to callsites.
> 
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 10 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h      |  9 +++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 46 +++++++++++++++++++++++++++++------
>  3 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 331084082545..2933ad38094f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	if (modeset) {
>  		update_scanline_offset(intel_crtc);
>  		dev_priv->display.crtc_enable(pipe_config, state);
> +
> +		/* vblanks work again, re-enable pipe CRC. */
> +		intel_crtc_enable_pipe_crc(intel_crtc);
>  	} else {
>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>  				       pipe_config);
> @@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  		if (old_crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> +
> +			/*
> +			 * We need to disable pipe CRC before disabling the pipe,
> +			 * or we race against vblank off.
> +			 */
> +			intel_crtc_disable_pipe_crc(intel_crtc);
> +
>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>  			intel_crtc->active = false;
>  			intel_fbc_disable(intel_crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d4368589b355..31f9859cddfc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>  #ifdef CONFIG_DEBUG_FS
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt);
> +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
> +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>  #else
>  #define intel_crtc_set_crc_source NULL
> +static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
> +{
> +}
> +
> +static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
                                                                         ^
Extra ';'

> +{
> +}
>  #endif
>  extern const struct file_operations i915_display_crc_ctl_fops;
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 1f5cd572a7ff..2c7a48b5a571 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				enum pipe pipe,
>  				enum intel_pipe_crc_source *source,
> -				uint32_t *val)
> +				uint32_t *val,
> +				bool set_wa)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if ((IS_HASWELL(dev_priv) ||
> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>  			hsw_pipe_A_crc_wa(dev_priv, true);
>  
> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
> -			       enum intel_pipe_crc_source *source, u32 *val)
> +			       enum intel_pipe_crc_source *source, u32 *val,
> +			       bool set_wa)
>  {
>  	if (IS_GEN2(dev_priv))
>  		return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>  		return ilk_pipe_crc_ctl_reg(source, val);
>  	else
> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>  }
>  
>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt)
>  {
> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>  	enum intel_display_power_domain power_domain;
>  	enum intel_pipe_crc_source source;
> @@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -959,3 +961,33 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  
>  	return ret;
>  }
> +
> +void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum intel_pipe_crc_source source;
> +	u32 val = 0;
> +
> +	if (!crtc->crc.opened)
> +		return;
> +
> +	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
> +		return;
> +
> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
> +		return;
> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));

I wonder what's going to happen with the scrambler reset stuff while the
pipe is off. At least the power well going off on VLV/CHV could
potentially clobber that state. But I guess we already have that
problem so probably no point in worrying about it now.

> +}
> +
> +void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);

I wonder whether we should trust 0 to be "off". Should make use
get_new_crc_ctl_reg() here as well? Dunno.

Anyways, with the ';' fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +	synchronize_irq(dev_priv->drm.irq);
> +}
> -- 
> 2.16.2

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-07 14:23   ` Ville Syrjälä
@ 2018-03-07 15:04     ` Maarten Lankhorst
  2018-03-07 15:21       ` Ville Syrjälä
  0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-07 15:04 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Changes since v1:
- Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
Changes since v2:
- Make crc functions for !DEBUGFS case inline.
- Pass intel_crtc to crc functions.
- Add comments to callsites.

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 10 ++++++++
 drivers/gpu/drm/i915/intel_drv.h      |  9 +++++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 46 +++++++++++++++++++++++++++++------
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..2933ad38094f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+
+		/* vblanks work again, re-enable pipe CRC. */
+		intel_crtc_enable_pipe_crc(intel_crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12330,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..31f9859cddfc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,17 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
+{
+}
+
+static inline void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
+{
+}
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..2c7a48b5a571 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				uint32_t *val)
+				uint32_t *val,
+				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PF;
@@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if ((IS_HASWELL(dev_priv) ||
+		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
 
@@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val)
+			       enum intel_pipe_crc_source *source, u32 *val,
+			       bool set_wa)
 {
 	if (IS_GEN2(dev_priv))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
@@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -959,3 +961,33 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
+		return;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-07 13:23 ` Ville Syrjälä
@ 2018-03-07 14:23   ` Ville Syrjälä
  2018-03-07 15:04     ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-03-07 14:23 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Mar 07, 2018 at 03:23:12PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 07, 2018 at 01:10:26PM +0100, Maarten Lankhorst wrote:
> > This will get rid of the following error:
> > [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> > [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
> > [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
> > [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
> > [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> > [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
> > [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
> > [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
> > [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
> > [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
> > [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
> > [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
> > [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
> > [   74.730382] Call Trace:
> > [   74.730385]  <IRQ>
> > [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
> > [   74.730401]  drm_update_vblank_count+0x64/0x240
> > [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
> > [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
> > [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
> > [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
> > [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
> > [   74.730556]  handle_irq_event_percpu+0x1b/0x50
> > [   74.730561]  handle_irq_event+0x2f/0x50
> > [   74.730566]  handle_edge_irq+0xe4/0x1b0
> > [   74.730572]  handle_irq+0x11/0x20
> > [   74.730576]  do_IRQ+0x5e/0x120
> > [   74.730584]  common_interrupt+0x84/0x84
> > [   74.730586]  </IRQ>
> > [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
> > [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
> > [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
> > [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
> > [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
> > [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
> > [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
> > [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
> > [   74.730629]  do_idle+0x188/0x1d0
> > [   74.730636]  cpu_startup_entry+0x14/0x20
> > [   74.730641]  start_secondary+0x129/0x160
> > [   74.730646]  secondary_startup_64+0xa5/0xb0
> > [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
> > [   74.730754] ---[ end trace 14b1345705b68565 ]---
> > 
> > Changes since v1:
> > - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
> > 
> > Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> > Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h      |  3 +++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 44 +++++++++++++++++++++++++++++------
> >  3 files changed, 42 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 331084082545..67d13fe4c24f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12147,6 +12147,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> >  	if (modeset) {
> >  		update_scanline_offset(intel_crtc);
> >  		dev_priv->display.crtc_enable(pipe_config, state);
> > +		intel_crtc_enable_pipe_crc(crtc);
> 
> I think I'd prefer this to live next to the vblank on/off, with a
> comment. Putting it here gives absolutely no clue why we have to
> do it.

...or at least we need a comment here. 

> 
> >  	} else {
> >  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
> >  				       pipe_config);
> > @@ -12327,6 +12328,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  
> >  		if (old_crtc_state->active) {
> >  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> > +			intel_crtc_disable_pipe_crc(crtc);
> >  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
> >  			intel_crtc->active = false;
> >  			intel_fbc_disable(intel_crtc);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index d4368589b355..002429957dd5 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2138,8 +2138,11 @@ int intel_pipe_crc_create(struct drm_minor *minor);
> >  #ifdef CONFIG_DEBUG_FS
> >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			      size_t *values_cnt);
> > +void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc);
> > +void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc);

Pass intel_crtc pls.

> >  #else
> >  #define intel_crtc_set_crc_source NULL
> > +#define intel_crtc_disable_pipe_crc(crtc) do {} while (0)

static inline ?

enable?

> >  #endif
> >  extern const struct file_operations i915_display_crc_ctl_fops;
> >  #endif /* __INTEL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 1f5cd572a7ff..9422ed98e987 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> >  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  				enum pipe pipe,
> >  				enum intel_pipe_crc_source *source,
> > -				uint32_t *val)
> > +				uint32_t *val,
> > +				bool set_wa)
> >  {
> >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> >  		*source = INTEL_PIPE_CRC_SOURCE_PF;
> > @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> >  		break;
> >  	case INTEL_PIPE_CRC_SOURCE_PF:
> > -		if ((IS_HASWELL(dev_priv) ||
> > +		if (set_wa && (IS_HASWELL(dev_priv) ||
> >  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> >  			hsw_pipe_A_crc_wa(dev_priv, true);
> >  
> > @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  
> >  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe,
> > -			       enum intel_pipe_crc_source *source, u32 *val)
> > +			       enum intel_pipe_crc_source *source, u32 *val,
> > +			       bool set_wa)
> >  {
> >  	if (IS_GEN2(dev_priv))
> >  		return i8xx_pipe_crc_ctl_reg(source, val);
> > @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
> >  		return ilk_pipe_crc_ctl_reg(source, val);
> >  	else
> > -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> > +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> >  }
> >  
> >  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> > @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> >  		return -EIO;
> >  	}
> >  
> > -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> > +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
> >  	if (ret != 0)
> >  		goto out;
> >  
> > @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
> >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			      size_t *values_cnt)
> >  {
> > -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> >  	enum intel_display_power_domain power_domain;
> >  	enum intel_pipe_crc_source source;
> > @@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  		return -EIO;
> >  	}
> >  
> > -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> > +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
> >  	if (ret != 0)
> >  		goto out;
> >  
> > @@ -959,3 +961,31 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  
> >  	return ret;
> >  }
> > +
> > +void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	enum intel_pipe_crc_source source;
> > +	u32 val = 0;
> > +
> > +	if (!crtc->crc.opened)
> > +		return;
> > +
> > +	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
> > +		return;
> > +
> > +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
> > +		return;
> > +
> > +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > +}
> > +
> > +void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +
> > +	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
> > +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > +	synchronize_irq(dev_priv->drm.irq);
> > +}
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
  2018-03-07 12:10 Maarten Lankhorst
@ 2018-03-07 13:23 ` Ville Syrjälä
  2018-03-07 14:23   ` Ville Syrjälä
  0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-03-07 13:23 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Mar 07, 2018 at 01:10:26PM +0100, Maarten Lankhorst wrote:
> This will get rid of the following error:
> [   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
> [   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
> [   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
> [   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
> [   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
> [   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
> [   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
> [   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
> [   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
> [   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
> [   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
> [   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
> [   74.730382] Call Trace:
> [   74.730385]  <IRQ>
> [   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
> [   74.730401]  drm_update_vblank_count+0x64/0x240
> [   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
> [   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
> [   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
> [   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
> [   74.730548]  __handle_irq_event_percpu+0x3c/0x340
> [   74.730556]  handle_irq_event_percpu+0x1b/0x50
> [   74.730561]  handle_irq_event+0x2f/0x50
> [   74.730566]  handle_edge_irq+0xe4/0x1b0
> [   74.730572]  handle_irq+0x11/0x20
> [   74.730576]  do_IRQ+0x5e/0x120
> [   74.730584]  common_interrupt+0x84/0x84
> [   74.730586]  </IRQ>
> [   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
> [   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
> [   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
> [   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
> [   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
> [   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
> [   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
> [   74.730621]  ? cpuidle_enter_state+0xa6/0x350
> [   74.730629]  do_idle+0x188/0x1d0
> [   74.730636]  cpu_startup_entry+0x14/0x20
> [   74.730641]  start_secondary+0x129/0x160
> [   74.730646]  secondary_startup_64+0xa5/0xb0
> [   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
> [   74.730754] ---[ end trace 14b1345705b68565 ]---
> 
> Changes since v1:
> - Don't try to apply CRC workaround when enabling pipe, it should already be enabled.
> 
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h      |  3 +++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 44 +++++++++++++++++++++++++++++------
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 331084082545..67d13fe4c24f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12147,6 +12147,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	if (modeset) {
>  		update_scanline_offset(intel_crtc);
>  		dev_priv->display.crtc_enable(pipe_config, state);
> +		intel_crtc_enable_pipe_crc(crtc);

I think I'd prefer this to live next to the vblank on/off, with a
comment. Putting it here gives absolutely no clue why we have to
do it.

>  	} else {
>  		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>  				       pipe_config);
> @@ -12327,6 +12328,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  		if (old_crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> +			intel_crtc_disable_pipe_crc(crtc);
>  			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
>  			intel_crtc->active = false;
>  			intel_fbc_disable(intel_crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d4368589b355..002429957dd5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2138,8 +2138,11 @@ int intel_pipe_crc_create(struct drm_minor *minor);
>  #ifdef CONFIG_DEBUG_FS
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt);
> +void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc);
> +void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc);
>  #else
>  #define intel_crtc_set_crc_source NULL
> +#define intel_crtc_disable_pipe_crc(crtc) do {} while (0)
>  #endif
>  extern const struct file_operations i915_display_crc_ctl_fops;
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 1f5cd572a7ff..9422ed98e987 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				enum pipe pipe,
>  				enum intel_pipe_crc_source *source,
> -				uint32_t *val)
> +				uint32_t *val,
> +				bool set_wa)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>  		*source = INTEL_PIPE_CRC_SOURCE_PF;
> @@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
> -		if ((IS_HASWELL(dev_priv) ||
> +		if (set_wa && (IS_HASWELL(dev_priv) ||
>  		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
>  			hsw_pipe_A_crc_wa(dev_priv, true);
>  
> @@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
> -			       enum intel_pipe_crc_source *source, u32 *val)
> +			       enum intel_pipe_crc_source *source, u32 *val,
> +			       bool set_wa)
>  {
>  	if (IS_GEN2(dev_priv))
>  		return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
>  		return ilk_pipe_crc_ctl_reg(source, val);
>  	else
> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
>  }
>  
>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
> @@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt)
>  {
> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
>  	enum intel_display_power_domain power_domain;
>  	enum intel_pipe_crc_source source;
> @@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -959,3 +961,31 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  
>  	return ret;
>  }
> +
> +void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum intel_pipe_crc_source source;
> +	u32 val = 0;
> +
> +	if (!crtc->crc.opened)
> +		return;
> +
> +	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
> +		return;
> +
> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
> +		return;
> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +}
> +
> +void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +
> +	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
> +	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +	synchronize_irq(dev_priv->drm.irq);
> +}
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
@ 2018-03-07 12:10 Maarten Lankhorst
  2018-03-07 13:23 ` Ville Syrjälä
  0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-07 12:10 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Changes since v1:
- Don't try to apply CRC workaround when enabling pipe, it should already be enabled.

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h      |  3 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 44 +++++++++++++++++++++++++++++------
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..67d13fe4c24f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+		intel_crtc_enable_pipe_crc(crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12328,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+			intel_crtc_disable_pipe_crc(crtc);
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..002429957dd5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,11 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+#define intel_crtc_disable_pipe_crc(crtc) do {} while (0)
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..9422ed98e987 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -569,7 +569,8 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				uint32_t *val)
+				uint32_t *val,
+				bool set_wa)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PF;
@@ -582,7 +583,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
-		if ((IS_HASWELL(dev_priv) ||
+		if (set_wa && (IS_HASWELL(dev_priv) ||
 		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
 			hsw_pipe_A_crc_wa(dev_priv, true);
 
@@ -600,7 +601,8 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val)
+			       enum intel_pipe_crc_source *source, u32 *val,
+			       bool set_wa)
 {
 	if (IS_GEN2(dev_priv))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -611,7 +613,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
 }
 
 static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
@@ -636,7 +638,7 @@ static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -916,7 +918,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -934,7 +936,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
 	if (ret != 0)
 		goto out;
 
@@ -959,3 +961,31 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, false) < 0)
+		return;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.
@ 2018-03-07  8:17 Maarten Lankhorst
  0 siblings, 0 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2018-03-07  8:17 UTC (permalink / raw)
  To: intel-gfx

This will get rid of the following error:
[   74.730271] WARNING: CPU: 4 PID: 0 at drivers/gpu/drm/drm_vblank.c:614 drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730311] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel crct10dif_pclmul snd_hda_codec crc32_pclmul snd_hwdep broadcom ghash_clmulni_intel snd_hda_core bcm_phy_lib snd_pcm tg3 lpc_ich mei_me mei prime_numbers
[   74.730353] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G     U           4.16.0-rc2-CI-CI_DRM_3822+ #1
[   74.730355] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
[   74.730359] RIP: 0010:drm_calc_vbltimestamp_from_scanoutpos+0x13e/0x2f0
[   74.730361] RSP: 0018:ffff88022fb03d10 EFLAGS: 00010086
[   74.730365] RAX: ffffffffa0291d20 RBX: ffff88021a180000 RCX: 0000000000000001
[   74.730367] RDX: ffffffff820e7db8 RSI: 0000000000000001 RDI: ffffffff82068cea
[   74.730369] RBP: ffff88022fb03d70 R08: 0000000000000000 R09: ffffffff815d26d0
[   74.730371] R10: 0000000000000000 R11: ffffffffa0161ca0 R12: 0000000000000001
[   74.730373] R13: ffff880212448008 R14: ffff880212448330 R15: 0000000000000000
[   74.730376] FS:  0000000000000000(0000) GS:ffff88022fb00000(0000) knlGS:0000000000000000
[   74.730378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.730380] CR2: 000055edcbec9000 CR3: 0000000002210001 CR4: 00000000000606e0
[   74.730382] Call Trace:
[   74.730385]  <IRQ>
[   74.730397]  drm_get_last_vbltimestamp+0x36/0x50
[   74.730401]  drm_update_vblank_count+0x64/0x240
[   74.730409]  drm_crtc_accurate_vblank_count+0x41/0x90
[   74.730453]  display_pipe_crc_irq_handler+0x176/0x220 [i915]
[   74.730497]  i9xx_pipe_crc_irq_handler+0xfe/0x150 [i915]
[   74.730537]  ironlake_irq_handler+0x618/0xa30 [i915]
[   74.730548]  __handle_irq_event_percpu+0x3c/0x340
[   74.730556]  handle_irq_event_percpu+0x1b/0x50
[   74.730561]  handle_irq_event+0x2f/0x50
[   74.730566]  handle_edge_irq+0xe4/0x1b0
[   74.730572]  handle_irq+0x11/0x20
[   74.730576]  do_IRQ+0x5e/0x120
[   74.730584]  common_interrupt+0x84/0x84
[   74.730586]  </IRQ>
[   74.730591] RIP: 0010:cpuidle_enter_state+0xaa/0x350
[   74.730593] RSP: 0018:ffffc9000008beb8 EFLAGS: 00000212 ORIG_RAX: ffffffffffffffde
[   74.730597] RAX: ffff880226b80040 RBX: 000000000031fc3e RCX: 0000000000000001
[   74.730599] RDX: 0000000000000000 RSI: ffffffff8210fb59 RDI: ffffffff820c02e7
[   74.730601] RBP: 0000000000000004 R08: 00000000000040af R09: 0000000000000018
[   74.730603] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[   74.730606] R13: ffffe8ffffd00430 R14: 0000001166120bf4 R15: ffffffff82294460
[   74.730621]  ? cpuidle_enter_state+0xa6/0x350
[   74.730629]  do_idle+0x188/0x1d0
[   74.730636]  cpu_startup_entry+0x14/0x20
[   74.730641]  start_secondary+0x129/0x160
[   74.730646]  secondary_startup_64+0xa5/0xb0
[   74.730660] Code: e1 48 c7 c2 b8 7d 0e 82 be 01 00 00 00 48 c7 c7 ea 8c 06 82 e8 64 ec ff ff 48 8b 83 c8 07 00 00 48 83 78 28 00 0f 84 e2 fe ff ff <0f> 0b 45 31 ed e9 db fe ff ff 41 b8 d3 4d 62 10 89 c8 6a 03 41
[   74.730754] ---[ end trace 14b1345705b68565 ]---

Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h      |  3 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 30 +++++++++++++++++++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331084082545..67d13fe4c24f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12147,6 +12147,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
+		intel_crtc_enable_pipe_crc(crtc);
 	} else {
 		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
 				       pipe_config);
@@ -12327,6 +12328,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+			intel_crtc_disable_pipe_crc(crtc);
 			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4368589b355..002429957dd5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2138,8 +2138,11 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc);
+void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+#define intel_crtc_disable_pipe_crc(crtc) do {} while (0)
 #endif
 extern const struct file_operations i915_display_crc_ctl_fops;
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1f5cd572a7ff..eca391022edc 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -916,7 +916,7 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
 	enum intel_display_power_domain power_domain;
 	enum intel_pipe_crc_source source;
@@ -959,3 +959,31 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 	return ret;
 }
+
+void intel_crtc_enable_pipe_crc(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+	u32 val = 0;
+
+	if (!crtc->crc.opened)
+		return;
+
+	if (display_crc_ctl_parse_source(crtc->crc.source, &source) < 0)
+		return;
+
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val) < 0)
+		return;
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+}
+
+void intel_crtc_disable_pipe_crc(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+
+	I915_WRITE(PIPE_CRC_CTL(crtc->index), 0);
+	POSTING_READ(PIPE_CRC_CTL(crtc->index));
+	synchronize_irq(dev_priv->drm.irq);
+}
-- 
2.16.2

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-03-08 16:38 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-08 12:02 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
2018-03-08 12:20 ` ✓ Fi.CI.BAT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
2018-03-08 15:22 ` [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Ville Syrjälä
2018-03-08 16:02   ` Maarten Lankhorst
2018-03-08 16:21     ` Ville Syrjälä
2018-03-08 16:38       ` Maarten Lankhorst
2018-03-08 15:25 ` ✓ Fi.CI.IGT: success for drm/i915: Handle pipe CRC around enabling/disabling pipe. (rev6) Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2018-03-08  9:17 [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe Maarten Lankhorst
2018-03-08  9:21 ` Maarten Lankhorst
2018-03-07 12:10 Maarten Lankhorst
2018-03-07 13:23 ` Ville Syrjälä
2018-03-07 14:23   ` Ville Syrjälä
2018-03-07 15:04     ` Maarten Lankhorst
2018-03-07 15:21       ` Ville Syrjälä
2018-03-07 16:40         ` Maarten Lankhorst
2018-03-07  8:17 Maarten Lankhorst

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.