10:59 <robmur01> robclark, bbrezillon: unless we go out of our way to merge multiple objects' sg_tables and pass concatenated segments to io_pgtable_ops::map, we should never have "unexpected" blocks to split in the GPU pagetables regardless of whatever the mm layer might have done with the CPU mappings

11:00 <robmur01> (assuming that GEM itself doesn't merge or split entire objects)

11:30 <bbrezillon> robmur01: there should be no split in the map path, but there can be splits in the unmap path, if pancsf VM logic decided to merge 2 physically+virtually contiguous mappings that were not huge-page aligned, and the unmap then undoes that.

11:31 <bbrezillon> well, there can be splits in the the map path is a map operation overlaps a previously mapped section

11:31 <bbrezillon> not sure that if case is allowed with sparse bindings though

11:32 <robmur01> no, io-pgtable would object very strongly to overlapping maps :)

11:32 <bbrezillon> that can be split in an unmap+map operation internally

11:32 <bbrezillon> question is, is it allowed by the vk API

11:34 <bbrezillon> generally speaking, pancsf way of dealing with VA mappings is different from what we had in panfrost, we can partially map/unmap GEMs now

11:35 <bbrezillon> note that I reject any map operation that overlap an existing VA mapping right now (or at least, I intended to do that)

11:38 <robmur01> oh, so vma->bo could be some kind of "partial" object?

11:39 <bbrezillon> yes, it's passed an offset+size

11:39 <bbrezillon> in addition to the BO itself

11:41 <bbrezillon> robmur01: you might want to have a closer look at https://patchwork.kernel.org/project/dri-devel/patch/20230217134422.14116-6-dakr@redhat.com/

11:42 <robmur01> noted, thanks for the pointer

11:42 <bbrezillon> case 7 is the partial map overlap I was mentioning

11:42 <bbrezillon> I think

11:42 <bbrezillon> so I suspect that's a valid Vk sparse binding case

11:43 <bbrezillon> FWIW, I'm planning to use this gpuva_manager in pancsf

11:44 <bbrezillon> don't know what robclark's plans are though

11:47 <robmur01> ah, so IIUC, the map at the gpuva level allows this "replace" semantic, which we'd then implement as a distinct unmap (with split) + map (of the new BO) at the io-pgtable level

11:47 <bbrezillon> yep

11:52 <robmur01> and I guess we can't just demand 2MB alignment for VM_BIND...

11:54 <bbrezillon> Of course, it'd be better to have some atomic remap operation, so we can revert back to the old mapping if the split fails. Once the split is done, I think we can assume the map always succeeds.

11:56 <bbrezillon> According to https://www.asawicki.info/news_1698_vulkan_sparse_binding_-_a_quick_overview, VkMemoryRequirements::​alignment also serves are the binding/mapping alignment, so in theory we could, but I'm not sure we want to force a 2MB alignment, especially for small images/buffers

11:56 <bbrezillon> *serves as

11:58 <bbrezillon> https://registry.khronos.org/vulkan/specs/1.3/html/chap29.html#sparsememory-memory-requirements

13:59 <robclark> bbrezillon, robmur01: bypassing io-pgtable is somewhat awkward for me since many gens == multiple pgtable formats.. bypassing iommu and using io-pgtable for all gens seems kind of tracktable by bypassing io-pgtable less so. Also it is the case on some gens that single smmu has multiple ctx banks, some managed as normal iommu and some w/ io-pgtable. I've been starting to think about vm_bind style API (although I think it will

13:59 <robclark> need to be augmented with some kind of "active set" which is a subset of the vm for browser / non-game use cases) and sparse.. but more as a long term thing not something I'll start working on immediately

14:00 <robclark> but pretty sure I need to make this work w/ io-pgtable, I'm not sure there is an alternative sane option

14:02 <bbrezillon> I think gpuva_manager is one level up. You can still use it and rely on the iopage-table interface to issue map/unmap operations

14:03 <bbrezillon> that was my plan actually

14:06 <robmur01> also bear in mind that the io-pgtable API is shaped to fit the IOMMU API because that's where it started life, but there's no real reason we couldn't extend it further if other users have justifiable needs

14:08 <bbrezillon> I guess the main blocker, if we want to do async map/unmap operations (AKA VM_BIND), is the ability to pass free pages to map/unmap calls and collect freed pages back (so we can keep those in pool instead of releasing immediately)

14:14 <bbrezillon> Having a hook to let the page table implem return the pessimistic number of pages to reserve for a specific operation would be nice, so you don't have to bother having one function per format in the msm driver, but I don't think that's a problem for pancsf (AFAICT, there's just one format)

14:16 <bbrezillon> The other tricky aspect is atomicity when you do a remap operation (which I don't support yet). Ideally, we'd want to be sure that the whole operation succeeds, or things are reverted back to its previous state when the operation fails.

14:20 <robclark> hmm, hadn't really looked at gpuva_manager (have been using drm_mm which seems ok for our uses.. which is moving more towards userspace allocated iova in most cases, ie. other than kernel internal buffers)

14:21 <robclark> re: remap, I wonder if you could do it in passes, ie. first splitting huge-pages where the thing you are replacing the mapping with can't use huge-pages, so the actual remap step is just re-writing pte's

14:22 <robclark> that way, you can't fail

14:22 <robclark> (or if you do fail it is prior to the remap step)

14:22 <bbrezillon> I've been using drm_mm in my first version too, but I've been told it would be good to switch to something dedicated to VA space management, and just around the same time, the nouveau folks came up with this gpuva_manager API

14:23 <bbrezillon> robclark: remap => that's the sort of tricks I had in mind, yes

14:25 <bbrezillon> guess we could want to put things back into huge pages if one of the huge-page split failed

14:25 <robclark> I guess that could be a 3rd "optimize" pass, ie. if the remap succeeds or not?

14:26 <bbrezillon> yep

14:28 <bbrezillon> ok, so that means we'd need a io_pgtable::split() hook/helper

14:29 <robclark> maybe, I've not really thought about implementation details yet ;-)

14:29 <robmur01> those kind of passes have also been proposed for dirty-tracking at the IOMMU API level, so it's definitely not unreasonable

14:30 <bbrezillon> or simply a remap helper that delegates the operation atomicity to the driver

14:30 <bbrezillon> dunno

14:34 <robmur01> I can't see that delegating to drivers would be workable, since the thing being delegated would be very very internal to the particular io-pgtable implementation