[Intel-gfx] [PATCH] drm/i915: Don't WARN() when sprite watermark > 0 for disabled LP2/LP3 levels
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Mon Mar 7 10:52:18 UTC 2016
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 at linux.intel.com>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Testcase: kms_universal_plane
>>>> Fixes: d81f04c5ef ("drm/i915: Allow preservation of watermarks, v2.")
>>>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>>> ---
>>>> 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
More information about the Intel-gfx
mailing list