| Message ID | 20180308120202.52446-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 :/
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.
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); +}
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(-)