All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	"Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH] drm/i915: Only use sanitized values for ILK watermarks
Date: Tue, 8 Mar 2016 16:33:22 +0200	[thread overview]
Message-ID: <20160308143322.GN10446@intel.com> (raw)
In-Reply-To: <56DEA1FC.8080703@linux.intel.com>

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

      reply	other threads:[~2016-03-08 14:33 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-08  9:57 [PATCH] drm/i915: Only use sanitized values for ILK watermarks Maarten Lankhorst
2016-03-08 14:33 ` Ville Syrjälä [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160308143322.GN10446@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=paulo.r.zanoni@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.