* [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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2018-03-08 16:38 UTC | newest]
Thread overview: 7+ 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
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.