On Tue, Jul 03, 2018 at 03:02:11PM +0200, Christian König wrote: > Am 03.07.2018 um 14:52 schrieb Daniel Vetter: > > On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote: > > > Am 25.06.2018 um 11:12 schrieb Daniel Vetter: > > > > On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote: > > > > > On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote: > > > > > > First step towards unpinned DMA buf operation. > > > > > > > > > > > > I've checked the DRM drivers to potential locking of the reservation > > > > > > object, but essentially we need to audit all implementations of the > > > > > > dma_buf _ops for this to work. > > > > > > > > > > > > v2: reordered > > > > > > > > > > > > Signed-off-by: Christian König <christian.koenig@xxxxxxx> > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > > Ok I did review drivers a bit, but apparently not well enough by far. i915 > > > > CI is unhappy: > > > > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@xxxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > > > So yeah inserting that lock in there isn't the most trivial thing :-/ > > > > > > > > I kinda assume that other drivers will have similar issues, e.g. omapdrm's > > > > use of dev->struct_mutex also very much looks like it'll result in a new > > > > locking inversion. > > > Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as > > > bad as this is rather disappointing. > > > > > > Thanks for the info, going to keep thinking about how to solve those issues. > > Side note: We want to make sure that drivers don't get the reservation_obj > > locking hierarchy wrong in other places (using dev->struct_mutex is kinda > > a pre-existing mis-use that we can't wish away retroactively > > unfortunately). One really important thing is that shrinker vs resv_obj > > must work with trylocks in the shrinker, so that you can allocate memory > > while holding reservation objects. > > > > One neat trick to teach lockdep about this would be to have a dummy > > > > if (IS_ENABLED(CONFIG_PROVE_LOCKING)) { > > ww_mutex_lock(dma_buf->resv_obj); > > fs_reclaim_acquire(GFP_KERNEL); > > fs_reclaim_release(GFP_KERNEL); > > ww_mutex_unlock(dma_buf->resv_obj); > > } > > > > in dma_buf_init(). We're using the fs_reclaim_acquire/release check very > > successfully to improve our igt test coverage for i915.ko in other areas. > > > > Totally unrelated to dev->struct_mutex, but thoughts? Well for > > dev->struct_mutex we could at least decide on one true way to nest > > resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how > > much that would help. > > I don't think that would help. As far as I can see we only have two choices: > > 1. Either have a big patch which fixes all DMA-buf implementations to allow > the reservation lock to be held during map/unmap (unrealistic). > > 2. Add a flag to at least in the mid term tell the DMA-buf helper functions > what to do. E.g. create the mapping without the reservation lock held. > > > How about moving the SGL caching from the DRM layer into the DMA-buf layer > and add a flag if the exporter wants/needs this caching? > > Then only the implementations which can deal with dynamic invalidation > disable SGL caching and with it enable creating the sgl with the reservation > object locked. > > This way we can kill two birds with one stone by both avoiding the SGL > caching in the DRM layer as well as having a sane handling for the locking. > > Thoughts? I don't see how the SGL stuff factors into neither the dev->struct_mutex nor into the need to do allocations while holding resv_obj. Neither changes by moving that piece around. At least as far as I can see it SGL caching is fully orthogonal to any kind of locking fun. Why do you see a connection here? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
