All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Fix the possible watermark miswriting for skl+
@ 2018-11-13 14:31 Stanislav Lisovskiy
  2018-11-13 15:43 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Stanislav Lisovskiy @ 2018-11-13 14:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: ville.syrjala, martin.peres, dri-devel

Currently whenever we attempt to recalculate
watermarks, we assign dirty_pipes to zero,
then compare current wm results to the recalculated
one and if they changed we set correspondent dirty_pipes
bit again.
This can lead to situation, when we are clearing dirty_pipes,
same wm results twice in a row and not setting dirty_pipes
=> so that watermarks are not actually updated, which then might
lead to fifo underruns, crc mismatch and other issues.

Instead, whenever we detect that wm results are changed,
need to set correspondent dirty_pipes bit and clear it
only once the change is written, but not clear it everytime
we attempt to recalculate those in skl_compute_wm.

v2: Fixed commit message

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dc034617febb..f7fbc4bc0d43 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
 	bool changed = false;
 	int ret, i;
 
-	/* Clear all dirty flags */
-	results->dirty_pipes = 0;
-
 	ret = skl_ddb_add_affected_pipes(state, &changed);
 	if (ret || !changed)
 		return ret;
@@ -5496,6 +5493,7 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
+	struct skl_ddb_values *results = &state->wm_results;
 	enum pipe pipe = crtc->pipe;
 	enum plane_id plane_id;
 
@@ -5512,6 +5510,10 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 			skl_write_cursor_wm(crtc, &pipe_wm->planes[plane_id],
 					    ddb);
 	}
+
+	/* Clear correspondent dirty bit */
+	results->dirty_pipes &= ~drm_crtc_mask(&crtc->base);
+
 }
 
 static void skl_initial_wm(struct intel_atomic_state *state,
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* ✓ Fi.CI.BAT: success for Fix the possible watermark miswriting for skl+
  2018-11-13 14:31 [PATCH v2] Fix the possible watermark miswriting for skl+ Stanislav Lisovskiy
@ 2018-11-13 15:43 ` Patchwork
  2018-11-13 16:40 ` [PATCH v2] " Chris Wilson
  2018-11-13 19:37 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-11-13 15:43 UTC (permalink / raw)
  To: Stanislav Lisovskiy; +Cc: intel-gfx

== Series Details ==

Series: Fix the possible watermark miswriting for skl+
URL   : https://patchwork.freedesktop.org/series/52425/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5132 -> Patchwork_10814 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52425/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10814 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-bsw-kefka:       PASS -> INCOMPLETE (fdo#108714)

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#108714 https://bugs.freedesktop.org/show_bug.cgi?id=108714


== Participating hosts (49 -> 45) ==

  Additional (3): fi-byt-j1900 fi-glk-j4005 fi-kbl-7567u 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u 


== Build changes ==

    * Linux: CI_DRM_5132 -> Patchwork_10814

  CI_DRM_5132: 7fa49786320fddb8a4f89318e7eeb65ce6134f8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4714: cab148ca3ec904a94d0cd43476cf7e1f8663f906 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10814: 4b84e468f305112108e6b42d8fad677db4d7e5ca @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4b84e468f305 Fix the possible watermark miswriting for skl+

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10814/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Fix the possible watermark miswriting for skl+
  2018-11-13 14:31 [PATCH v2] Fix the possible watermark miswriting for skl+ Stanislav Lisovskiy
  2018-11-13 15:43 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-11-13 16:40 ` Chris Wilson
  2018-11-14  8:19   ` Lisovskiy, Stanislav
  2018-11-13 19:37 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2018-11-13 16:40 UTC (permalink / raw)
  To: Stanislav Lisovskiy, intel-gfx; +Cc: ville.syrjala, martin.peres, dri-devel

Quoting Stanislav Lisovskiy (2018-11-13 14:31:38)
> Currently whenever we attempt to recalculate
> watermarks, we assign dirty_pipes to zero,
> then compare current wm results to the recalculated
> one and if they changed we set correspondent dirty_pipes
> bit again.
> This can lead to situation, when we are clearing dirty_pipes,
> same wm results twice in a row and not setting dirty_pipes
> => so that watermarks are not actually updated, which then might
> lead to fifo underruns, crc mismatch and other issues.
> 
> Instead, whenever we detect that wm results are changed,
> need to set correspondent dirty_pipes bit and clear it
> only once the change is written, but not clear it everytime
> we attempt to recalculate those in skl_compute_wm.

Ok, but are not dirty_pipes being recomputed for each commit wrt to the
current HW state. Should dirty_pipes not be 0 at the start of compute_wm
naturally due to it not being used before in the atomic commit sequence?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* ✓ Fi.CI.IGT: success for Fix the possible watermark miswriting for skl+
  2018-11-13 14:31 [PATCH v2] Fix the possible watermark miswriting for skl+ Stanislav Lisovskiy
  2018-11-13 15:43 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-11-13 16:40 ` [PATCH v2] " Chris Wilson
@ 2018-11-13 19:37 ` Patchwork
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-11-13 19:37 UTC (permalink / raw)
  To: Stanislav Lisovskiy; +Cc: intel-gfx

== Series Details ==

Series: Fix the possible watermark miswriting for skl+
URL   : https://patchwork.freedesktop.org/series/52425/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5132_full -> Patchwork_10814_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10814_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10814_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10814_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_atomic_interruptible@universal-setplane-cursor:
      shard-snb:          PASS -> SKIP +2

    igt@perf_pmu@rc6:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_10814_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_softpin@noreloc-s3:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
      shard-kbl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-kbl:          NOTRUN -> DMESG-WARN (fdo#103313)

    igt@kms_color@pipe-c-legacy-gamma:
      shard-apl:          PASS -> FAIL (fdo#104782)

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          PASS -> FAIL (fdo#103232)

    igt@kms_flip@plain-flip-ts-check:
      shard-skl:          PASS -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
      shard-glk:          NOTRUN -> FAIL (fdo#108145)

    igt@kms_plane_lowres@pipe-b-tiling-none:
      shard-kbl:          NOTRUN -> DMESG-WARN (fdo#105345)

    igt@kms_properties@connector-properties-legacy:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105345, fdo#103313)

    igt@pm_rpm@gem-pread:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807)

    igt@pm_rpm@modeset-pc8-residency-stress:
      shard-skl:          SKIP -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@drm_import_export@import-close-race-flink:
      shard-skl:          TIMEOUT (fdo#108667) -> PASS

    igt@gem_exec_suspend@basic-s3:
      shard-glk:          INCOMPLETE (k.org#198133, fdo#103359) -> PASS

    igt@gem_exec_whisper@normal:
      shard-skl:          TIMEOUT (fdo#108592) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665, fdo#106887) -> PASS

    igt@gem_tiled_blits@normal:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
      shard-skl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_cursor_crc@cursor-256x256-onscreen:
      shard-skl:          FAIL (fdo#103232) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-apl:          FAIL (fdo#103167) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-glk:          FAIL (fdo#103167) -> PASS +1

    igt@kms_plane@plane-position-covered-pipe-b-planes:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
      shard-skl:          FAIL (fdo#107815) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS

    igt@kms_properties@plane-properties-atomic:
      shard-kbl:          DMESG-WARN (fdo#105345, fdo#103313) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105345 https://bugs.freedesktop.org/show_bug.cgi?id=105345
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106887 https://bugs.freedesktop.org/show_bug.cgi?id=106887
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108592 https://bugs.freedesktop.org/show_bug.cgi?id=108592
  fdo#108667 https://bugs.freedesktop.org/show_bug.cgi?id=108667
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5132 -> Patchwork_10814

  CI_DRM_5132: 7fa49786320fddb8a4f89318e7eeb65ce6134f8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4714: cab148ca3ec904a94d0cd43476cf7e1f8663f906 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10814: 4b84e468f305112108e6b42d8fad677db4d7e5ca @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10814/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Fix the possible watermark miswriting for skl+
  2018-11-13 16:40 ` [PATCH v2] " Chris Wilson
@ 2018-11-14  8:19   ` Lisovskiy, Stanislav
  2018-11-14 11:09     ` Ville Syrjälä
  0 siblings, 1 reply; 6+ messages in thread
From: Lisovskiy, Stanislav @ 2018-11-14  8:19 UTC (permalink / raw)
  To: intel-gfx, chris; +Cc: Syrjala, Ville, Peres, Martin, dri-devel

On Tue, 2018-11-13 at 16:40 +0000, Chris Wilson wrote:
> Quoting Stanislav Lisovskiy (2018-11-13 14:31:38)
> > Currently whenever we attempt to recalculate
> > watermarks, we assign dirty_pipes to zero,
> > then compare current wm results to the recalculated
> > one and if they changed we set correspondent dirty_pipes
> > bit again.
> > This can lead to situation, when we are clearing dirty_pipes,
> > same wm results twice in a row and not setting dirty_pipes
> > => so that watermarks are not actually updated, which then might
> > lead to fifo underruns, crc mismatch and other issues.
> > 
> > Instead, whenever we detect that wm results are changed,
> > need to set correspondent dirty_pipes bit and clear it
> > only once the change is written, but not clear it everytime
> > we attempt to recalculate those in skl_compute_wm.
> 
> Ok, but are not dirty_pipes being recomputed for each commit wrt to
> the
> current HW state. Should dirty_pipes not be 0 at the start of
> compute_wm
> naturally due to it not being used before in the atomic commit
> sequence?
> -Chris

We discussed this yesterday with Ville, as I understand the whole
drm_atomic_state is allocated each time the commit starts and it is 
already naturally 0. So anyway assigning here dirty_pipes to 0 looked
redundant at least to me and Ville.
However the problem I mean was that if skl_compute_wm is invoked 
twice and getting the same wm results, then we are not going to set
correspondent dirty_pipes bit and because it was zeroed in the
beginning of skl_compute_wm, we are not going to update the watermarks
at all, even though we should as it had changed.

During our discussion with Ville, we agreed that theoretically this
shouldn't happen because skl_compute_wm(called via
compute_global_watermarks hook) should be called only once per commit
in intel_atomic_check and then results are written, when crtc is
updated and this potentially can be caught using some WARN_ON.

What I did is I added a WARN_ON here in skl_compute_wm and removed
assigning dirty_pipes to zero(so that we can see that it might happen
that correspondent bit is already set, but we are getting here again):

@@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
        bool changed = false;
        int ret, i;
 
-       /* Clear all dirty flags */
-       results->dirty_pipes = 0;
-
@@ -5471,6 +5471,8 @@ skl_compute_wm(struct drm_atomic_state *state)
		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
		
		 &results->ddb, &changed);
                if (ret)
                        return ret;
 
+               WARN_ON(!changed && (results->dirty_pipes &
drm_crtc_mask(crtc)));
+
                if (changed)
                        results->dirty_pipes |= drm_crtc_mask(crtc);

If skl_compute_wm is called only once we should never get it, as
we would otherwise never have correpondent crtc bit already set here
(and we would already had zeroed dirty_pipes here as we do now, the 
update would be lost).

However this WARN really catches the issue:

[   35.681909] ------------[ cut here ]------------
[   35.681917] WARN_ON(!changed && (results->dirty_pipes &
drm_crtc_mask(crtc)))
[   35.681998] WARNING: CPU: 3 PID: 1499 at
drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
[   35.682002] Modules linked in: cdc_ether usbnet snd_hda_codec_hdmi
snd_hda_codec_realtek snd_hda_codec_generic r8152 mii snd_hda_intel
snd_hda_codec x86_pkg_temp_thermal snd_hwdep coretemp snd_hda_core
snd_pcm pl2303 usbserial btusb btrtl btbcm btintel bluetooth
ecdh_generic mei_me mei dm_crypt crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel i915 prime_numbers i2c_hid pinctrl_sunrisepoint
pinctrl_intel
[   35.682044] CPU: 3 PID: 1499 Comm: Xorg Tainted:
G        W         4.20.0-rc1-CI-CI_DRM_5076+ #123
[   35.682047] Hardware name: LENOVO 81A8/LNVNB161216, BIOS 5SCN34WW
09/01/2017
[   35.682093] RIP: 0010:skl_compute_wm+0x709/0x800 [i915]
[   35.682097] Code: c1 e8 11 e9 40 fe ff ff d3 e0 41 85 86 98 04 00 00
0f 84 85 fe ff ff 48 c7 c6 10 48 1c a0 48 c7 c7 2c 13 1b a0 e8 07 49 03
e1 <0f> 0b 41 8b 86 98 04 00 00 e9 4f fe ff ff 44 89 ca e9 a5 f9 ff ff
[   35.682101] RSP: 0018:ffffc90000c53a08 EFLAGS: 00010282
[   35.682106] RAX: 0000000000000000 RBX: 00000000007784bf RCX:
0000000000000006
[   35.682109] RDX: 0000000000000006 RSI: ffffffff8212886a RDI:
ffffffff820d6d57
[   35.682112] RBP: ffff88029ef40000 R08: 000000001d5e3b8b R09:
0000000000000000
[   35.682116] R10: ffff88029be2ea54 R11: 0000000000000000 R12:
ffff880299db6fc8
[   35.682119] R13: ffff88029be2e678 R14: ffff88026000c138 R15:
0000000000000001
[   35.682123] FS:  00007f3bbca90600(0000) GS:ffff8802b9d80000(0000)
knlGS:0000000000000000
[   35.682126] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   35.682130] CR2: 00007f838a7b1158 CR3: 000000026cbe2003 CR4:
00000000003606e0
[   35.682133] Call Trace:
[   35.682148]  ? __mutex_unlock_slowpath+0x46/0x2b0
[   35.682215]  intel_atomic_check+0x4d6/0x1230 [i915]
[   35.682233]  drm_atomic_check_only+0x477/0x710
[   35.682245]  drm_atomic_commit+0xe/0x50
[   35.682253]  drm_atomic_helper_set_config+0x7b/0x90
[   35.682260]  drm_mode_setcrtc+0x195/0x6b0
[   35.682289]  ? drm_mode_getcrtc+0x180/0x180
[   35.682295]  drm_ioctl_kernel+0x81/0xf0
[   35.682305]  drm_ioctl+0x2de/0x390
[   35.682312]  ? drm_mode_getcrtc+0x180/0x180
[   35.682327]  ? lock_acquire+0xa6/0x1c0
[   35.682336]  do_vfs_ioctl+0xa0/0x6e0
[   35.682345]  ? __fget+0xfc/0x1e0
[   35.682353]  ksys_ioctl+0x35/0x60
[   35.682361]  __x64_sys_ioctl+0x11/0x20
[   35.682369]  do_syscall_64+0x55/0x190
[   35.682376]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   35.682380] RIP: 0033:0x7f3bb9ea85d7
[   35.682385] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00
48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
[   35.682388] RSP: 002b:00007fff41c6e8d8 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
[   35.682393] RAX: ffffffffffffffda RBX: 000055fca1377680 RCX:
00007f3bb9ea85d7
[   35.682396] RDX: 00007fff41c6e9c0 RSI: 00000000c06864a2 RDI:
000000000000000d
[   35.682400] RBP: 00007fff41c6e9c0 R08: 0000000000000001 R09:
00007f3bbc9f4000
[   35.682403] R10: 0000000000000001 R11: 0000000000000246 R12:
00000000c06864a2
[   35.682406] R13: 000000000000000d R14: 000000000000000a R15:
000055fca137fa50
[   35.682424] irq event stamp: 847224
[   35.682429] hardirqs last  enabled at (847223): [<ffffffff810fba24>]
vprintk_emit+0x124/0x320
[   35.682434] hardirqs last disabled at (847224): [<ffffffff810019a0>]
trace_hardirqs_off_thunk+0x1a/0x1c
[   35.682438] softirqs last  enabled at (847182): [<ffffffff81c0033a>]
__do_softirq+0x33a/0x4b9
[   35.682444] softirqs last disabled at (847175): [<ffffffff8108def9>]
irq_exit+0xa9/0xc0
[   35.682484] WARNING: CPU: 3 PID: 1499 at
drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
[   35.682487] ---[ end trace 7b4397aa7cd7d405 ]---

Which means that we are losing some watermark updates, in case if we
assign dirty_pipes to zero and then not setting it back.


> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Best Regards,

Lisovskiy Stanislav
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Fix the possible watermark miswriting for skl+
  2018-11-14  8:19   ` Lisovskiy, Stanislav
@ 2018-11-14 11:09     ` Ville Syrjälä
  0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2018-11-14 11:09 UTC (permalink / raw)
  To: Lisovskiy, Stanislav; +Cc: intel-gfx, Syrjala, Ville, Peres, Martin, dri-devel

On Wed, Nov 14, 2018 at 08:19:26AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2018-11-13 at 16:40 +0000, Chris Wilson wrote:
> > Quoting Stanislav Lisovskiy (2018-11-13 14:31:38)
> > > Currently whenever we attempt to recalculate
> > > watermarks, we assign dirty_pipes to zero,
> > > then compare current wm results to the recalculated
> > > one and if they changed we set correspondent dirty_pipes
> > > bit again.
> > > This can lead to situation, when we are clearing dirty_pipes,
> > > same wm results twice in a row and not setting dirty_pipes
> > > => so that watermarks are not actually updated, which then might
> > > lead to fifo underruns, crc mismatch and other issues.
> > > 
> > > Instead, whenever we detect that wm results are changed,
> > > need to set correspondent dirty_pipes bit and clear it
> > > only once the change is written, but not clear it everytime
> > > we attempt to recalculate those in skl_compute_wm.
> > 
> > Ok, but are not dirty_pipes being recomputed for each commit wrt to
> > the
> > current HW state. Should dirty_pipes not be 0 at the start of
> > compute_wm
> > naturally due to it not being used before in the atomic commit
> > sequence?
> > -Chris
> 
> We discussed this yesterday with Ville, as I understand the whole
> drm_atomic_state is allocated each time the commit starts and it is 
> already naturally 0. So anyway assigning here dirty_pipes to 0 looked
> redundant at least to me and Ville.
> However the problem I mean was that if skl_compute_wm is invoked 
> twice and getting the same wm results, then we are not going to set
> correspondent dirty_pipes bit and because it was zeroed in the
> beginning of skl_compute_wm, we are not going to update the watermarks
> at all, even though we should as it had changed.
> 
> During our discussion with Ville, we agreed that theoretically this
> shouldn't happen because skl_compute_wm(called via
> compute_global_watermarks hook) should be called only once per commit
> in intel_atomic_check and then results are written, when crtc is
> updated and this potentially can be caught using some WARN_ON.
> 
> What I did is I added a WARN_ON here in skl_compute_wm and removed
> assigning dirty_pipes to zero(so that we can see that it might happen
> that correspondent bit is already set, but we are getting here again):
> 
> @@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
>         bool changed = false;
>         int ret, i;
>  
> -       /* Clear all dirty flags */
> -       results->dirty_pipes = 0;
> -
> @@ -5471,6 +5471,8 @@ skl_compute_wm(struct drm_atomic_state *state)
> 		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> 		
> 		 &results->ddb, &changed);
>                 if (ret)
>                         return ret;
>  
> +               WARN_ON(!changed && (results->dirty_pipes &
> drm_crtc_mask(crtc)));
> +
>                 if (changed)
>                         results->dirty_pipes |= drm_crtc_mask(crtc);
> 
> If skl_compute_wm is called only once we should never get it, as
> we would otherwise never have correpondent crtc bit already set here
> (and we would already had zeroed dirty_pipes here as we do now, the 
> update would be lost).
> 
> However this WARN really catches the issue:

Probably due to skl_ddb_add_affected_pipes(), which looks safe enough
to me.

Not sure why it sets dirty_pipes though. Either the ddb allocations
change or they don't, so I don't really see why we need to mark
every pipe as dirty here. They will be marked naturally by
skl_ddb_add_affected_planes() anyway.

So looks to me we can just drop the dirty_pipes frobbing from
skl_ddb_add_affected_pipes().

> 
> [   35.681909] ------------[ cut here ]------------
> [   35.681917] WARN_ON(!changed && (results->dirty_pipes &
> drm_crtc_mask(crtc)))
> [   35.681998] WARNING: CPU: 3 PID: 1499 at
> drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
> [   35.682002] Modules linked in: cdc_ether usbnet snd_hda_codec_hdmi
> snd_hda_codec_realtek snd_hda_codec_generic r8152 mii snd_hda_intel
> snd_hda_codec x86_pkg_temp_thermal snd_hwdep coretemp snd_hda_core
> snd_pcm pl2303 usbserial btusb btrtl btbcm btintel bluetooth
> ecdh_generic mei_me mei dm_crypt crct10dif_pclmul crc32_pclmul
> ghash_clmulni_intel i915 prime_numbers i2c_hid pinctrl_sunrisepoint
> pinctrl_intel
> [   35.682044] CPU: 3 PID: 1499 Comm: Xorg Tainted:
> G        W         4.20.0-rc1-CI-CI_DRM_5076+ #123
> [   35.682047] Hardware name: LENOVO 81A8/LNVNB161216, BIOS 5SCN34WW
> 09/01/2017
> [   35.682093] RIP: 0010:skl_compute_wm+0x709/0x800 [i915]
> [   35.682097] Code: c1 e8 11 e9 40 fe ff ff d3 e0 41 85 86 98 04 00 00
> 0f 84 85 fe ff ff 48 c7 c6 10 48 1c a0 48 c7 c7 2c 13 1b a0 e8 07 49 03
> e1 <0f> 0b 41 8b 86 98 04 00 00 e9 4f fe ff ff 44 89 ca e9 a5 f9 ff ff
> [   35.682101] RSP: 0018:ffffc90000c53a08 EFLAGS: 00010282
> [   35.682106] RAX: 0000000000000000 RBX: 00000000007784bf RCX:
> 0000000000000006
> [   35.682109] RDX: 0000000000000006 RSI: ffffffff8212886a RDI:
> ffffffff820d6d57
> [   35.682112] RBP: ffff88029ef40000 R08: 000000001d5e3b8b R09:
> 0000000000000000
> [   35.682116] R10: ffff88029be2ea54 R11: 0000000000000000 R12:
> ffff880299db6fc8
> [   35.682119] R13: ffff88029be2e678 R14: ffff88026000c138 R15:
> 0000000000000001
> [   35.682123] FS:  00007f3bbca90600(0000) GS:ffff8802b9d80000(0000)
> knlGS:0000000000000000
> [   35.682126] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   35.682130] CR2: 00007f838a7b1158 CR3: 000000026cbe2003 CR4:
> 00000000003606e0
> [   35.682133] Call Trace:
> [   35.682148]  ? __mutex_unlock_slowpath+0x46/0x2b0
> [   35.682215]  intel_atomic_check+0x4d6/0x1230 [i915]
> [   35.682233]  drm_atomic_check_only+0x477/0x710
> [   35.682245]  drm_atomic_commit+0xe/0x50
> [   35.682253]  drm_atomic_helper_set_config+0x7b/0x90
> [   35.682260]  drm_mode_setcrtc+0x195/0x6b0
> [   35.682289]  ? drm_mode_getcrtc+0x180/0x180
> [   35.682295]  drm_ioctl_kernel+0x81/0xf0
> [   35.682305]  drm_ioctl+0x2de/0x390
> [   35.682312]  ? drm_mode_getcrtc+0x180/0x180
> [   35.682327]  ? lock_acquire+0xa6/0x1c0
> [   35.682336]  do_vfs_ioctl+0xa0/0x6e0
> [   35.682345]  ? __fget+0xfc/0x1e0
> [   35.682353]  ksys_ioctl+0x35/0x60
> [   35.682361]  __x64_sys_ioctl+0x11/0x20
> [   35.682369]  do_syscall_64+0x55/0x190
> [   35.682376]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [   35.682380] RIP: 0033:0x7f3bb9ea85d7
> [   35.682385] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00
> 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
> [   35.682388] RSP: 002b:00007fff41c6e8d8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000010
> [   35.682393] RAX: ffffffffffffffda RBX: 000055fca1377680 RCX:
> 00007f3bb9ea85d7
> [   35.682396] RDX: 00007fff41c6e9c0 RSI: 00000000c06864a2 RDI:
> 000000000000000d
> [   35.682400] RBP: 00007fff41c6e9c0 R08: 0000000000000001 R09:
> 00007f3bbc9f4000
> [   35.682403] R10: 0000000000000001 R11: 0000000000000246 R12:
> 00000000c06864a2
> [   35.682406] R13: 000000000000000d R14: 000000000000000a R15:
> 000055fca137fa50
> [   35.682424] irq event stamp: 847224
> [   35.682429] hardirqs last  enabled at (847223): [<ffffffff810fba24>]
> vprintk_emit+0x124/0x320
> [   35.682434] hardirqs last disabled at (847224): [<ffffffff810019a0>]
> trace_hardirqs_off_thunk+0x1a/0x1c
> [   35.682438] softirqs last  enabled at (847182): [<ffffffff81c0033a>]
> __do_softirq+0x33a/0x4b9
> [   35.682444] softirqs last disabled at (847175): [<ffffffff8108def9>]
> irq_exit+0xa9/0xc0
> [   35.682484] WARNING: CPU: 3 PID: 1499 at
> drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
> [   35.682487] ---[ end trace 7b4397aa7cd7d405 ]---
> 
> Which means that we are losing some watermark updates, in case if we
> assign dirty_pipes to zero and then not setting it back.
> 
> 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -- 
> Best Regards,
> 
> Lisovskiy Stanislav
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-14 11:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 14:31 [PATCH v2] Fix the possible watermark miswriting for skl+ Stanislav Lisovskiy
2018-11-13 15:43 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-11-13 16:40 ` [PATCH v2] " Chris Wilson
2018-11-14  8:19   ` Lisovskiy, Stanislav
2018-11-14 11:09     ` Ville Syrjälä
2018-11-13 19:37 ` ✓ Fi.CI.IGT: success for " Patchwork

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.