[PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Maarten Lankhorst]

If an atomic update fails intel_crtc->atomic may have have some values left
from the last atomic check. One example is atomic->wait_for_vblank,
which results in spurious errors in kms_flip.

Testcase: kms_flip
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org #v4.3
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493213b7..b9539b14fe0d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13141,6 +13141,9 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
+		memset(&to_intel_crtc(crtc)->atomic, 0,
+		       sizeof(struct intel_crtc_atomic_commit));
+
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;

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

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Daniel Stone]

Hi,

On 16 November 2015 at 11:49, Maarten Lankhorst <maarten@mblankhorst.nl> wrote:
> If an atomic update fails intel_crtc->atomic may have have some values left
> from the last atomic check. One example is atomic->wait_for_vblank,
> which results in spurious errors in kms_flip.
>
> Testcase: kms_flip
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.3
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..b9539b14fe0d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13141,6 +13141,9 @@ static int intel_atomic_check(struct drm_device *dev,
>                 struct intel_crtc_state *pipe_config =
>                         to_intel_crtc_state(crtc_state);
>
> +               memset(&to_intel_crtc(crtc)->atomic, 0,
> +                      sizeof(struct intel_crtc_atomic_commit));
> +
>                 /* Catch I915_MODE_FLAG_INHERITED */
>                 if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
>                         crtc_state->mode_changed = true;

How about doing it in intel_crtc_duplicate_state instead? Should be a
bit more bulletproof/correct against various other failure modes. But
I guess this does work, and it is all going away shortly, so:
Reviewed-by: Daniel Stone <daniels@collabora.com>

I think the reason we haven't been bitten by cdclk failures inside
intel_atomic_state.atomic, is because intel_atomic_state always starts
with a zeroed structure.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Jani Nikula]

On Mon, 16 Nov 2015, Maarten Lankhorst <maarten@mblankhorst.nl> wrote:
> If an atomic update fails intel_crtc->atomic may have have some values left
> from the last atomic check. One example is atomic->wait_for_vblank,
> which results in spurious errors in kms_flip.

Please add the "spurious errors" you see in the commit message, so we
have a better chance at mapping bug reports to fixes. We used to be good
at this, but we seem to have lost the habit lately.

BR,
Jani.


>
> Testcase: kms_flip
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.3
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..b9539b14fe0d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13141,6 +13141,9 @@ static int intel_atomic_check(struct drm_device *dev,
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
>  
> +		memset(&to_intel_crtc(crtc)->atomic, 0,
> +		       sizeof(struct intel_crtc_atomic_commit));
> +
>  		/* Catch I915_MODE_FLAG_INHERITED */
>  		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
>  			crtc_state->mode_changed = true;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Maarten Lankhorst]

Op 16-11-15 om 13:35 schreef Jani Nikula:
> On Mon, 16 Nov 2015, Maarten Lankhorst <maarten@mblankhorst.nl> wrote:
>> If an atomic update fails intel_crtc->atomic may have have some values left
>> from the last atomic check. One example is atomic->wait_for_vblank,
>> which results in spurious errors in kms_flip.
> Please add the "spurious errors" you see in the commit message, so we
> have a better chance at mapping bug reports to fixes. We used to be good
> at this, but we seem to have lost the habit lately.
>
Debian pastebin had an error but the paste expired. :/

Ville do you still have it?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Ville Syrjälä]

On Mon, Nov 16, 2015 at 01:41:54PM +0100, Maarten Lankhorst wrote:
> Op 16-11-15 om 13:35 schreef Jani Nikula:
> > On Mon, 16 Nov 2015, Maarten Lankhorst <maarten@mblankhorst.nl> wrote:
> >> If an atomic update fails intel_crtc->atomic may have have some values left
> >> from the last atomic check. One example is atomic->wait_for_vblank,
> >> which results in spurious errors in kms_flip.
> > Please add the "spurious errors" you see in the commit message, so we
> > have a better chance at mapping bug reports to fixes. We used to be good
> > at this, but we seem to have lost the habit lately.
> >
> Debian pastebin had an error but the paste expired. :/
> 
> Ville do you still have it?

