Am 21.06.19 um 18:27 schrieb Daniel Vetter: >>> Your scenario here is new, and iirc my suggestion back then was to >>> count the number of pending mappings so you don't go around calling >>> ->invalidate on mappings that don't exist. >> Well the key point is we don't call invalidate on mappings, but we call >> invalidate on attachments. >> >> When the invalidate on an attachment is received all the importer should at >> least start to tear down all mappings. > Hm, so either we invalidate mappings instead (pretty big change for > dma-buf, but maybe worth it). Or importers need to deal with invalidate on > stuff they're don't even have mapped anywhere anyway. I actually I don't see a problem with this, but see below. >> [SNIP] >>> - your scenario, where you call ->invalidate on an attachment which >>> doesn't have a mapping. I'll call that very lazy accounting, feels >>> like a bug :-) It's also very easy to fix by keeping track who >>> actually has a mapping, and then you fix it everywhere, not just for >>> the specific case of a recursion into the same caller. >> Yeah, exactly. Unfortunately it's not so easy to handle as just a counter. >> >> When somebody unmaps a mapping you need to know if that is already >> invalidated or not. And this requires tracking of each mapping. > Yeah we'd need to track mappings. Well, someone has to track mappings, and > atm it seems to be a mix of both importer and exporter (and dma-buf.c). Maybe I'm missing something, but I don't see the mix? Only the importer is responsible to tracking mappings, e.g. the importer calls dma_buf_map_attachment() when it needs a mapping and calls dma_buf_unmap_attachment() when it is done with the mapping. In between those two neither the exporter nor the DMA-buf cares about what mappings currently exist. And apart from debugging I actually don't see a reason why they should. >> [SNIP] >>> But I guess there's other fixes too possible. >>> >>> Either way none of this is about recursion, I think the recursive case >>> is simply the one where you've hit this already. Drivers will have to >>> handle all these additional ->invalidates no matter what with your >>> current proposal. After all the point here is that the exporter can >>> move the buffers around whenever it feels like, for whatever reasons. >> The recursion case is still perfectly valid. In the importer I need to >> ignore invalidations which are caused by creating a mapping. >> >> Otherwise it is perfectly possible that we invalidate a mapping because of >> its creation which will result in creating a new one.... >> >> So even if you fix up your mapping case, you absolutely still need this to >> prevent recursion :) > Hm, but if we stop tracking attachments and instead start tracking > mappings, then how is that possible: Yeah, but why should we do this? I don't see a benefit here. Importers just create/destroy mappings as they need them. > 1. importer has no mappings > 2. importer creates attachment. still no mapping > 3. importer calls dma_buf_attach_map_sg, still no mapping at this point > 4. we call into the exporter implementation. still no mapping > 5. exporter does whatever it does. still no mapping > 6. exporter finishes. conceptually from the dma-buf pov, _this_ is where > the mapping starts to exist. > 7. invalidates (hey the exporter maybe changed its mind!) are totally > fine, and will be serialized with ww_mutex. > > So I kinda don understand why the exporter here is allowed to call > invalidate too early (the mapping doesn't exist yet from dma-buf pov), and > dma-buf needs to filter it out. > > But anywhere else where we might call ->invalidate and there's not yet a > mapping (again purely from dma-buf pov), there the importer is supposed to > do the filter. Maybe this becomes clearer if we call the callback "moved" instead of "invalidated"? I mean this is actually all about the exporter informing the importer that the DMA-buf in question is moving to a new location. That we need to create a new mapping and destroy the old one at some point is an implementation detail on the importer. I mean the long term idea is to use this for notification that a buffer is moving inside the same driver as well. And in this particular case I actually don't think that we would create mappings at all. Thinking more about it this is actually a really good argument to favor the implementation as it is currently. > Someone needs to keep track of all this, and I want clear > responsibilities. What they are exactly is not that important. Clear responsibilities is indeed a good idea. >>> We could also combine the last two with some helpers, e.g. if your >>> exporter really expects importers to delay the unmap until it's no >>> longer in use, then we could do a small helper which puts all these >>> unmaps onto a list with a worker. But I think you want to integrate >>> that into your exporters lru management directly. >>> >>>> So this is just the most defensive thing I was able to come up with, >>>> which leaves the least possibility for drivers to do something stupid. >>> Maybe we're still talking past each another, but I feel like the big >>> issues are all still there. Problem identified, yes, solved, no. >> Yeah, agreed your last mail sounded like we are absolutely on the same page >> on the problem now. > So I pondered a few ideas while working out: > > 1) We drop this filtering. Importer needs to keep track of all its > mappings and filter out invalidates that aren't for that specific importer > (either because already invalidated, or not yet mapped, or whatever). > Feels fragile. > > 2) dma-buf.c keeps track of all the mappings. Will be quite invasive I > think, and will duplicate what either the importer or exporter are already > doing anyway. We might need a map_dynamic_sg so that the invalidate > happens on that, and the invalidate is one-shot (i.e. once unmapped you > can never use the same mapping again, you need to create a new one). Will > probably be quite a bit of code churn. > > 3) Like two, but instead of creating something new we change the semantics > of attachments. For dynamic dma-buf importers, create a new > dma_buf_attach_and_map_sg, which does both, together, atomically. Same for > unmap. For unmap I see a clever option here: These attachements are > one-shot only, i.e. you attach_and_map_sg, then you get an invalidate, > after that the attachment is dead. Can't ever remap it. I think this would > neatly solve the "you've invalidated this already" issue. But it is a bit > more code churn I guess. Also, it breaks the idea of using the attachment > to indicate for which devices a buffer should be accessible, so it's not > accidentally pinned into a bad spot. We might still need that even for > dynamic importers. > > 4) The exporter keeps track of which attachments have a mapping, and > invalidates them individually. Not sure this would actually solve > anything because of the double-invalidate thing. If we go with this we'd > probably need to put the responsibility of delaying the unmap after the > corresponding fence has signalled also onto the exporter. So importer > would unmap from it's ->invalidate callback, but exporter needs to obey > the current fences attached to the resv_obj. Also feels a bit fragile > since it depends upon the exporter instead of shared code to do the right > thing. 5) Just keep it as it is currently implemented, but we work on the documentation to make it clear how it is supposed to work. I mean as far as I can see this is actually doing exactly what is expected. > I think there's probably a few more variants that might work, but those is > what I came up with. I will take a moment and look into #1 as well, but I still don't see the need to change anything. Christian. > -Daniel _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
