On Thu, Oct 30, 2025 at 06:17:11AM +0000, Kasireddy, Vivek wrote: > It mostly looks OK to me but there are a few things that I want to discuss, > after briefly looking at the patches in your branch: > - I am wondering what is the benefit of the SGT compatibility stuff especially > when Christian suggested that he'd like to see SGT usage gone from > dma-buf
I think to get rid of SGT we do need to put it in a little well defined box and then create alternatives and remove things using SGT. This is a long journey, and I think this is the first step. If SGT is some special case it will be harder to excise. So the next steps would be to make all the exporters directly declare a SGT and then remove the SGT related ops from dma_ops itself and remove the compat sgt in the attach logic. This is not hard, it is all simple mechanical work. This way the only compat requirement is to automatically give an import match list for a SGT only importer which is very little code in the core. The point is we make the SGT stuff nonspecial and fully aligned with the mapping type in small steps. This way neither importer nor exporter should have any special code to deal with interworking. To remove SGT we'd want to teach the core code how to create some kind of conversion mapping type, eg exporter uses SGT importer uses NEW so the magic conversion mapping type does the adapatation. In this way we can convert importers and exporters to use NEW in any order and they still interwork with each other. > eventually. Also, if matching fails, IMO, indicating that to the > importer (allow_ic) and having both exporter/importer fallback to > the current legacy mechanism would be simpler than the SGT > compatibility stuff. I don't want to have three paths in importers. If the importer supports SGT it should declare it in a match and the core code should always return a SGT match for the importer to use The importer should not have to code 'oh it is sgt but it somehow a little different' via an allow_ic type idea. > - Also, I thought PCIe P2P (along with SGT) use-cases are already well handled > by the existing map_dma_buf() and other interfaces. So, it might be confusing > if the newer interfaces also provide a mechanism to handle P2P although a > bit differently. I might be missing something here but shouldn't the existing > allow_peer2peer and other related stuff be left alone? P2P is part of SGT, it gets pulled into the SGT stuff as steps toward isolating SGT properly. Again as we move things to use native SGT exporters we would remove the exporter related allow_peer2peer items when they become unused. > - You are also adding custom attach/detach ops for each mapping_type. I think > it makes sense to reuse existing attach/detach ops if possible and initiate > the > matching process from there, at-least initially. I started there, but as soon as I went to adding PAL I realized the attach/detach logic was completely different for each of the mapping types. So this is looking alot simpler. If the driver wants to share the same attach/detach ops for some of its mapping types then it can just set the same function pointer to all of them and pick up the mapping type from the attach->map_type. > - Looks like your design doesn't call for a dma_buf_map_interconnect() or > other > similar helpers provided by dma-buf core that the importers can use. Is that > because the return type would not be known to the core? I don't want to have a single shared 'map' operation, that is the whole point of this design. Each mapping type has its own ops, own types, own function signatures that the client calls directly. No more type confusion or trying to abuse phys_addr_t, dma_addr_t, or scatterlist for in appropriate things. If your driver wants something special, like IOV, then give it proper clear types so it is understandable. > - And, just to confirm, with your design if I want to add a new interconnect/ > mapping_type (not just IOV but in general), all that is needed is to provide > custom > attach/detach, match ops and one or more ops to map/unmap the address list > right? Does this mean that the role of dma-buf core would be limited to just > match and the exporters are expected to do most of the heavy lifting and > checking for stuff like dynamic importers, resv lock held, etc? I expect the core code would continue to provide wrappers and helpers to call the ops that can do any required common stuff. However, keep in mind, when the importer moves to use mapping type it also must be upgraded to use the dynamic importer flow as this API doesn't support non-dynamic importers using mapping type. I will add some of these remarks to the commit messages.. Thanks! Jason
