[Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Koenig, Christian]

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