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

[Asahi Lina]

On 2/3/25 3:53 AM, Danilo Krummrich wrote:
> 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'd hope we're at least somewhere "this year" close to upstreaming
drm/asahi!

> 
> 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?

Mostly just making sure our driver passes GL/Vulkan CTS including sparse
after this change. I do want to put together some low-level tests in
igt-gpu-tools, but I haven't gotten around to it yet...

~~ Lina