Re: [PATCH 0/4] drm/gpuvm: Add support for single-page-filled mappings

[Danilo Krummrich]

Hi Lina,

On Sun, Feb 02, 2025 at 10:34:49PM +0900, Asahi Lina wrote:
> Some hardware requires dummy page mappings to efficiently implement
> Vulkan sparse features. These mappings consist of the same physical
> memory page, repeated for a large range of address space (e.g. 16GiB).
> 
> Add support for this to drm_gpuvm. Currently, drm_gpuvm expects BO
> ranges to correspond 1:1 to virtual memory ranges that are mapped, and
> does math on the BO offset accordingly. To make single page mappings
> work, we need a way to turn off that math, keeping the BO offset always
> constant and pointing to the same page (typically BO offset 0).
> 
> To make this work, we need to handle all the corner cases when these
> mappings intersect with regular mappings. The rules are simply to never
> mix or merge a "regular" mapping with a single page mapping.
> 
> drm_gpuvm has support for a flags field in drm_gpuva objects. This is
> normally managed by drivers directly. We can introduce a
> DRM_GPUVA_SINGLE_PAGE flag to handle this. However, to make it work,
> sm_map and friends need to know ahead of time whether the new mapping is
> a single page mapping or not. Therefore, we need to add an argument to
> these functions so drivers can provide the flags to be filled into
> drm_gpuva.flags.
> 
> These changes should not affect any existing drivers that use drm_gpuvm
> other than the API change:
> 
> - imagination: Does not use flags at all
> - nouveau: Only uses drm_gpuva_invalidate(), which is only called on
>   existing drm_gpuva objects (after the map steps)
> - panthor: Does not use flags at all
> - xe: Does not use drm_gpuva_init_from_op() or
>   drm_gpuva_remap()/drm_gpuva_map() (which call it). This means that the
> flags field of the gpuva object is managed by the driver only, so these
> changes cannot clobber it.
> 
> Note that the way this is implemented, drm_gpuvm does not need to know
> the GPU page size. It only has to never do math on the BO offset to meet
> the requirements.
> 
> I suspect that after this change there could be some cleanup possible in
> the xe driver (which right now passes flags around in various
> driver-specific ways from the map step through to drm_gpuva objects),
> but I'll leave that to the Xe folks.
> 
> Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx>
> ---
> Asahi Lina (4):
>       drm/gpuvm: Add a flags argument to drm_gpuvm_sm_map[_*]
>       drm/gpuvm: Plumb through flags into drm_gpuva_op_map
>       drm/gpuvm: Add DRM_GPUVA_SINGLE_PAGE flag and logic
>       drm/gpuvm: Plumb through flags into drm_gpuva_init

Without looking into any details yet:

This is a bit of tricky one, since we're not even close to having a user for
this new feature upstream yet, are we?

I wonder if we could do an exception by adding some KUnit tests (which
admittedly I never got to) validating things with and without this new feature.

Speaking of tests, how did you validate this change to validate the behavior
without DRM_GPUVA_SINGLE_PAGE?

- Danilo