Here's one of them:

[ 1551.892708] ------------[ cut here ]------------
[ 1551.892721] WARNING: CPU: 3 PID: 4179 at ../drivers/gpu/drm/drm_irq.c:1199 drm_wait_one_vblank+0x197/0x1a0 [drm]()
[ 1551.892722] vblank not available on crtc 2, ret=-22
[ 1551.892751] Modules linked in: snd_hda_intel i915 drm_kms_helper drm intel_gtt i2c_algo_bit cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea agpgart cfg80211 binfmt_misc snd_hda_codec_hdmi intel_rapl iosf_mbi x86_pkg_temp_thermal coretemp kvm_intel snd_hda_codec_realtek kvm snd_hda_codec_generic iTCO_wdt aesni_intel aes_x86_64 glue_helper lrw snd_hda_codec gf128mul ablk_helper cryptd snd_hwdep psmouse snd_hda_core pcspkr snd_pcm snd_timer snd lpc_ich i2c_i801 mfd_core soundcore wmi evdev [last unloaded: drm]
[ 1551.892753] CPU: 3 PID: 4179 Comm: kms_pipe_crc_ba Tainted: G     U  W       4.3.0-reg+ #6
[ 1551.892754] Hardware name:                  /DZ77BH-55K, BIOS BHZ7710H.86A.0100.2013.0517.0942 05/17/2013
[ 1551.892758]  ffffffffa03128d8 ffff8800cec73890 ffffffff812c0f3c ffff8800cec738d8
[ 1551.892760]  ffff8800cec738c8 ffffffff8104ff36 ffff880116ae2290 0000000000000002
[ 1551.892762]  ffff8800d39fcda0 ffff8800d038b4d0 ffff8800d42b5550 ffff8800cec73928
[ 1551.892763] Call Trace:
[ 1551.892768]  [<ffffffff812c0f3c>] dump_stack+0x4e/0x82
[ 1551.892771]  [<ffffffff8104ff36>] warn_slowpath_common+0x86/0xc0
[ 1551.892773]  [<ffffffff8104ffbc>] warn_slowpath_fmt+0x4c/0x50
[ 1551.892781]  [<ffffffffa02e6708>] ? drm_vblank_get+0x78/0xd0 [drm]
[ 1551.892787]  [<ffffffffa02e6d47>] drm_wait_one_vblank+0x197/0x1a0 [drm]
[ 1551.892813]  [<ffffffffa03d052f>] intel_post_plane_update+0xef/0x120 [i915]
[ 1551.892832]  [<ffffffffa03d11d2>] intel_atomic_commit+0x4c2/0x1600 [i915]
[ 1551.892862]  [<ffffffffa02ff0c7>] ? drm_atomic_check_only+0x147/0x5e0 [drm]
[ 1551.892872]  [<ffffffffa02feeb7>] ? drm_atomic_add_affected_connectors+0x27/0xf0 [drm]
[ 1551.892881]  [<ffffffffa02ff597>] drm_atomic_commit+0x37/0x60 [drm]
[ 1551.892887]  [<ffffffffa034301a>] restore_fbdev_mode+0x28a/0x2c0 [drm_kms_helper]
[ 1551.892895]  [<ffffffffa0345253>] drm_fb_helper_restore_fbdev_mode_unlocked+0x33/0x80 [drm_kms_helper]
[ 1551.892900]  [<ffffffffa03452cd>] drm_fb_helper_set_par+0x2d/0x50 [drm_kms_helper]
[ 1551.892920]  [<ffffffffa03e7a9a>] intel_fbdev_set_par+0x1a/0x60 [i915]
[ 1551.892923]  [<ffffffff8131a5a7>] fb_set_var+0x1a7/0x3f0
[ 1551.892927]  [<ffffffff8109732f>] ? trace_hardirqs_on_caller+0x12f/0x1c0
[ 1551.892931]  [<ffffffff81314f32>] fbcon_blank+0x212/0x2f0
[ 1551.892935]  [<ffffffff81373f4a>] do_unblank_screen+0xba/0x1d0
[ 1551.892937]  [<ffffffff8136b725>] vt_ioctl+0x13d5/0x1450
[ 1551.892940]  [<ffffffff8107cdd1>] ? preempt_count_sub+0x41/0x50
[ 1551.892943]  [<ffffffff8135d8a3>] tty_ioctl+0x423/0xe30
[ 1551.892947]  [<ffffffff8119f721>] do_vfs_ioctl+0x301/0x560
[ 1551.892949]  [<ffffffff8119b1e3>] ? putname+0x53/0x60
[ 1551.892952]  [<ffffffff811ab376>] ? __fget_light+0x66/0x90
[ 1551.892955]  [<ffffffff8119f9f9>] SyS_ioctl+0x79/0x90
[ 1551.892958]  [<ffffffff81552e97>] entry_SYSCALL_64_fastpath+0x12/0x6f
[ 1551.892961] ---[ end trace 3e764d4b6628c91c ]---

