On Mon, 3 Feb 2025 22:46:15 +0900 Asahi Lina <lina@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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? > > > > 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.
