dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Brost <matthew.brost@intel.com>
To: "Huang, Honglei" <honghuan@amd.com>
Cc: <sima@ffwll.ch>, <rodrigo.vivi@intel.com>,
	<thomas.hellstrom@linux.intel.com>, <dakr@kernel.org>,
	<intel-xe@lists.freedesktop.org>, <aliceryhl@google.com>,
	<Alexander.Deucher@amd.com>, <Felix.Kuehling@amd.com>,
	<Christian.Koenig@amd.com>, <Ray.Huang@amd.com>,
	<Lingshan.Zhu@amd.com>, <Junhua.Shen@amd.com>, <Yiru.Ma@amd.com>,
	<amd-gfx@lists.freedesktop.org>,
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v10 0/5] drm/gpusvm: split MM and device state across gpusvm/range/pages
Date: Wed, 8 Jul 2026 15:56:22 -0700	[thread overview]
Message-ID: <ak7VlleUwy65QLtg@gsse-cloud1.jf.intel.com> (raw)
In-Reply-To: <4c0d3658-9480-43b9-9c33-667238697d1a@amd.com>

On Wed, Jul 01, 2026 at 11:34:57AM +0800, Huang, Honglei wrote:
> 
> 
> On 6/30/2026 10:26 PM, Matthew Brost wrote:
> > On Tue, Jun 30, 2026 at 06:21:22PM +0800, Honglei Huang wrote:
> > > The intent of this series is to make drm_gpusvm more flexible and give
> > > drivers more freedom over how they assemble the MM related and device
> > > side operations. It implements the direction Matt suggested in [1]:
> > > Mirror MR in gitlab: [4]
> > > 
> > >    - Move struct drm_gpusvm_pages out of struct drm_gpusvm_range.
> > >    - Embed a struct drm_device in struct drm_gpusvm_pages and drive all
> > >      DMA through it.
> > >    - Drop struct drm_device from struct drm_gpusvm.
> > >    - Have the driver's range structure embed one or more struct
> > >      drm_gpusvm_pages in addition to struct drm_gpusvm_range.
> > >    - Drop the range-based helpers (drm_gpusvm_range_pages_valid,
> > >      drm_gpusvm_range_get_pages, drm_gpusvm_range_unmap_pages) and update
> > >      drivers to use the drm_gpusvm_pages helpers instead.
> > > 
> > > In essence the series does only two abstractions, plus the xe
> > > adaptation that follows from them:
> > > 
> > >    - range vs pages: split drm_gpusvm_range (MM / VA range state) from
> > >      drm_gpusvm_pages (device physical related), so the two sides can
> > >      have independent lifetimes and ownership.
> > >    - drm_gpusvm vs drm_device: make drm_gpusvm pure MM level and push
> > >      the device side down onto drm_gpusvm_pages, which is where DMA
> > >      actually happens.
> > >    - xe is updated to fit the modifications, no functional change
> > >      intended.
> > > 
> > > V10:
> > >    Fix two issues found by the AI review:
> > >    - patch 1: fix a KCSAN data race in xe_svm_alloc_vram(): read
> > >      range->base.flags.__flags with READ_ONCE() and assert on a local
> > >      copy, pairing with the WRITE_ONCE() in
> > >      drm_gpusvm_range_set_unmapped().
> > >    - patch 3: remove the DMA unmap in xe_svm_fini(): since
> > >      drm_gpusvm_range_remove() no longer unmaps synchronously, explicitly
> > >      drm_gpusvm_unmap_pages() all remaining ranges before
> > >      drm_gpusvm_fini().
> > > 
> > > V9:
> > >    - patch 3: fix the build with CONFIG_DRM_XE_GPUSVM disabled: move
> > >      pages out of the nested base struct in the stub xe_svm_range and
> > >      route has_dma_mapping through range->pages. No functional change.
> > > 
> > > V8:
> > >    - patch 4: add reviewed-by for Matt's review.
> > > 
> > > V7:
> > >    - patch 1: split MM state flags: the AI review found a KCSAN / memory
> > >      model cleanliness issue. Address it for consistency with
> > >      drm_gpusvm_pages_flags, set the range flags with WRITE_ONCE() on
> > >      __flags and read them with READ_ONCE().
> > > 
> > > V6:
> > >    - The AI review flagged a potential DMA free issue: the DMA unmap
> > >      step was moved into the range_free callback, but on the invalidate
> > >      path a range can be removed from the MMU interval tree while its DMA
> > >      mappings are still live, so a concurrent unmap event can miss it.
> > >    - patch 3: have xe_svm_range embed one drm_gpusvm_pages: explicitly
> > >      call drm_gpusvm_unmap_pages() before drm_gpusvm_range_remove() in the
> > >      garbage collector, so a range is never off the tree while still DMA
> > >      mapped, and document this caller contract in drm_gpusvm_range_remove()
> > >      kernel-doc.
> > >    - patch 4: move struct drm_gpusvm_pages out: document the
> > >      unmap before remove contract in the garbage collector example and
> > >      note that range_free()'s drm_gpusvm_free_pages() as a final fallback.
> > >    - patch 1: split MM state flags: return -EACCES directly.
> > >    - Fold in the pre existing IOVA/DMA unmap fixes the AI review found
> > >      previously sent separately: the uninitialized dma_addr[0].dir on
> > >      the get_pages() error path, the whole reservation IOVA free for
> > >      mixed ranges, and the device mapping leak on the get_pages() error
> > >      path. [6]
> > > 
> > > V5:
> > >    - add reviewed-by in patches 1, 2, 3, 5 for Matt's review.
> > > 
> > > V4:
> > >    - drm_gpusvm_init_pages(): memset() the pages to zero before recording
> > >      the owning drm_device.
> > >    - DOC: overview: recommend a zeroing allocator: kcalloc() for the
> > >      N:1 pages array.
> > >    - Rebased onto the latest drm-xe.
> > >    - The AI review of this series flagged two preexisting issues in the
> > >      IOVA unmap path that are not introduced by this series; they are
> > >      fixed in a separate series [5].
> > > 
> > > V3:
> > >    - Fix a kernel-doc/Sphinx warning from the kernel test robot: use
> > >      ".. code-block:: c" for the drm_gpusvm_pages example in DOC: overview.
> > >    - drm_gpusvm_range_set_unmapped(): use WRITE_ONCE() on the whole
> > >      pages[i].flags.__flags word to pair with the lockless READ_ONCE()
> > >      readers and avoid a data race.
> > >    - xe_userptr_setup(): call drm_gpusvm_init_pages() before
> > >      mmu_interval_notifier_insert() to avoid exposing uninitialized
> > >      pages.drm to invalidation callbacks.
> > >    - Fix per commit build of the set_unmapped() pages.
> > > 
> > > V2:
> > >    - Followed in Matt's v0 review fixups [2]:
> > >       - keep unmapped flag in pages structures.
> > >       - add pages_count to drm_gpusvm_range_set_unmapped() to set the pages
> > >         unmapped flag, so the framework can check unmapped status in
> > >         drm_gpusvm_get_pages().
> > >    - Add drm_gpusvm_init_pages to init the drm_device and sequence number.
> > >    - Remove drm_device from drm_gpusvm_get_pages() parameters.
> > >    - Reworked the DOC: overview and usage examples to describe the new
> > >      model: struct drm_gpusvm_pages, the 1:1 / N:1 driver layouts, and
> > >      examples that operate on a driver embedded pages object by the
> > >      drm_gpusvm_pages helpers and etc.
> > >    - remove WARN_ON_ONCE in __drm_gpusvm_unmap_pages.
> > >    - Dropped RFC.
> > > 
> > > Follow-up (not in this series):
> > > 
> > >    - modify drm_gpusvm_get_pages() to support one time hmm range fault
> > >      and multi drm device dma mapping.
> > >    - Add no dma device support for drm_gpusvm_get_pages().
> > > 
> > > tests:
> > > AMDGPU:
> > >    based on amdgpu adaptation patch in [3], but still SVM:DRM = 1:1,
> > >    1:n is on going needs many modifications and testings.
> > > 
> > >    Tested on gfx943 (MI300X) and gfx906 (MI60) with XNACK on/off:
> > >    - KFD test: 95%+ passed.
> > >    - ROCR test: all passed.
> > >    - HIP catch test: gfx943 (MI300X): 99% passed.
> > >                      gfx906 (MI60): 99% passed.
> > > INTEL XE:
> > >    CI tests passed in rev5 tests.[7]
> > 
> > 
> > CI full is in flight, but it seems likely to pass. Once it completes, we
> > can merge this; we just need to decide which branch to use.
> > 
> > The series does not apply cleanly to drm-misc-next because it depends on
> > changes in drm-xe-next that have not yet been merged into drm-misc-next.
> > It does apply cleanly to drm-xe-next, though.
> > 
> > If we merge the patches in drm-xe-next and pick them up via a rebase in
> > the AMD trees, that works. Alternatively, we can ask the drm-misc-next
> > maintainers to rebase and apply them there. Either option works for me.
> > 
> 
> Thanks a lot, Matt, for the guidance and the reviews in this series.
> 
> For the merge, happy to rebase onto whatever base/tag decided, just
> let me know.
> 
> And will work on the follow up works.

