On Mon, Mar 12, 2018 at 8:15 PM, Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 12.03.2018 um 18:24 schrieb Daniel Vetter:
On Fri, Mar 09, 2018 at 08:11:40PM +0100, Christian K??nig wrote:
This set of patches adds an option invalidate_mappings callback to each DMA-buf attachment which can be filled in by the importer.
This callback allows the exporter to provided the DMA-buf content without pinning it. The reservation objects lock acts as synchronization point for buffer moves and creating mappings.
This set includes an implementation for amdgpu which should be rather easily portable to other DRM drivers.
Bunch of higher level comments, and one I've forgotten in reply to patch 1:
- What happens when a dma-buf is pinned (e.g. i915 loves to pin buffers for scanout)?
When you need to pin an imported DMA-buf you need to detach and reattach without the invalidate_mappings callback.
I think that must both be better documented, and also somehow enforced with checks. Atm nothing makes sure you actually manage to unmap if you claim to be able to do so.
I think a helper to switch from pinned to unpinned would be lovely (just need to clear/reset the ->invalidate_mapping pointer while holding the reservation). Or do you expect to map buffers differently depending whether you can move them or not? At least for i915 we'd need to rework our driver quite a bit if you expect us to throw the mapping away just to be able to pin it. Atm pinning requires that it's mapped already (and depending upon platform the gpu might be using that exact mapping to render, so unmapping for pinning is a bad idea for us).
- pulling the dma-buf implementations into amdgpu makes sense, that's kinda how it was meant to be anyway. The gem prime helpers are a bit
too much midlayer for my taste (mostly because nvidia wanted to bypass the EXPORT_SYMBOL_GPL of core dma-buf, hooray for legal bs). We can always extract more helpers once there's more ttm based drivers doing this.
Yeah, I though to abstract that similar to the AGP backend.
Just moving some callbacks around in TTM should be sufficient to de-midlayer the whole thing.
Yeah TTM has all the abstractions needed to handle dma-bufs "properly", it's just sometimes at the wrong level or can't be overriden. At least per my understanding of TTM (which is most likely ... confused). -Daniel