On Fri, Nov 13, 2020 at 03:51:20AM +0000, Xiong, Jianxin wrote: > > > +static void mlx5_ib_dmabuf_invalidate_cb(struct dma_buf_attachment > > > +*attach) { > > > + struct ib_umem_dmabuf *umem_dmabuf = attach->importer_priv; > > > + struct mlx5_ib_mr *mr = umem_dmabuf->private; > > > + > > > + dma_resv_assert_held(umem_dmabuf->attach->dmabuf->resv); > > > + > > > + if (mr) > > > > I don't think this 'if (mr)' test is needed anymore? I certainly prefer it gone as it is kind of messy. I expect unmapping the dma to ensure this > > function is not running, and won't run again. > > It is still needed. When the dma-buf moves, the callback function of every attached importer is invoked, regardless if the importer has mapped the dma or not. > > > > > > +/** > > > + * mlx5_ib_fence_dmabuf_mr - Stop all access to the dmabuf MR > > > + * @mr: to fence > > > + * > > > + * On return no parallel threads will be touching this MR and no DMA > > > +will be > > > + * active. > > > + */ > > > +void mlx5_ib_fence_dmabuf_mr(struct mlx5_ib_mr *mr) { > > > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > > > + > > > + /* Prevent new page faults and prefetch requests from succeeding */ > > > + xa_erase(&mr->dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key)); > > > + > > > + /* Wait for all running page-fault handlers to finish. */ > > > + synchronize_srcu(&mr->dev->odp_srcu); > > > + > > > + wait_event(mr->q_deferred_work, > > > +!atomic_read(&mr->num_deferred_work)); > > > + > > > + dma_resv_lock(umem_dmabuf->attach->dmabuf->resv, NULL); > > > + mlx5_mr_cache_invalidate(mr); > > > + umem_dmabuf->private = NULL; > > > + ib_umem_dmabuf_unmap_pages(umem_dmabuf); > > > + dma_resv_unlock(umem_dmabuf->attach->dmabuf->resv); > > > + > > > + if (!mr->cache_ent) { > > > + mlx5_core_destroy_mkey(mr->dev->mdev, &mr->mmkey); > > > + WARN_ON(mr->descs); > > > + } > > > > I didn't check carefully, but are you sure this destroy_mkey should be here?? > > To my understanding, yes. This is similar to what dma_fence_odp_mr() does, > just inlined here since it's not called from other places. I think you should put the calls to dma_buf_dynamic_attach() and dma_buf_detach() into mlx5, it makes the whole thing a little cleaner, then the umem->private isn't needed any more either. Jason _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel