From: "Koenig, Christian" <Christian.Koenig@amd.com> To: "sumit.semwal@linaro.org" <sumit.semwal@linaro.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linaro-mm-sig@lists.linaro.org" <linaro-mm-sig@lists.linaro.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 04/12] dma-buf: add optional invalidate_mappings callback v5 Date: Thu, 18 Apr 2019 08:28:51 +0000 [thread overview] Message-ID: <0611f62c-2b81-b85f-a8d9-69c3daf0c635@amd.com> (raw) In-Reply-To: <20190418080826.GN13337@phenom.ffwll.local> 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@amd.com> >>>> --- >>>> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org> To: "sumit.semwal-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org" <sumit.semwal-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, "linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" <dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>, "linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw@public.gmane.org" <linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> Subject: Re: [PATCH 04/12] dma-buf: add optional invalidate_mappings callback v5 Date: Thu, 18 Apr 2019 08:28:51 +0000 [thread overview] Message-ID: <0611f62c-2b81-b85f-a8d9-69c3daf0c635@amd.com> (raw) In-Reply-To: <20190418080826.GN13337-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org> 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@amd.com> >>>> --- >>>> 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 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2019-04-18 8:28 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-16 18:38 dynamic DMA-buf sharing between devices Christian König 2019-04-16 18:38 ` Christian König 2019-04-16 18:38 ` [PATCH 01/12] dma-buf: add dynamic caching of sg_table Christian König 2019-04-16 18:38 ` [PATCH 02/12] dma-buf: add dma_buf_(un)map_attachment_locked variants v3 Christian König 2019-05-27 10:56 ` Christian König 2019-04-16 18:38 ` [PATCH 03/12] dma-buf: lock the reservation object during (un)map_dma_buf v3 Christian König 2019-04-16 18:38 ` Christian König 2019-04-17 14:08 ` Daniel Vetter 2019-04-17 14:08 ` Daniel Vetter 2019-04-17 14:14 ` Christian König 2019-04-17 14:26 ` Daniel Vetter 2019-04-17 14:26 ` Daniel Vetter 2019-04-17 17:10 ` Christian König 2019-04-17 17:10 ` Christian König 2019-04-16 18:38 ` [PATCH 04/12] dma-buf: add optional invalidate_mappings callback v5 Christian König 2019-04-16 18:38 ` Christian König 2019-04-17 14:01 ` Daniel Vetter 2019-04-17 14:01 ` Daniel Vetter 2019-04-17 14:33 ` Daniel Vetter 2019-04-17 14:33 ` Daniel Vetter 2019-04-17 19:07 ` Daniel Vetter 2019-04-17 19:07 ` Daniel Vetter 2019-04-17 19:13 ` Christian König 2019-04-17 19:13 ` Christian König 2019-04-18 8:08 ` Daniel Vetter 2019-04-18 8:28 ` Koenig, Christian [this message] 2019-04-18 8:28 ` Koenig, Christian 2019-04-18 8:40 ` Daniel Vetter 2019-04-18 8:40 ` Daniel Vetter 2019-04-16 18:38 ` [PATCH 05/12] dma-buf: add explicit buffer pinning Christian König 2019-04-16 18:38 ` Christian König 2019-04-17 14:20 ` Daniel Vetter 2019-04-17 14:20 ` Daniel Vetter 2019-04-17 14:30 ` Daniel Vetter 2019-04-17 14:30 ` Daniel Vetter 2019-04-17 14:40 ` Daniel Vetter 2019-04-17 19:05 ` Daniel Vetter 2019-04-19 19:05 ` Alex Deucher 2019-04-19 19:05 ` Alex Deucher 2019-04-16 18:38 ` [PATCH 06/12] drm: remove prime sg_table caching Christian König 2019-04-16 18:38 ` Christian König 2019-04-16 18:38 ` [PATCH 07/12] drm/ttm: remove the backing store if no placement is given Christian König 2019-04-16 18:38 ` Christian König 2019-04-16 18:38 ` [PATCH 08/12] drm/ttm: use the parent resv for ghost objects Christian König 2019-04-16 18:38 ` Christian König 2019-04-16 18:38 ` [PATCH 09/12] drm/amdgpu: add independent DMA-buf export v3 Christian König 2019-04-16 18:38 ` [PATCH 10/12] drm/amdgpu: add independent DMA-buf import v4 Christian König 2019-04-16 18:38 ` [PATCH 11/12] drm/amdgpu: add DMA-buf pin/unpin implementation Christian König 2019-04-16 18:38 ` Christian König 2019-04-16 18:38 ` [PATCH 12/12] drm/amdgpu: add DMA-buf invalidation callback v2 Christian König 2019-04-16 18:38 ` Christian König 2019-04-17 13:52 ` dynamic DMA-buf sharing between devices Chunming Zhou 2019-04-17 13:52 ` Chunming Zhou 2019-04-17 13:59 ` Christian König 2019-04-17 14:14 ` Chunming Zhou 2019-04-18 9:13 ` Daniel Vetter 2019-04-18 10:57 ` Christian König 2019-04-18 10:57 ` Christian König 2019-04-27 0:01 ` [PATCH 01/12] dma-buf: add dynamic caching of sg_table Liam Mark 2019-05-22 16:17 ` Sumit Semwal 2019-05-22 17:27 ` Christian König 2019-05-22 17:27 ` Christian König 2019-05-22 18:30 ` Daniel Vetter 2019-05-22 18:30 ` Daniel Vetter 2019-05-23 11:21 ` Koenig, Christian 2019-05-23 11:21 ` Koenig, Christian 2019-05-23 11:30 ` Daniel Vetter 2019-05-23 11:30 ` Daniel Vetter 2019-05-23 11:32 ` Daniel Vetter 2019-05-23 11:32 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0611f62c-2b81-b85f-a8d9-69c3daf0c635@amd.com \ --to=christian.koenig@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=sumit.semwal@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.