[PATCH v5 0/3] drm/gpusvm, drm/pagemap, drm/xe: Restructure migration in preparation for multi-device

[Thomas Hellström]

Hi, Alistair, Thanks for having a look!

On Thu, 2025-06-19 at 14:52 +1000, Alistair Popple wrote:
> On Wed, Jun 18, 2025 at 10:16:14PM +0200, Thomas Hellström wrote:
> > This patchset modifies the migration part of drm_gpusvm to
> > drm_pagemap and
> > adds a populate_mm() op to drm_pagemap.
> > 
> > The idea is that the device that receives a pagefault determines if
> > it wants to
> > migrate content and to where. It then calls the populate_mm()
> > method of relevant
> > drm_pagemap.
> > 
> > This functionality was mostly already in place, but hard-coded for
> > xe only without
> > going through a pagemap op. Since we might be dealing with separate
> > devices moving
> > forward, it also now becomes the responsibilit of the populate_mm()
> > op to
> > grab any necessary local device runtime pm references and keep them
> > held while
> > its pages are present in an mm (struct mm_struct).
> > 
> > On thing to decide here is whether the populate_mm() callback
> > should sit on a
> > struct drm_pagemap for now while we sort multi-device usability out
> > or whether
> > we should add it (or something equivalent) to struct dev_pagemap.
> 
> I'm still looking at this series (sorry it took until v5 for me to
> notice
> it!) but my immediate reaction here is why do/would you need to add
> anything
> to struct dev_pagemap? The common approach here has been to embed
> struct
> dev_pagemap in some driver defined struct and use container_of to go
> from the
> page to the driver (or in this case DRM) specific pagemap.
> 
> See for example dmirror_page_to_chunk() in the HMM self test or
> nouveau_page_to_chunk(). Is there some reason something like that
> would work
> here?

Future patches will, as they are currently written, do something like
this for embedding:

struct xe_pagemap {
	struct dev_pagemap pagemap;
	struct drm_pagemap dpagemap;
};

So that the drm_pagemap can be obtained that way. The reason for that
is to avoid diamond inheritance if the driver wanted to embed a
pcie_p2p pagemap instead of a struct dev_pagemap. But if that becomes
unlikely we could of course embed the dev_pagemap directly into the
drm_pagemap.


> 
> Actually I notice the Xe driver currently does use this to point to a
> struct
> xe_vram_region which contains drm_pagemap pointer. If I understand
> correctly
> we're trying to move a lot of the SVM functionality into a generic
> DRM layer,
> so would it make more sense to have dev_pgmap embeded in drm_pgmap
> and have that
> contain the pointer to any required driver-specific data?
> 
> Also FWIW I don't think zone_device_data is strictly required. It's
> convenient,
> but I suspect it only exists because it could be easily provided
> within the
> footprint of the existing struct page due to not using all the fields
> for
> ZONE_DEVICE pages. I can imagine we might eventually remove it, once
> we no
> longer need struct pages and move to folios/memdescs.

It looks like, correct me if I'm wrong, like the nouveau version has
one dev_pagemap per bo. The code at hand here use multiple bos for the
memory of a single pagemap so in essence the zone_device_data provides
a pointer to the bo used for that particular page. If we lose the
zone_device_data there is probably other ways we can do that backwards
lookup, but it may become nasty.

So while I fully agree we should use some form of embedding of the
dev_pagemap, and that could be the authoritative way to go from a page
to a drm_pagemap, that is not really sufficient to go from a page to a
bo.

But more on this in upcoming multi-device patches. This series is
mostly about separating the drm_gpusvm (GPU mapping) and
drm_pagemap(migration) functionality that is already i place.

But please let me know if there are any concerns with this.

Thanks,
Thomas


> 
> > v2:
> > - Rebase.
> > v3:
> > - Documentation updates (CI, Matt Brost)
> > - Don't change TTM buffer object type for VRAM allocations (Matt
> > Brost)
> > v4:
> > - Documentation Updates (Himal Ghimiray, Matt Brost)
> > - Add an assert (Matt Brost)
> > v5:
> > - Rebase
> > - Add R-Bs and SOBs.
> > 
> > Matthew Brost (1):
> >   drm/gpusvm, drm/pagemap: Move migration functionality to
> > drm_pagemap
> > 
> > Thomas Hellström (2):
> >   drm/pagemap: Add a populate_mm op
> >   drm/xe: Implement and use the drm_pagemap populate_mm op
> > 
> >  Documentation/gpu/rfc/gpusvm.rst     |  12 +-
> >  drivers/gpu/drm/Makefile             |   6 +-
> >  drivers/gpu/drm/drm_gpusvm.c         | 761 +----------------------
> > -
> >  drivers/gpu/drm/drm_pagemap.c        | 838
> > +++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/Kconfig           |  10 +-
> >  drivers/gpu/drm/xe/xe_bo_types.h     |   2 +-
> >  drivers/gpu/drm/xe/xe_device_types.h |   2 +-
> >  drivers/gpu/drm/xe/xe_svm.c          | 125 ++--
> >  drivers/gpu/drm/xe/xe_svm.h          |  10 +-
> >  drivers/gpu/drm/xe/xe_tile.h         |  11 +
> >  drivers/gpu/drm/xe/xe_vm.c           |   2 +-
> >  include/drm/drm_gpusvm.h             |  96 ---
> >  include/drm/drm_pagemap.h            | 135 +++++
> >  13 files changed, 1098 insertions(+), 912 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_pagemap.c
> > 
> > -- 
> > 2.49.0
> >