On Fri, Mar 04, 2016 at 09:37:40AM -0800, Matt Roper wrote:
> On Fri, Mar 04, 2016 at 12:45:38PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 03, 2016 at 02:18:09PM -0800, Matt Roper wrote:
> > > As of commit d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
> > > it's now possible to have non-zero values for watermark levels that are
> > > disabled (e.g., in the higher LP levels that can only be used when the sprite
> > > is disabled). Stop WARN()'ing when we see these non-zero sprite values in LP2
> > > or LP3.
> > >
> > > 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 [last unloaded: drm]
> > > 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: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > 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: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > > The comment above this block of code indicates that we need to turn on
> > > WM1S_LP_EN, even if the LP level is disabled, to avoid underruns. It's not
> > > clear to me from the bspec why this is, so I'm trusting that the comment
> > > is still accurate, even though we wind up with non-zero values more often
> > > now than we previously did.
> > >
> > > drivers/gpu/drm/i915/intel_pm.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 823437f..9698360 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2576,10 +2576,9 @@ static void ilk_compute_wm_results(struct drm_device *dev,
> > > * Always set WM1S_LP_EN when spr_val != 0, even if the
> > > * level is disabled. Doing otherwise could cause underruns.
> > > */
> > > - if (INTEL_INFO(dev)->gen <= 6 && r->spr_val) {
> > > - WARN_ON(wm_lp != 1);
> >
> > NAK
> >
> > There are no LP2+ sprite watermarks on ilk/snb, so if something is
> > computing it, then we have clearly a bug somewhere else.
>
> That's the point of this patch...having these values computed is
> intentional now, not a bug. The patch from Maarten/Paaulo computes the
> values for *all* levels, even the ones we know are invalid, but marks
> them as disabled.
Then I think it should compute it as 0 since sprite LP2+ watermarks simply
don't exist.
>
> Given that change in behavior, it no longer makes sense to warn on
> non-zero values since they're expected now (even though they'll never be
> used). Maybe instead of removing the WARN() I should just add an
> "&& r->enable" to the warn condition?
>
>
> Matt
>
> >
> > > + if (INTEL_INFO(dev)->gen <= 6 && r->spr_val)
> > > results->wm_lp_spr[wm_lp - 1] = WM1S_LP_EN | r->spr_val;
> > > - } else
> > > + else
> > > results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> > > }
> > >
> > > --
> > > 2.1.4
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