And my t-b was replied to the other mail.

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

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Jani Nikula]

On Mon, 16 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Nov 16, 2015 at 01:41:54PM +0100, Maarten Lankhorst wrote:
>> Op 16-11-15 om 13:35 schreef Jani Nikula:
>> > On Mon, 16 Nov 2015, Maarten Lankhorst <maarten@mblankhorst.nl> wrote:
>> >> If an atomic update fails intel_crtc->atomic may have have some values left
>> >> from the last atomic check. One example is atomic->wait_for_vblank,
>> >> which results in spurious errors in kms_flip.
>> > Please add the "spurious errors" you see in the commit message, so we
>> > have a better chance at mapping bug reports to fixes. We used to be good
>> > at this, but we seem to have lost the habit lately.
>> >
>> Debian pastebin had an error but the paste expired. :/
>> 
>> Ville do you still have it?
>
> Here's one of them:
>
> [ 1551.892708] ------------[ cut here ]------------
> [ 1551.892721] WARNING: CPU: 3 PID: 4179 at ../drivers/gpu/drm/drm_irq.c:1199 drm_wait_one_vblank+0x197/0x1a0 [drm]()
> [ 1551.892722] vblank not available on crtc 2, ret=-22
> [ 1551.892751] Modules linked in: snd_hda_intel i915 drm_kms_helper drm intel_gtt i2c_algo_bit cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea agpgart cfg80211 binfmt_misc snd_hda_codec_hdmi intel_rapl iosf_mbi x86_pkg_temp_thermal coretemp kvm_intel snd_hda_codec_realtek kvm snd_hda_codec_generic iTCO_wdt aesni_intel aes_x86_64 glue_helper lrw snd_hda_codec gf128mul ablk_helper cryptd snd_hwdep psmouse snd_hda_core pcspkr snd_pcm snd_timer snd lpc_ich i2c_i801 mfd_core soundcore wmi evdev [last unloaded: drm]
> [ 1551.892753] CPU: 3 PID: 4179 Comm: kms_pipe_crc_ba Tainted: G     U  W       4.3.0-reg+ #6
> [ 1551.892754] Hardware name:                  /DZ77BH-55K, BIOS BHZ7710H.86A.0100.2013.0517.0942 05/17/2013
> [ 1551.892758]  ffffffffa03128d8 ffff8800cec73890 ffffffff812c0f3c ffff8800cec738d8
> [ 1551.892760]  ffff8800cec738c8 ffffffff8104ff36 ffff880116ae2290 0000000000000002
> [ 1551.892762]  ffff8800d39fcda0 ffff8800d038b4d0 ffff8800d42b5550 ffff8800cec73928
> [ 1551.892763] Call Trace:
> [ 1551.892768]  [<ffffffff812c0f3c>] dump_stack+0x4e/0x82
> [ 1551.892771]  [<ffffffff8104ff36>] warn_slowpath_common+0x86/0xc0
> [ 1551.892773]  [<ffffffff8104ffbc>] warn_slowpath_fmt+0x4c/0x50
> [ 1551.892781]  [<ffffffffa02e6708>] ? drm_vblank_get+0x78/0xd0 [drm]
> [ 1551.892787]  [<ffffffffa02e6d47>] drm_wait_one_vblank+0x197/0x1a0 [drm]
> [ 1551.892813]  [<ffffffffa03d052f>] intel_post_plane_update+0xef/0x120 [i915]
> [ 1551.892832]  [<ffffffffa03d11d2>] intel_atomic_commit+0x4c2/0x1600 [i915]
> [ 1551.892862]  [<ffffffffa02ff0c7>] ? drm_atomic_check_only+0x147/0x5e0 [drm]
> [ 1551.892872]  [<ffffffffa02feeb7>] ? drm_atomic_add_affected_connectors+0x27/0xf0 [drm]
> [ 1551.892881]  [<ffffffffa02ff597>] drm_atomic_commit+0x37/0x60 [drm]
> [ 1551.892887]  [<ffffffffa034301a>] restore_fbdev_mode+0x28a/0x2c0 [drm_kms_helper]
> [ 1551.892895]  [<ffffffffa0345253>] drm_fb_helper_restore_fbdev_mode_unlocked+0x33/0x80 [drm_kms_helper]
> [ 1551.892900]  [<ffffffffa03452cd>] drm_fb_helper_set_par+0x2d/0x50 [drm_kms_helper]
> [ 1551.892920]  [<ffffffffa03e7a9a>] intel_fbdev_set_par+0x1a/0x60 [i915]
> [ 1551.892923]  [<ffffffff8131a5a7>] fb_set_var+0x1a7/0x3f0
> [ 1551.892927]  [<ffffffff8109732f>] ? trace_hardirqs_on_caller+0x12f/0x1c0
> [ 1551.892931]  [<ffffffff81314f32>] fbcon_blank+0x212/0x2f0
> [ 1551.892935]  [<ffffffff81373f4a>] do_unblank_screen+0xba/0x1d0
> [ 1551.892937]  [<ffffffff8136b725>] vt_ioctl+0x13d5/0x1450
> [ 1551.892940]  [<ffffffff8107cdd1>] ? preempt_count_sub+0x41/0x50
> [ 1551.892943]  [<ffffffff8135d8a3>] tty_ioctl+0x423/0xe30
> [ 1551.892947]  [<ffffffff8119f721>] do_vfs_ioctl+0x301/0x560
> [ 1551.892949]  [<ffffffff8119b1e3>] ? putname+0x53/0x60
> [ 1551.892952]  [<ffffffff811ab376>] ? __fget_light+0x66/0x90
> [ 1551.892955]  [<ffffffff8119f9f9>] SyS_ioctl+0x79/0x90
> [ 1551.892958]  [<ffffffff81552e97>] entry_SYSCALL_64_fastpath+0x12/0x6f
> [ 1551.892961] ---[ end trace 3e764d4b6628c91c ]---
>
> And my t-b was replied to the other mail.

Pushed to drm-intel-fixes, thanks for the patch, review and testing.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear intel_crtc->atomic before updating it.

[Ville Syrjälä]

On Mon, Nov 16, 2015 at 12:50:13PM +0100, Maarten Lankhorst wrote:
> If an atomic update fails intel_crtc->atomic may have have some values left
> from the last atomic check. One example is atomic->wait_for_vblank,
> which results in spurious errors in kms_flip.
> 
> Testcase: kms_flip
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.3
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..b9539b14fe0d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13141,6 +13141,9 @@ static int intel_atomic_check(struct drm_device *dev,
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
>  
> +		memset(&to_intel_crtc(crtc)->atomic, 0,
> +		       sizeof(struct intel_crtc_atomic_commit));

sizeof(to_intel_crtc(crtc)->atomic) would be better.

This cures a ton of "wait_for_vblank called with a disabled pipe"
type of errors.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  		/* Catch I915_MODE_FLAG_INHERITED */
>  		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
>  			crtc_state->mode_changed = true;

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