This now applies to drm-misc-next, so going to apply there by EOD unless
I hear an objection.

Matt

> 
> Regards,
> Honglei
> 
> > Matt
> > 
> > > 
> > > links:
> > > [1] https://lore.kernel.org/amd-gfx/acRgr7QwdULsn6G2@gsse-cloud1/#:~:text=I%20think%20roughly,drm_gpusvm_pages%0A%20%20helpers%20instead.
> > > [2] https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commit/623f6a50c037d9e44f6c9fbe6859a0ba7ad50177
> > > [3] https://lore.kernel.org/amd-gfx/20260603065030.2554403-1-honglei1.huang@amd.com/
> > > [4] https://gitlab.freedesktop.org/drm/xe/kernel/-/merge_requests/360
> > > [5] https://lore.kernel.org/all/20260627033325.3795298-1-honglei1.huang@amd.com/
> > > [6] https://lore.kernel.org/all/20260628061757.4093701-1-honglei1.huang@amd.com/
> > > [7] https://patchwork.freedesktop.org/series/169384/#rev5
> > > 
> > > Honglei Huang (5):
> > >    drm/gpusvm: split MM state flags out of drm_gpusvm_pages_flags
> > >    drm/gpusvm: embed struct drm_device into drm_gpusvm_pages
> > >    drm/xe: have xe_svm_range embed one drm_gpusvm_pages
> > >    drm/gpusvm: move struct drm_gpusvm_pages out of struct
> > >      drm_gpusvm_range
> > >    drm/gpusvm: let the drm_gpusvm core context purely MM level
> > > 
> > >   drivers/gpu/drm/drm_gpusvm.c    | 243 +++++++++++++++++++-------------
> > >   drivers/gpu/drm/xe/xe_pt.c      |   2 +-
> > >   drivers/gpu/drm/xe/xe_svm.c     |  66 +++++++--
> > >   drivers/gpu/drm/xe/xe_svm.h     |  14 +-
> > >   drivers/gpu/drm/xe/xe_userptr.c |   5 +-
> > >   include/drm/drm_gpusvm.h        |  67 ++++++---
> > >   6 files changed, 255 insertions(+), 142 deletions(-)
> > > 
> > > -- 
> > > 2.34.1
> > > 
> 

  reply	other threads:[~2026-07-08 22:56 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-30 10:21 [PATCH v10 0/5] drm/gpusvm: split MM and device state across gpusvm/range/pages Honglei Huang
2026-06-30 10:21 ` [PATCH v10 1/5] drm/gpusvm: split MM state flags out of drm_gpusvm_pages_flags Honglei Huang
2026-06-30 10:21 ` [PATCH v10 2/5] drm/gpusvm: embed struct drm_device into drm_gpusvm_pages Honglei Huang
2026-06-30 10:21 ` [PATCH v10 3/5] drm/xe: have xe_svm_range embed one drm_gpusvm_pages Honglei Huang
2026-06-30 10:21 ` [PATCH v10 4/5] drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range Honglei Huang
2026-06-30 10:21 ` [PATCH v10 5/5] drm/gpusvm: let the drm_gpusvm core context purely MM level Honglei Huang
2026-06-30 14:26 ` [PATCH v10 0/5] drm/gpusvm: split MM and device state across gpusvm/range/pages Matthew Brost
2026-06-30 14:33   ` Danilo Krummrich
2026-07-01 15:56     ` Thomas Hellström
2026-07-01 16:26       ` Danilo Krummrich
2026-07-01  3:34   ` Huang, Honglei
2026-07-08 22:56     ` Matthew Brost [this message]
2026-07-09  6:26       ` Huang, Honglei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ak7VlleUwy65QLtg@gsse-cloud1.jf.intel.com \
    --to=matthew.brost@intel.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Christian.Koenig@amd.com \
    --cc=Felix.Kuehling@amd.com \
    --cc=Junhua.Shen@amd.com \
    --cc=Lingshan.Zhu@amd.com \
    --cc=Ray.Huang@amd.com \
    --cc=Yiru.Ma@amd.com \
    --cc=aliceryhl@google.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=dakr@kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=honghuan@amd.com \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=rodrigo.vivi@intel.com \
    --cc=sima@ffwll.ch \
    --cc=thomas.hellstrom@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox