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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 3 Feb 2025 22:46:15 +0900
Asahi Lina <lina@asahilina.net> wrote:

> Hi,
> 
> On 2/3/25 6:21 PM, Boris Brezillon wrote:
> > +Akash with whom we've been discussing adding a 'REPEAT' mode to
> > drm_gpuvm/panthor.
> > 
> > On Sun, 2 Feb 2025 19:53:47 +0100
> > Danilo Krummrich <dakr@kernel.org> 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@asahilina.net>
> >>> ---
> >>> 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?  
> > 
> > Actually, we would be interesting in having this feature hooked up in
> > panthor. One use case we have is Vulkan sparse bindings, of course. But
> > we also have cases where we need to map a dummy page repeatedly on the
> > FW side. The approach we've been considering is slightly different:
> > pass a DRM_GPUVA_REPEAT_FLAG along with GEM range, so we can repeat a
> > range of the GEM (see the below diff, which is completely untested by
> > the way), but I think we'd be fine with this SINGLE_PAGE flag.  
> 
> That sounds similar, though your patch does not handle gpuva
> splitting/remapping and all the other corner cases.

Indeed, I didn't really consider the remapping could be in the middle
of a repeated region, and I see how it complicates things.

> I think you'll find
> that once you handle those, the logic will become significantly more
> complicated, since you need to start storing the start offset within the
> repeat range on GPUVAs to be able to split them while keeping the
> mappings identical, and do modular arithmetic to keep it all consistent
> across all the corner cases.
> 
> If SINGLE_PAGE works for you then I would advocate for that.

I'm perfectly fine with that.

> It keeps
> complexity down to a minimum in drm_gpuvm. You can still have a range
> that's greater than one page in practice, you'd just have to handle it
> driver-internal and pass the desired range out of band as a flag or
> other field. For example, you could decide that the mapping is always
> congruent to the VA (GEM page offset = start offset + VA % range) and
> always treat SINGLE_PAGE mappings like that when you actually set up the
> page tables, or pass in an extra offset to be able to shift the phase of
> the mapping to whatever you want. You just need to ensure that, if you
> mix range sizes or other configuration, you don't do that for the same
> GEM BO at the same offset, so that the drm_gpuvm core does not wrongly
> consider them equivalent.
> 
> Maybe I should rename SINGLE_PAGE to something else, since it isn't
> technically limited to that as far as gpuvm is concerned. Something like
> FIXED_OFFSET?

FWIW, I think I prefer SINGLE_PAGE or REPEAT over FIXED_OFFSET. I mean,
the documentation should clear any confusion, but I like when names are
obvious enough that people can guess their purpose without having to go
read the doc, and I don't think FIXED_OFFSET is clear enough in this
regard.