Op 04-03-16 om 18:47 schreef Ville Syrjälä: > 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 Would the following work? ilk_validate_wm_level checks for !wm->enable, so it does the right thing here.. diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f65e84137060..8640199061cb 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1976,8 +1976,24 @@ static bool ilk_validate_wm_level(int level, return ret; } +static bool ilk_valid_sprite_level(const struct drm_i915_private *dev_priv, + const struct intel_pipe_wm *pipe_wm, + int level) +{ + /* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */ + if (level && pipe_wm->sprites_scaled) + return false; + + /* ILK/SNB: LP2+ watermarks only w/o sprites */ + if (level > 1 && INTEL_INFO(dev_priv)->gen <= 6 && pipe_wm->sprites_enabled) + return false; + + return true; +} + static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv, const struct intel_crtc *intel_crtc, + const struct intel_pipe_wm *pipe_wm, int level, struct intel_crtc_state *cstate, struct intel_plane_state *pristate, @@ -1988,6 +2004,7 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv, uint16_t pri_latency = dev_priv->wm.pri_latency[level]; uint16_t spr_latency = dev_priv->wm.spr_latency[level]; uint16_t cur_latency = dev_priv->wm.cur_latency[level]; + bool spr_valid = ilk_valid_sprite_level(dev_priv, pipe_wm, level); /* WM1+ latency values stored in 0.5us units */ if (level > 0) { @@ -2002,13 +2019,19 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv, result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val); } - if (sprstate) - result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency); + if (sprstate && spr_valid) + result->spr_val = ilk_compute_spr_wm(cstate, sprstate, + spr_latency); + else if (sprstate) + result->spr_val = 0; if (curstate) result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency); - result->enable = true; + if (level && pipe_wm->sprites_enabled && !spr_valid) + result->enable = false; + else + result->enable = true; } static uint32_t @@ -2306,7 +2329,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) struct intel_plane_state *pristate = NULL; struct intel_plane_state *sprstate = NULL; struct intel_plane_state *curstate = NULL; - int level, max_level = ilk_wm_max_level(dev), usable_level; + int level, max_level = ilk_wm_max_level(dev), usable_level = max_level; struct ilk_wm_maximums max; pipe_wm = &cstate->wm.optimal.ilk; @@ -2335,18 +2358,7 @@ 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 */ - if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled) - usable_level = 1; - - /* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */ - if (pipe_wm->sprites_scaled) - usable_level = 0; - - ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, + ilk_compute_wm_level(dev_priv, intel_crtc, pipe_wm, 0, cstate, pristate, sprstate, curstate, &pipe_wm->wm[0]); if (IS_HASWELL(dev) || IS_BROADWELL(dev)) @@ -2360,8 +2372,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate) for (level = 1; level <= max_level; level++) { struct intel_wm_level *wm = &pipe_wm->wm[level]; - ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, - pristate, sprstate, curstate, wm); + ilk_compute_wm_level(dev_priv, intel_crtc, pipe_wm, + level, cstate, pristate, sprstate, + curstate, wm); /* * Disable any watermark level that exceeds the _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx