| Message ID | 20221202122844.428006-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/3] drm/i915/migrate: Account for the reserved_space | expand |
Hi, I messed-up with versions, my prev comment landed in v2, so I put it here to clean things up. On 02.12.2022 13:28, Matthew Auld wrote: > From: Chris Wilson <chris.p.wilson@intel.com> > > If the ring is nearly full when calling into emit_pte(), we might > incorrectly trample the reserved_space when constructing the packet to > emit the PTEs. This then triggers the GEM_BUG_ON(rq->reserved_space > > ring->space) when later submitting the request, since the request itself > doesn't have enough space left in the ring to emit things like > workarounds, breadcrumbs etc. > > v2: Fix the whitespace errors > > Testcase: igt@i915_selftests@live_emit_pte_full_ring > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7535 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6889 > Fixes: cf586021642d ("drm/i915/gt: Pipelined page migration") > Signed-off-by: Chris Wilson <chris.p.wilson@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: <stable@vger.kernel.org> # v5.15+ > Tested-by: Nirmoy Das <nirmoy.das@intel.com> > Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_migrate.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c > index b405a04135ca..b783f6f740c8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_migrate.c > +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c > @@ -342,6 +342,16 @@ static int emit_no_arbitration(struct i915_request *rq) > return 0; > } > > +static int max_pte_pkt_size(struct i915_request *rq, int pkt) > +{ > + struct intel_ring *ring = rq->ring; > + > + pkt = min_t(int, pkt, (ring->space - rq->reserved_space) / sizeof(u32) + 5); > + pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); > + > + return pkt; > +} > + I guess, the assumption that subtractions of u32 values do not overflows, is valid. Then I guess more natural would be use u32 for all vars involved, this way we can use min instead of min_t, minor nit. Anyway: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > static int emit_pte(struct i915_request *rq, > struct sgt_dma *it, > enum i915_cache_level cache_level, > @@ -388,8 +398,7 @@ static int emit_pte(struct i915_request *rq, > return PTR_ERR(cs); > > /* Pack as many PTE updates as possible into a single MI command */ > - pkt = min_t(int, dword_length, ring->space / sizeof(u32) + 5); > - pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); > + pkt = max_pte_pkt_size(rq, dword_length); > > hdr = cs; > *cs++ = MI_STORE_DATA_IMM | REG_BIT(21); /* as qword elements */ > @@ -422,8 +431,7 @@ static int emit_pte(struct i915_request *rq, > } > } > > - pkt = min_t(int, dword_rem, ring->space / sizeof(u32) + 5); > - pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); > + pkt = max_pte_pkt_size(rq, dword_rem); > > hdr = cs; > *cs++ = MI_STORE_DATA_IMM | REG_BIT(21);
Hi Matt, On Fri, Dec 02, 2022 at 12:28:42PM +0000, Matthew Auld wrote: > From: Chris Wilson <chris.p.wilson@intel.com> > > If the ring is nearly full when calling into emit_pte(), we might > incorrectly trample the reserved_space when constructing the packet to > emit the PTEs. This then triggers the GEM_BUG_ON(rq->reserved_space > > ring->space) when later submitting the request, since the request itself > doesn't have enough space left in the ring to emit things like > workarounds, breadcrumbs etc. > > v2: Fix the whitespace errors > > Testcase: igt@i915_selftests@live_emit_pte_full_ring > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7535 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6889 > Fixes: cf586021642d ("drm/i915/gt: Pipelined page migration") > Signed-off-by: Chris Wilson <chris.p.wilson@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: <stable@vger.kernel.org> # v5.15+ > Tested-by: Nirmoy Das <nirmoy.das@intel.com> > Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
On 02/12/2022 14:06, Patchwork wrote: > *Patch Details* > *Series:* series starting with [v6,1/3] drm/i915/migrate: Account for > the reserved_space > *URL:* https://patchwork.freedesktop.org/series/111583/ > <https://patchwork.freedesktop.org/series/111583/> > *State:* failure > *Details:* > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/index.html > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/index.html> > > > CI Bug Log - changes from CI_DRM_12462 -> Patchwork_111583v1 > > > Summary > > *FAILURE* > > Serious unknown changes coming with Patchwork_111583v1 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_111583v1, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/index.html > > > Participating hosts (42 -> 42) > > Additional (3): fi-hsw-4770 bat-dg1-7 bat-adlp-9 > Missing (3): fi-ilk-m540 fi-rkl-11600 bat-atsm-1 > > > Possible new issues > > Here are the unknown changes that may have been introduced in > Patchwork_111583v1: > > > IGT changes > > > Possible regressions > > * igt@i915_selftest@live@guc_multi_lrc: > o fi-kbl-soraka: PASS > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/fi-kbl-soraka/igt@i915_selftest@live@guc_multi_lrc.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-kbl-soraka/igt@i915_selftest@live@guc_multi_lrc.html> > Unrelated it seems. > > Suppressed > > The following results come from untrusted machines, tests, or statuses. > They do not affect the overall result. > > * igt@i915_selftest@live@requests: > o {bat-dg2-11}: PASS > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/bat-dg2-11/igt@i915_selftest@live@requests.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/bat-dg2-11/igt@i915_selftest@live@requests.html> > > > Known issues > > Here are the changes found in Patchwork_111583v1 that come from known > issues: > > > IGT changes > > > Issues hit > > * > > igt@core_hotunplug@unbind-rebind: > > o fi-apl-guc: PASS > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html> (i915#7073 <https://gitlab.freedesktop.org/drm/intel/issues/7073>) > * > > igt@gem_softpin@allocator-basic-reserve: > > o fi-hsw-4770: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-hsw-4770/igt@gem_softpin@allocator-basic-reserve.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +11 similar issues > * > > igt@i915_selftest@live@hangcheck: > > o > > fi-hsw-4770: NOTRUN -> INCOMPLETE > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html> (i915#4785 <https://gitlab.freedesktop.org/drm/intel/issues/4785>) > > o > > fi-ivb-3770: PASS > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html> (i915#3303 <https://gitlab.freedesktop.org/drm/intel/issues/3303> / i915#7122 <https://gitlab.freedesktop.org/drm/intel/issues/7122>) > > * > > igt@kms_chamelium@dp-crc-fast: > > o fi-hsw-4770: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-hsw-4770/igt@kms_chamelium@dp-crc-fast.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / fdo#111827 <https://bugs.freedesktop.org/show_bug.cgi?id=111827>) +7 similar issues > * > > igt@kms_psr@sprite_plane_onoff: > > o fi-hsw-4770: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-hsw-4770/igt@kms_psr@sprite_plane_onoff.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#1072 <https://gitlab.freedesktop.org/drm/intel/issues/1072>) +3 similar issues > * > > igt@runner@aborted: > > o > > fi-hsw-4770: NOTRUN -> FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-hsw-4770/igt@runner@aborted.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#4312 <https://gitlab.freedesktop.org/drm/intel/issues/4312> / i915#5594 <https://gitlab.freedesktop.org/drm/intel/issues/5594>) > > o > > fi-ivb-3770: NOTRUN -> FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-ivb-3770/igt@runner@aborted.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#4312 <https://gitlab.freedesktop.org/drm/intel/issues/4312>) > > > Possible fixes > > * > > igt@gem_exec_suspend@basic-s3@smem: > > o {bat-rplp-1}: DMESG-WARN > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html> (i915#2867 <https://gitlab.freedesktop.org/drm/intel/issues/2867>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html> > * > > igt@i915_selftest@live@requests: > > o {bat-rpls-2}: INCOMPLETE > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/bat-rpls-2/igt@i915_selftest@live@requests.html> (i915#4983 <https://gitlab.freedesktop.org/drm/intel/issues/4983> / i915#6257 <https://gitlab.freedesktop.org/drm/intel/issues/6257>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/bat-rpls-2/igt@i915_selftest@live@requests.html> > * > > igt@i915_selftest@live@slpc: > > o {bat-adln-1}: DMESG-FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/bat-adln-1/igt@i915_selftest@live@slpc.html> (i915#6997 <https://gitlab.freedesktop.org/drm/intel/issues/6997>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/bat-adln-1/igt@i915_selftest@live@slpc.html> > * > > igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions: > > o fi-bsw-kefka: FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html> (i915#6298 <https://gitlab.freedesktop.org/drm/intel/issues/6298>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html> > * > > igt@kms_psr@primary_page_flip: > > o fi-kbl-soraka: DMESG-WARN > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12462/fi-kbl-soraka/igt@kms_psr@primary_page_flip.html> (i915#1982 <https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111583v1/fi-kbl-soraka/igt@kms_psr@primary_page_flip.html> > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > > Build changes > > * Linux: CI_DRM_12462 -> Patchwork_111583v1 > > CI-20190529: 20190529 > CI_DRM_12462: 00b10bdfd8b9edc9b2c681d806fbb6ae2e5f31a3 @ > git://anongit.freedesktop.org/gfx-ci/linux > IGT_7078: 71bce31c26998d5d53cff3138049261fd6c4fbaf @ > https://gitlab.freedesktop.org/drm/igt-gpu-tools.git > Patchwork_111583v1: 00b10bdfd8b9edc9b2c681d806fbb6ae2e5f31a3 @ > git://anongit.freedesktop.org/gfx-ci/linux > > > Linux commits > > 108fd0fd08c2 drm/i915/selftests: exercise emit_pte() with nearly full ring > cc3ee4e1969b drm/i915/selftests: use live_subtests for live_migrate > bc2bb88fad52 drm/i915/migrate: Account for the reserved_space >
diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c index b405a04135ca..b783f6f740c8 100644 --- a/drivers/gpu/drm/i915/gt/intel_migrate.c +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c @@ -342,6 +342,16 @@ static int emit_no_arbitration(struct i915_request *rq) return 0; } +static int max_pte_pkt_size(struct i915_request *rq, int pkt) +{ + struct intel_ring *ring = rq->ring; + + pkt = min_t(int, pkt, (ring->space - rq->reserved_space) / sizeof(u32) + 5); + pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); + + return pkt; +} + static int emit_pte(struct i915_request *rq, struct sgt_dma *it, enum i915_cache_level cache_level, @@ -388,8 +398,7 @@ static int emit_pte(struct i915_request *rq, return PTR_ERR(cs); /* Pack as many PTE updates as possible into a single MI command */ - pkt = min_t(int, dword_length, ring->space / sizeof(u32) + 5); - pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); + pkt = max_pte_pkt_size(rq, dword_length); hdr = cs; *cs++ = MI_STORE_DATA_IMM | REG_BIT(21); /* as qword elements */ @@ -422,8 +431,7 @@ static int emit_pte(struct i915_request *rq, } } - pkt = min_t(int, dword_rem, ring->space / sizeof(u32) + 5); - pkt = min_t(int, pkt, (ring->size - ring->emit) / sizeof(u32) + 5); + pkt = max_pte_pkt_size(rq, dword_rem); hdr = cs; *cs++ = MI_STORE_DATA_IMM | REG_BIT(21);