[PATCH] drm/i915: Only use sanitized values for ILK watermarks

[PATCH] drm/i915: Only use sanitized values for ILK watermarks

[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/0x70

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Testcase: kms_universal_plane
Fixes: d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
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_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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Only use sanitized values for ILK watermarks

[Ville Syrjälä]

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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Testcase: kms_universal_plane
> Fixes: d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> 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@linux.intel.com>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx