On Wed, Jul 10, 2013 at 2:03 PM, Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx> wrote: > Op 10-07-13 13:54, Daniel Vetter schreef: >> I've checked both implementations (radeon/nouveau) and they both grab >> the page array from ttm simply by dereferencing it and then wrapping >> it up with drm_prime_pages_to_sg in the callback and map it with >> dma_map_sg (in the helper). >> >> Only the grabbing of the underlying page array is anything we need to >> be concerned about, and either those pages are pinned independently, >> or we're screwed no matter what. >> >> And indeed, nouveau/radeon pin the backing storage in their >> attach/detach functions. >> >> The only thing we might claim it does is prevent concurrent mapping of >> dma_buf attachments. But a) that's not allowed and b) the current code >> is racy already since it checks whether the sg mapping exists _before_ >> grabbing the lock. >> >> So the dev->struct_mutex locking here does absolutely nothing useful, >> but only distracts. Remove it. >> >> This should also help Maarten's work to eventually pin the backing >> storage more dynamically by preventing locking inversions around >> dev->struct_mutex. > > This pleases me, but I think it's not thorough enough. > > if (prime_attach->dir == dir) > return prime_attach->sgt; > > ^ That check must go too. I don't think recursive map_dma_buf is valid. > > and unmap_dma_buf should set prime_attach->dir = DMA_NONE; again. So after a bit of irc chatting with Maarten this seems to be more involved. The above check is to cache the dma mapping, but the implementation is bogus in tons of ways: - If direction changes we don't bother with unmaping and freeing the mapping, but simply leak it. - This will break if the dma mapping needs explicit syncing since the helpers don't call sync_to_cpu/sync_to_device anywhere. So I think I'll decline to poke around more in this hornet nest and leave it at the locking removal. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
