drm/i915: Only use sanitized values for ILK watermarks
Subsystems (2)
PATCH 1/1 drm/i915: Only use sanitized values for ILK watermarks
The raw watermark values are needed when planes are not part of the state, but this introduced a regression and possibly an overflow when merging the watermarks because invalid values may end up used. Solve this by calculating raw watermarks for all levels, and only setting non-zero values when the level is valid.
Fixes the SNB warning:
WARNING: CPU: 1 PID: 25405 at /home/mattrope/work/kernel/gms/drivers/gpu/drm/i915/intel_pm.c:2580 ilk_program_watermarks+0x7b2/0x9d0 [i915]()
WARN_ON(wm_lp != 1)
Modules linked in: i915 drm_kms_helper drm bluetooth fuse iTCO_wdt iTCO_vendor_support syscopyarea sysfillrect sysimgblt fb_sys_fops tpm_tis mei_me e1000e snd_hda_codec_hdmi pcspkr tpm mei i2c_i801 lpc_ich snd_hda_codec snd_hda_core
CPU: 1 PID: 25405 Comm: kms_universal_p Tainted: G U W 4.5.0-rc6apollolake+ #462
Hardware name: /DH67GD, BIOS BLH6710H.86A.0160.2012.1204.1156 12/04/2012
0000000000000000 ffff88009d42b918 ffffffff8143cfab ffff88009d42b960
ffffffffa0363580 ffff88009d42b950 ffffffff81082746 ffff8800b9a24928
ffff88009d42ba00 ffff88009d4a0000 0000000000000000 ffff88009d42ba6c
Call Trace:
[<ffffffff8143cfab>] dump_stack+0x4d/0x72
[<ffffffff81082746>] warn_slowpath_common+0x86/0xc0
[<ffffffff810827cc>] warn_slowpath_fmt+0x4c/0x50
[<ffffffffa0292862>] ilk_program_watermarks+0x7b2/0x9d0 [i915]
[<ffffffffa0292cb7>] ilk_initial_watermarks+0x107/0x120 [i915]
[<ffffffffa02feffa>] intel_pre_plane_update+0x12a/0x190 [i915]
[<ffffffffa02ffb36>] intel_atomic_commit+0x546/0xd50 [i915]
[<ffffffffa012c9e7>] drm_atomic_commit+0x37/0x60 [drm]
[<ffffffffa0217361>] drm_atomic_helper_disable_plane+0xb1/0xf0 [drm_kms_helper]
[<ffffffffa011cdb4>] __setplane_internal+0x184/0x280 [drm]
[<ffffffffa012b57a>] ? drm_modeset_lock_all_ctx+0x9a/0xb0 [drm]
[<ffffffffa012010f>] drm_mode_setplane+0x13f/0x1c0 [drm]
[<ffffffffa0111b52>] drm_ioctl+0x142/0x590 [drm]
[<ffffffffa011ffd0>] ? drm_plane_check_pixel_format+0x50/0x50 [drm]
[<ffffffff811f2744>] ? mntput+0x24/0x40
[<ffffffff811d28d4>] ? __fput+0x194/0x200
[<ffffffffa012dec3>] drm_compat_ioctl+0x33/0x40 [drm]
[<ffffffffa029e1c2>] i915_compat_ioctl+0x32/0x40 [i915]
[<ffffffff81228d72>] compat_SyS_ioctl+0xc2/0x330
[<ffffffff810021d5>] ? exit_to_usermode_loop+0x95/0xb0
[<ffffffff81002d2e>] do_fast_syscall_32+0x9e/0x210
[<ffffffff8197faf2>] entry_SYSENTER_compat+0x52/0x70Cc: Matt Roper <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Testcase: kms_universal_plane
Fixes d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_drv.h +1 -0
@@ -390,6 +390,7 @@ struct intel_crtc_scaler_state {struct intel_pipe_wm {struct intel_wm_level wm[5 ]; +struct intel_wm_level raw_wm[5 ];uint32_t linetime;bool fbc_wm_enabled;bool pipe_enabled;
drivers/gpu/drm/i915/intel_pm.c +11 -8
@@ -2335,7 +2335,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16); } - usable_level = max_level;/* ILK/SNB: LP2+ watermarks only w/o sprites */
@@ -2347,7 +2346,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) usable_level =0 ;ilk_compute_wm_level (dev_priv, intel_crtc,0 , cstate, - pristate, sprstate, curstate, &pipe_wm->wm[0 ]); + pristate, sprstate, curstate, &pipe_wm->raw_wm[0 ]); + +memset (&pipe_wm->wm,0 ,sizeof (pipe_wm->wm)); + pipe_wm->wm[0 ] = pipe_wm->raw_wm[0 ];if (IS_HASWELL (dev) ||IS_BROADWELL (dev)) pipe_wm->linetime =hsw_compute_linetime_wm (dev, cstate);
@@ -2358,7 +2360,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)ilk_compute_wm_reg_maximums (dev,1 , &max);for (level =1 ; level <= max_level; level++) { -struct intel_wm_level *wm = &pipe_wm->wm[level]; +struct intel_wm_level *wm = &pipe_wm->raw_wm[level];ilk_compute_wm_level (dev_priv, intel_crtc, level, cstate, pristate, sprstate, curstate, wm);
@@ -2368,12 +2370,13 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)* register maximums since such watermarks are * always invalid. */ -if (level > usable_level) { - wm->enable = false; - }else if (!ilk_validate_wm_level (level, &max, wm)) { - wm->enable = false; +if (level > usable_level) +continue ; + +if (ilk_validate_wm_level (level, &max, wm)) + pipe_wm->wm[level] = *wm; +else usable_level = level; - } }return 0 ;
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 98 lines · Maarten Lankhorst
The raw watermark values are needed when planes are not part of the state, but this introduced a regression and possibly an overflow when merging the watermarks because invalid values may end up used. Solve this by calculating raw watermarks for all levels, and only setting non-zero values when the level is valid.
Fixes the SNB warning:
WARNING: CPU: 1 PID: 25405 at /home/mattrope/work/kernel/gms/drivers/gpu/drm/i915/intel_pm.c:2580 ilk_program_watermarks+0x7b2/0x9d0 [i915]() WARN_ON(wm_lp != 1) Modules linked in: i915 drm_kms_helper drm bluetooth fuse iTCO_wdt iTCO_vendor_support syscopyarea sysfillrect sysimgblt fb_sys_fops tpm_tis mei_me e1000e snd_hda_codec_hdmi pcspkr tpm mei i2c_i801 lpc_ich snd_hda_codec snd_hda_core CPU: 1 PID: 25405 Comm: kms_universal_p Tainted: G U W 4.5.0-rc6apollolake+ #462 Hardware name: /DH67GD, BIOS BLH6710H.86A.0160.2012.1204.1156 12/04/2012 0000000000000000 ffff88009d42b918 ffffffff8143cfab ffff88009d42b960 ffffffffa0363580 ffff88009d42b950 ffffffff81082746 ffff8800b9a24928 ffff88009d42ba00 ffff88009d4a0000 0000000000000000 ffff88009d42ba6c Call Trace: [<ffffffff8143cfab>] dump_stack+0x4d/0x72 [<ffffffff81082746>] warn_slowpath_common+0x86/0xc0 [<ffffffff810827cc>] warn_slowpath_fmt+0x4c/0x50 [<ffffffffa0292862>] ilk_program_watermarks+0x7b2/0x9d0 [i915] [<ffffffffa0292cb7>] ilk_initial_watermarks+0x107/0x120 [i915] [<ffffffffa02feffa>] intel_pre_plane_update+0x12a/0x190 [i915] [<ffffffffa02ffb36>] intel_atomic_commit+0x546/0xd50 [i915] [<ffffffffa012c9e7>] drm_atomic_commit+0x37/0x60 [drm] [<ffffffffa0217361>] drm_atomic_helper_disable_plane+0xb1/0xf0 [drm_kms_helper] [<ffffffffa011cdb4>] __setplane_internal+0x184/0x280 [drm] [<ffffffffa012b57a>] ? drm_modeset_lock_all_ctx+0x9a/0xb0 [drm] [<ffffffffa012010f>] drm_mode_setplane+0x13f/0x1c0 [drm] [<ffffffffa0111b52>] drm_ioctl+0x142/0x590 [drm] [<ffffffffa011ffd0>] ? drm_plane_check_pixel_format+0x50/0x50 [drm] [<ffffffff811f2744>] ? mntput+0x24/0x40 [<ffffffff811d28d4>] ? __fput+0x194/0x200 [<ffffffffa012dec3>] drm_compat_ioctl+0x33/0x40 [drm] [<ffffffffa029e1c2>] i915_compat_ioctl+0x32/0x40 [i915] [<ffffffff81228d72>] compat_SyS_ioctl+0xc2/0x330 [<ffffffff810021d5>] ? exit_to_usermode_loop+0x95/0xb0 [<ffffffff81002d2e>] do_fast_syscall_32+0x9e/0x210 [<ffffffff8197faf2>] entry_SYSENTER_compat+0x52/0x70Cc: Matt Roper <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Testcase:kms_universal_planeFixes d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")Signed-off-by Maarten Lankhorst <[email protected]>---
drivers/gpu/drm/i915/intel_drv.h +1
-0@@ -390,6 +390,7 @@ struct intel_crtc_scaler_state {struct intel_pipe_wm {struct intel_wm_level wm[5 ]; +struct intel_wm_level raw_wm[5 ];uint32_t linetime;bool fbc_wm_enabled;bool pipe_enabled;drivers/gpu/drm/i915/intel_pm.c +5
-5@@ -2335,7 +2335,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16); } - usable_level = max_level;/* ILK/SNB: LP2+ watermarks only w/o sprites */ @@ -2347,7 +2346,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) usable_level =0 ;ilk_compbute_wm_level (dev_priv, intel_crtc,0 , cstate, - pristate, sprstate, curstate, &pipe_wm->wm[0 ]); + pristate, sprstate, curstate, &pipe_wm->raw_wm[0 ]); + +memset (&pipe_wm->wm,0 ,sizeof (pipe_wm->wm)); + pipe_wm->wm[0 ] = pipe_wm->raw_wm[0 ];if (IS_HASWELL (dev) ||IS_BROADWELL (dev)) pipe_wm->linetime =hsw_compute_linetime_wm (dev, cstate);@@ -2358,7 +2360,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)ilk_compute_wm_reg_maximums (dev,1 , &max);for (level =1 ; level <= max_level; level++) { -struct intel_wm_level *wm = &pipe_wm->wm[level]; +struct intel_wm_level *wm = &pipe_wm->raw_wm[level];ilk_compute_wm_level (dev_priv, intel_crtc, level, cstate, pristate, sprstate, curstate, wm);@@ -2368,12 +2370,13 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)* register maximums since such watermarks are * always invalid. */ -if (level > usable_level) { - wm->enable = false;
drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) · line 2376
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +6
-3- }else if (!ilk_validate_wm_level (level, &max, wm)) { - wm->enable = false; +if (level > usable_level) +continue ; + +if (ilk_validate_wm_level (level, &max, wm)) + pipe_wm->wm[level] = *wm; +else usable_level = level; - }
Yeah, that looks like it ought to work. raw_wm gets overwritten only for the planes that changed, and all disabled levels in pipe_wm will be zeroed in the end.
And sprites_enabled/scaled will be preserved in pipe_wm until the sprite gets enabled/disabled.
Reviewed-by Ville Syrjälä <[email protected]>
There is a bit of useless gunk in raw_wm now. So might make sense to not use intel_pipe_wm for that. Also I'm not sure it wouldn't be better to do the calc stuff in a more plane oriented way instead of passsing having to pass around all the plane states. But these could be left to some future cleanups.
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +0
-0}return 0 ;
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx