[PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Koenig, Christian]

Am 06.05.19 um 10:04 schrieb Daniel Vetter:
> [SNIP]
>>>> + /* pin buffer into GTT */
>>>> + return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
>>> This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
>>> this can fail is someone already pinned the buffer into vram. And that
>>> kind of checking is supposed to happen in the buffer attachment.
>> Why is that supposed to happen on the attachment? I mean it could be nice to
>> have for debugging, but I still don't see any practical reason for this.
> dma_buf_attach is supposed to make sure the buffer won't land somewhere
> where you can't get at it anymore. Wrt pin that means the exporter needs
> to make sure it can't get pinned into a wrong place, and also isn't pinned
> into a wrong place anymore. That's why I think pinning ties in with
> dma_buf_attach and not the overall buffer.
>
> In a way there's two pieces of a pin:
> - Do not move the buffer anymore.
> - Make sure I can still get at it.
>
> Internally the 2nd part is encoded in the domain parameter you pass to
> amdgpu_bo_pin. When going through dma-buf that information is derived
> from the attachment (e.g. if it's a p2p one, then you can put it wherever
> you feel like, if it's a normal one it must be in system ram). The dma-buf
> alone doesn't tell you _where_ to pin something.

Ok, that finally makes some sense. So the attachment describes where the 
buffer needs to be for the attaching device/use case to be able to 
access it.

Going to change it to use an attachment instead.

>> Well completely amdgpu internal handling here. Key point is we have both
>> preferred_domains as well as allowed_domains.
>>
>> During command submission we always try to move a BO to the
>> preferred_domains again.
>>
>> Now what could happen if we don't have this check is the following:
>>
>> 1. BO is allocate in VRAM. And preferred_domains says only VRAM please, but
>> allowed_domains says VRAM or GTT.
>>
>> 2. DMA-buf Importer comes along and moves the BO to GTT, which is perfectly
>> valid because of the allowed_domains.
>>
>> 3. Command submission is made and moves the BO to VRAM again.
>>
>> 4. Importer comes along and moves the BO to GTT.
>> ....
>>
>> E.g. a nice ping/pong situation which just eats up memory bandwidth.
> Hm yeah the ping/pong is bad, but I figure you have to already handle that
> (with some bias or whatever). Outright disabling invalidate/dynamic
> dma-buf seems like overkill.
>
> What about upgradging preferred_domains to include GTT here? Defacto what
> you do is forcing GTT, so just adding GTT as a possible domain seems like
> the better choice. Bonus points for undoing that when the last importer
> disappears.

Well that's exactly what we want to avoid here.

The preferred_domains is where userspace likes the buffer to be and 
should never be changed by the kernel.

The allowed_domains is where the buffer should be based on the hardware 
restrictions and is usually updated by the kernel driver.

> In general I think dynamic dma-buf needs to be able to handle this
> somehow, or it won't really work. Simplest is probably to just stop moving
> buffers around so much for buffers that are dynamically exported (so maybe
> could also change that in the CS ioctl to not move exported buffers
> anymore, would achieve the same).

Yeah, that's the obvious alternative. But I didn't wanted to add even 
more complexity to the patch right now.

Cleaning this up is pure amdgpu internally, e.g. we need to make sure to 
not move buffers around so much on command submission.

Christian.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch