On Tue, Mar 08, 2016 at 10:57:16AM +0100, Maarten Lankhorst wrote:
> 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/0x70
>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Testcase: kms_universal_plane
> Fixes: d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cd0b4eacbddf..28318425413a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f65e84137060..d7aef17bf0f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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;
> - } 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ä <ville.syrjala@xxxxxxxxxxxxxxx>
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.
> }
>
> return 0;
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
