Re: [PATCH] drm/i915: Handle pipe CRC around enabling/disabling pipe.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
>>>> Reported-by: Marta Löfstedt <marta.lofstedt@xxxxxxxxx>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105185
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux