On Fri, Mar 04, 2016 at 07:47:09PM +0200, Ville Syrjälä wrote:
> 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.
Actually I think the whole approach of computing all the levels is sort
of wrong. Eg. we would now compute a sprite LP1 watermark for the
case where sprite scaling is enabled. But that means we'll then merge
that bogus watermark with something else for the intermediate watermark,
and thus we may actually get a different result than what we had before
this new scheme was implemented.
Even worse we don't actually limit the computed watermarks anymore so
they might even overflow the bits allotted.
So I think the whole approach needs more thought.
>
> >
> > 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
