| Message ID | upbjdavlbcxku63ns4vstp5kgbn2anxwewpmnppszgb67fn66t@tfclfgkqijue (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] drm/i915: ensure segment offset never exceeds allowed max | expand |
On 2024-11-19 at 07:32:18 +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: ensure segment offset never exceeds allowed max (rev7) > URL : https://patchwork.freedesktop.org/series/140374/ > State : failure > > == Summary == > > CI Bug Log - changes from CI_DRM_15717 -> Patchwork_140374v7 > ==================================================== > > Summary > ------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_140374v7 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_140374v7, please notify your bug team (I915-ci-infra@lists.freedesktop.org) 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_140374v7/index.html > > Participating hosts (46 -> 45) > ------------------------------ > > Missing (1): fi-snb-2520m > > Possible new issues > ------------------- > > Here are the unknown changes that may have been introduced in Patchwork_140374v7: > > ### IGT changes ### > > #### Possible regressions #### > > * igt@i915_selftest@live@workarounds: > - bat-mtlp-6: [PASS][1] -> [ABORT][2] > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-mtlp-6/igt@i915_selftest@live@workarounds.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-mtlp-6/igt@i915_selftest@live@workarounds.html > > > #### Suppressed #### > > The following results come from untrusted machines, tests, or statuses. > They do not affect the overall result. > > * igt@kms_flip@basic-flip-vs-wf_vblank@d-dp6: > - {bat-mtlp-9}: [PASS][3] -> [FAIL][4] > [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-mtlp-9/igt@kms_flip@basic-flip-vs-wf_vblank@d-dp6.html > [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-mtlp-9/igt@kms_flip@basic-flip-vs-wf_vblank@d-dp6.html > > These issues do not appear in local testing and have not been detected on v4 of the patch (the code has not changed since then). I am going to retrigger the tests. Krzysztof > Known issues > ------------ > > Here are the changes found in Patchwork_140374v7 that come from known issues: > > ### IGT changes ### > > #### Issues hit #### > > * igt@gem_tiled_blits@basic: > - fi-pnv-d510: [PASS][5] -> [SKIP][6] +2 other tests skip > [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-pnv-d510/igt@gem_tiled_blits@basic.html > [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/fi-pnv-d510/igt@gem_tiled_blits@basic.html > > * igt@i915_module_load@load: > - bat-adlp-6: [PASS][7] -> [DMESG-WARN][8] ([i915#12253]) > [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-adlp-6/igt@i915_module_load@load.html > [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-adlp-6/igt@i915_module_load@load.html > > * igt@i915_selftest@live: > - bat-arlh-3: [PASS][9] -> [ABORT][10] ([i915#12829]) > [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-3/igt@i915_selftest@live.html > [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-arlh-3/igt@i915_selftest@live.html > - bat-mtlp-6: [PASS][11] -> [ABORT][12] ([i915#12829]) > [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-mtlp-6/igt@i915_selftest@live.html > [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-mtlp-6/igt@i915_selftest@live.html > > * igt@i915_selftest@live@workarounds: > - bat-arlh-3: [PASS][13] -> [ABORT][14] ([i915#12061]) > [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-3/igt@i915_selftest@live@workarounds.html > [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-arlh-3/igt@i915_selftest@live@workarounds.html > > * igt@runner@aborted: > - bat-dg2-13: NOTRUN -> [FAIL][15] ([i915#12658]) > [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-dg2-13/igt@runner@aborted.html > > > #### Possible fixes #### > > * igt@core_auth@basic-auth: > - bat-twl-1: [DMESG-WARN][16] ([i915#1982]) -> [PASS][17] > [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-twl-1/igt@core_auth@basic-auth.html > [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-twl-1/igt@core_auth@basic-auth.html > > * igt@dmabuf@all-tests: > - bat-apl-1: [INCOMPLETE][18] ([i915#12904]) -> [PASS][19] +1 other test pass > [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-apl-1/igt@dmabuf@all-tests.html > [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-apl-1/igt@dmabuf@all-tests.html > > * igt@dmabuf@all-tests@dma_fence_chain: > - fi-bsw-nick: [INCOMPLETE][20] ([i915#12904]) -> [PASS][21] +1 other test pass > [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-bsw-nick/igt@dmabuf@all-tests@dma_fence_chain.html > [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/fi-bsw-nick/igt@dmabuf@all-tests@dma_fence_chain.html > > * igt@i915_pm_rpm@module-reload: > - bat-rpls-4: [FAIL][22] ([i915#12903]) -> [PASS][23] > [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-rpls-4/igt@i915_pm_rpm@module-reload.html > [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-rpls-4/igt@i915_pm_rpm@module-reload.html > > * igt@i915_selftest@live: > - bat-arlh-2: [ABORT][24] ([i915#12829]) -> [PASS][25] > [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-2/igt@i915_selftest@live.html > [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-arlh-2/igt@i915_selftest@live.html > - {bat-mtlp-9}: [ABORT][26] ([i915#12829]) -> [PASS][27] > [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-mtlp-9/igt@i915_selftest@live.html > [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-mtlp-9/igt@i915_selftest@live.html > > * igt@i915_selftest@live@workarounds: > - bat-arlh-2: [ABORT][28] ([i915#12061]) -> [PASS][29] > [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-2/igt@i915_selftest@live@workarounds.html > [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-arlh-2/igt@i915_selftest@live@workarounds.html > - {bat-mtlp-9}: [ABORT][30] -> [PASS][31] > [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-mtlp-9/igt@i915_selftest@live@workarounds.html > [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/bat-mtlp-9/igt@i915_selftest@live@workarounds.html > > * igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1: > - fi-cfl-8109u: [DMESG-WARN][32] -> [PASS][33] +2 other tests pass > [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-cfl-8109u/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html > [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/fi-cfl-8109u/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html > > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061 > [i915#12253]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12253 > [i915#12658]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12658 > [i915#12829]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12829 > [i915#12903]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12903 > [i915#12904]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904 > [i915#1982]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1982 > [i915#2122]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2122 > > > Build changes > ------------- > > * Linux: CI_DRM_15717 -> Patchwork_140374v7 > > CI-20190529: 20190529 > CI_DRM_15717: 1fe9a6cc7d136c9a34c47ccd6ee5a2b7d02c0bd6 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_8115: 4942fc57c20f9cb2195e70991c4e4df03dd3db21 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git > Patchwork_140374v7: 1fe9a6cc7d136c9a34c47ccd6ee5a2b7d02c0bd6 @ git://anongit.freedesktop.org/gfx-ci/linux > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140374v7/index.html
Hi Krzysztof, On Mon, Nov 18, 2024 at 12:19:22PM +0000, Krzysztof Karas wrote: > Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for > partial memory mapping") introduced a new offset, which accounts for > userspace mapping not starting from the beginning of object's scatterlist. > > This works fine for cases where first object pte is larger than the new > offset - "r->sgt.curr" counter is set to the offset to match the difference > in the number of total pages. However, if object's first pte's size is > equal to or smaller than the offset, then information about the offset > in userspace is covered up by moving "r->sgt" pointer in remap_sg(): > > r->sgt.curr += PAGE_SIZE; > if (r->sgt.curr >= r->sgt.max) > r->sgt = __sgt_iter(__sg_next(r->sgt.sgp), use_dma(r->iobase)); > > This means that two or more pages from virtual memory are counted for > only one page in object's memory, because after moving "r->sgt" pointer > "r->sgt.curr" will be 0. > > We should account for this mismatch by moving "r->sgt" pointer to the > next pte. For that we may use "r.sgt.max", which already holds the max > allowed size. This change also eliminates possible confusion, when > looking at i915_scatterlist.h and remap_io_sg() code: former has > scatterlist pointer definition, which differentiates "s.max" value > based on "dma" flag (sg_dma_len() is used only when the flag is > enabled), while latter uses sg_dma_len() indiscriminately. > > This patch aims to resolve issue: > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12031 > > > v3: > - instead of checking if r.sgt.curr would exceed allowed max, changed > the value in the while loop to be aligned with `dma` value > > v4: > - remove unnecessary parent relation > > v5: > - update commit message with explanation about page counting mismatch > and link to the issue > > Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> merged to drm-intel-gt-next. Thanks, Andi
Hi Krzysztof, On Mon, Nov 18, 2024 at 12:19:22PM +0000, Krzysztof Karas wrote: > Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for > partial memory mapping") introduced a new offset, which accounts for > userspace mapping not starting from the beginning of object's scatterlist. > > This works fine for cases where first object pte is larger than the new > offset - "r->sgt.curr" counter is set to the offset to match the difference > in the number of total pages. However, if object's first pte's size is > equal to or smaller than the offset, then information about the offset > in userspace is covered up by moving "r->sgt" pointer in remap_sg(): > > r->sgt.curr += PAGE_SIZE; > if (r->sgt.curr >= r->sgt.max) > r->sgt = __sgt_iter(__sg_next(r->sgt.sgp), use_dma(r->iobase)); > > This means that two or more pages from virtual memory are counted for > only one page in object's memory, because after moving "r->sgt" pointer > "r->sgt.curr" will be 0. > > We should account for this mismatch by moving "r->sgt" pointer to the > next pte. For that we may use "r.sgt.max", which already holds the max > allowed size. This change also eliminates possible confusion, when > looking at i915_scatterlist.h and remap_io_sg() code: former has > scatterlist pointer definition, which differentiates "s.max" value > based on "dma" flag (sg_dma_len() is used only when the flag is > enabled), while latter uses sg_dma_len() indiscriminately. > > This patch aims to resolve issue: > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12031 > > > v3: > - instead of checking if r.sgt.curr would exceed allowed max, changed > the value in the while loop to be aligned with `dma` value > > v4: > - remove unnecessary parent relation > > v5: > - update commit message with explanation about page counting mismatch > and link to the issue > > Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> thanks for your patch, merged to drm-intel-gt-next. Thanks, Andi
diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c index f5c97a620962..76e2801619f0 100644 --- a/drivers/gpu/drm/i915/i915_mm.c +++ b/drivers/gpu/drm/i915/i915_mm.c @@ -143,8 +143,8 @@ int remap_io_sg(struct vm_area_struct *vma, /* We rely on prevalidation of the io-mapping to skip track_pfn(). */ GEM_BUG_ON((vma->vm_flags & EXPECTED_FLAGS) != EXPECTED_FLAGS); - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) { - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT; + while (offset >= r.sgt.max >> PAGE_SHIFT) { + offset -= r.sgt.max >> PAGE_SHIFT; r.sgt = __sgt_iter(__sg_next(r.sgt.sgp), use_dma(iobase)); if (!r.sgt.sgp) return -EINVAL;
Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for partial memory mapping") introduced a new offset, which accounts for userspace mapping not starting from the beginning of object's scatterlist. This works fine for cases where first object pte is larger than the new offset - "r->sgt.curr" counter is set to the offset to match the difference in the number of total pages. However, if object's first pte's size is equal to or smaller than the offset, then information about the offset in userspace is covered up by moving "r->sgt" pointer in remap_sg(): r->sgt.curr += PAGE_SIZE; if (r->sgt.curr >= r->sgt.max) r->sgt = __sgt_iter(__sg_next(r->sgt.sgp), use_dma(r->iobase)); This means that two or more pages from virtual memory are counted for only one page in object's memory, because after moving "r->sgt" pointer "r->sgt.curr" will be 0. We should account for this mismatch by moving "r->sgt" pointer to the next pte. For that we may use "r.sgt.max", which already holds the max allowed size. This change also eliminates possible confusion, when looking at i915_scatterlist.h and remap_io_sg() code: former has scatterlist pointer definition, which differentiates "s.max" value based on "dma" flag (sg_dma_len() is used only when the flag is enabled), while latter uses sg_dma_len() indiscriminately. This patch aims to resolve issue: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12031 v3: - instead of checking if r.sgt.curr would exceed allowed max, changed the value in the while loop to be aligned with `dma` value v4: - remove unnecessary parent relation v5: - update commit message with explanation about page counting mismatch and link to the issue Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> --- drivers/gpu/drm/i915/i915_mm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)