Am 18.04.19 um 10:08 schrieb Daniel Vetter: > On Wed, Apr 17, 2019 at 09:13:22PM +0200, Christian König wrote: >> Am 17.04.19 um 21:07 schrieb Daniel Vetter: >>> On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote: >>>> Each importer can now provide an invalidate_mappings callback. >>>> >>>> This allows the exporter to provide the mappings without the need to pin >>>> the backing store. >>>> >>>> v2: don't try to invalidate mappings when the callback is NULL, >>>> lock the reservation obj while using the attachments, >>>> add helper to set the callback >>>> v3: move flag for invalidation support into the DMA-buf, >>>> use new attach_info structure to set the callback >>>> v4: use importer_priv field instead of mangling exporter priv. >>>> v5: drop invalidation_supported flag >>>> >>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx> >>>> --- >>>> drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> include/linux/dma-buf.h | 33 +++++++++++++++++++++++++++++++-- >>>> 2 files changed, 68 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>>> index 83c92bfd964c..a3738fab3927 100644 >>>> --- a/drivers/dma-buf/dma-buf.c >>>> +++ b/drivers/dma-buf/dma-buf.c >>>> @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info >>>> attach->dev = info->dev; >>>> attach->dmabuf = dmabuf; >>>> + attach->importer_priv = info->importer_priv; >>>> + attach->invalidate = info->invalidate; >>>> mutex_lock(&dmabuf->lock); >>>> @@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info >>>> if (ret) >>>> goto err_attach; >>>> } >>>> + reservation_object_lock(dmabuf->resv, NULL); >>>> list_add(&attach->node, &dmabuf->attachments); >>>> + reservation_object_unlock(dmabuf->resv); >>>> mutex_unlock(&dmabuf->lock); >>>> @@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) >>>> DMA_BIDIRECTIONAL); >>>> mutex_lock(&dmabuf->lock); >>>> + reservation_object_lock(dmabuf->resv, NULL); >>>> list_del(&attach->node); >>>> + reservation_object_unlock(dmabuf->resv); >>>> if (dmabuf->ops->detach) >>>> dmabuf->ops->detach(dmabuf, attach); >>>> @@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, >>>> if (attach->sgt) >>>> return attach->sgt; >>>> + /* >>>> + * Mapping a DMA-buf can trigger its invalidation, prevent sending this >>>> + * event to the caller by temporary removing this attachment from the >>>> + * list. >>>> + */ >>>> + if (attach->invalidate) >>>> + list_del(&attach->node); >>> Just noticed this: Why do we need this? invalidate needs the reservation >>> lock, as does map_attachment. It should be impssoble to have someone else >>> sneak in here. >> I was having problems with self triggered invalidations. >> >> E.g. client A tries to map an attachment, that in turn causes the buffer to >> move to a new place and client A is informed about that movement with an >> invalidation. > Uh, that sounds like a bug in ttm or somewhere else in the exporter. If > you evict the bo that you're trying to map, that's bad. > > Or maybe it's a framework bug, and we need to track whether an attachment > has a map or not. That would make more sense ... Well neither, as far as I can see this is perfectly normal behavior. We just don't want any invalidation send to a driver which is currently making a mapping. If you want I can do this in the driver as well, but at least of hand it looks like a good idea to have that in common code. Tracking the mappings could work as well, but the problem here is that I actually want the lifetime of old and new mappings to overlap for pipelining. Regards, Christian. > -Daniel
