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

[Koenig, Christian]

Am 26.06.19 um 10:17 schrieb Daniel Vetter:
> On Wed, Jun 26, 2019 at 09:49:03AM +0200, Christian König wrote:
>> Am 25.06.19 um 18:05 schrieb Daniel Vetter:
>>> On Tue, Jun 25, 2019 at 02:46:49PM +0200, Christian König wrote:
>>>> On the exporter side we add optional explicit pinning callbacks. If those
>>>> callbacks are implemented the framework no longer caches sg tables and the
>>>> map/unmap callbacks are always called with the lock of the reservation object
>>>> held.
>>>>
>>>> On the importer side we add an optional invalidate callback. This callback is
>>>> used by the exporter to inform the importers that their mappings should be
>>>> destroyed as soon as possible.
>>>>
>>>> This allows the exporter to provide the mappings without the need to pin
>>>> the backing store.
>>>>
>>>> v2: don't try to invalidate mappings when the callback is NULL,
>>>>       lock the reservation obj while using the attachments,
>>>>       add helper to set the callback
>>>> v3: move flag for invalidation support into the DMA-buf,
>>>>       use new attach_info structure to set the callback
>>>> v4: use importer_priv field instead of mangling exporter priv.
>>>> v5: drop invalidation_supported flag
>>>> v6: squash together with pin/unpin changes
>>>> v7: pin/unpin takes an attachment now
>>>> v8: nuke dma_buf_attachment_(map|unmap)_locked,
>>>>       everything is now handled backward compatible
>>>> v9: always cache when export/importer don't agree on dynamic handling
>>>> v10: minimal style cleanup
>>>> v11: drop automatically re-entry avoidance
>>>> v12: rename callback to move_notify
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> One thing I've forgotten, just stumbled over ttm_bo->moving. For pinned
>>> buffer sharing that's not needed, and I think for dynamic buffer sharing
>>> it's also not going to be the primary requirement. But I think there's two
>>> reasons we should maybe look into moving that from ttm_bo to resv_obj:
>> That is already part of the resv_obj. The difference is that radeon is
>> overwriting the one in the resv_obj during CS while amdgpu isn't.
> I'm confused here: Atm ->moving isn't in resv_obj, there's only one
> exclusive fence. And yes you need to set that every time you do a move
> (because a move needs to be pretty exclusive access). But I'm not seeing a
> separate not_quite_exclusive fence slot for moves.

Yeah, but shouldn't that be sufficient? I mean why does somebody else 
than the exporter needs to know when a BO is moving?

>> So for amdgpu we keep an extra copy in ttm_bo->moving to keep the page fault
>> handler from unnecessary waiting for a fence in radeon.
> Yeah that's the main one. The other is in CS (at least for i915) we could
> run pipeline texture uploads in parallel with other rendering and stuff
> like that (with multiple engines, which atm is also not there yet). I
> think that could be somewhat useful for vk drivers.
>
> Anyway, totally not understand what you wanted to tell me here in these
> two lines.

Sorry it's 33C in my home office here and I mixed up radeon/amdgpu in 
the sentence above.

>>> - You sound like you want to use this a lot more, even internally in
>>>     amdgpu. For that I do think the sepearate dma_fence just to make sure
>>>     the buffer is accessible will be needed in resv_obj.
>>>
>>> - Once we have ->moving I think there's some good chances to extract a bit
>>>     of the eviction/pipeline bo move boilerplate from ttm, and maybe use it
>>>     in other drivers. i915 could already make use of this in upstream, since
>>>     we already pipeline get_pages and clflush of buffers. Ofc once we have
>>>     vram support, even more useful.
>> I actually indeed wanted to add more stuff to the reservation object
>> implementation, like finally cleaning up the distinction of readers/writers.
> Hm, more details? Not ringing a bell ...

I'm not yet sure about the details either, so please just wait until I 
solved that all up for me first.

>> And cleaning up the fence removal hack we have in the KFD for freed up BOs.
>> That would also allow for getting rid of this in the long term.
> Hm, what's that for?

When the KFD frees up memory it removes their eviction fence from the 
reservation object instead of setting it as signaled and adding a new 
one to all other used reservation objects.

Christian.

> -Daniel
>
>> Christian.
>>
>>> And doing that slight semantic change is much easier once we only have a
>>> few dynamic exporters/importers. And since it's a pure opt-in optimization
>>> (you can always fall back to the exclusive fence) it should be easy to
>>> roll out.
>>>
>>> Thoughts about moving ttm_bo->moving to resv_obj? Ofc strictly only as a
>>> follow up. Plus maybe with a clearer name :-)
>>>
>>> Cheers, Daniel
>>>