| Message ID | 20191203013627.85991-4-gurchetansingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] udmabuf: use cache_sgt_mapping option | expand |
On Mon, Dec 2, 2019 at 5:36 PM Gurchetan Singh <gurchetansingh@chromium.org> wrote: > > With the misc device, we should end up using the result of > get_arch_dma_ops(..) or dma-direct ops. > > This can allow us to have WC mappings in the guest after > synchronization. > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> > --- > drivers/dma-buf/udmabuf.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 0a610e09ae23..61b0a2cff874 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -18,6 +18,7 @@ static const size_t size_limit_mb = 64; /* total dmabuf size, in megabytes */ > struct udmabuf { > pgoff_t pagecount; > struct page **pages; > + struct sg_table *sg; > struct miscdevice *device; > }; > > @@ -98,20 +99,58 @@ static void unmap_udmabuf(struct dma_buf_attachment *at, > static void release_udmabuf(struct dma_buf *buf) > { > struct udmabuf *ubuf = buf->priv; > + struct device *dev = ubuf->device->this_device; > pgoff_t pg; > > + if (ubuf->sg) > + put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); > + > for (pg = 0; pg < ubuf->pagecount; pg++) > put_page(ubuf->pages[pg]); > kfree(ubuf->pages); > kfree(ubuf); > } > > +static int begin_cpu_udmabuf(struct dma_buf *buf, > + enum dma_data_direction direction) > +{ > + struct udmabuf *ubuf = buf->priv; > + struct device *dev = ubuf->device->this_device; > + > + if (!ubuf->sg) { > + ubuf->sg = get_sg_table(dev, buf, direction); > + if (IS_ERR(ubuf->sg)) > + return PTR_ERR(ubuf->sg); > + } else { > + dma_sync_sg_for_device(dev, ubuf->sg->sgl, > + ubuf->sg->nents, > + direction); I know this solves the issue (flush the CPU cache before WC access), but it looks like an abuse? It is counter-intuitive that the buffer is synced for device when one wants CPU access. > + } > + > + return 0; > +} > + > +static int end_cpu_udmabuf(struct dma_buf *buf, > + enum dma_data_direction direction) > +{ > + struct udmabuf *ubuf = buf->priv; > + struct device *dev = ubuf->device->this_device; > + > + if (!ubuf->sg) > + return -EINVAL; > + > + dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction); > + return 0; > +} > + > static const struct dma_buf_ops udmabuf_ops = { > .cache_sgt_mapping = true, > .map_dma_buf = map_udmabuf, > .unmap_dma_buf = unmap_udmabuf, > .release = release_udmabuf, > .mmap = mmap_udmabuf, > + .begin_cpu_access = begin_cpu_udmabuf, > + .end_cpu_access = end_cpu_udmabuf, > }; > > #define SEALS_WANTED (F_SEAL_SHRINK) > -- > 2.24.0.393.g34dc348eaf-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On Mon, Dec 9, 2019 at 2:44 PM Chia-I Wu <olvaffe@gmail.com> wrote: > > On Mon, Dec 2, 2019 at 5:36 PM Gurchetan Singh > <gurchetansingh@chromium.org> wrote: > > > > With the misc device, we should end up using the result of > > get_arch_dma_ops(..) or dma-direct ops. > > > > This can allow us to have WC mappings in the guest after > > synchronization. > > > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> > > --- > > drivers/dma-buf/udmabuf.c | 39 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > > index 0a610e09ae23..61b0a2cff874 100644 > > --- a/drivers/dma-buf/udmabuf.c > > +++ b/drivers/dma-buf/udmabuf.c > > @@ -18,6 +18,7 @@ static const size_t size_limit_mb = 64; /* total dmabuf size, in megabytes */ > > struct udmabuf { > > pgoff_t pagecount; > > struct page **pages; > > + struct sg_table *sg; > > struct miscdevice *device; > > }; > > > > @@ -98,20 +99,58 @@ static void unmap_udmabuf(struct dma_buf_attachment *at, > > static void release_udmabuf(struct dma_buf *buf) > > { > > struct udmabuf *ubuf = buf->priv; > > + struct device *dev = ubuf->device->this_device; > > pgoff_t pg; > > > > + if (ubuf->sg) > > + put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); > > + > > for (pg = 0; pg < ubuf->pagecount; pg++) > > put_page(ubuf->pages[pg]); > > kfree(ubuf->pages); > > kfree(ubuf); > > } > > > > +static int begin_cpu_udmabuf(struct dma_buf *buf, > > + enum dma_data_direction direction) > > +{ > > + struct udmabuf *ubuf = buf->priv; > > + struct device *dev = ubuf->device->this_device; > > + > > + if (!ubuf->sg) { > > + ubuf->sg = get_sg_table(dev, buf, direction); > > + if (IS_ERR(ubuf->sg)) > > + return PTR_ERR(ubuf->sg); > > + } else { > > + dma_sync_sg_for_device(dev, ubuf->sg->sgl, > > + ubuf->sg->nents, > > + direction); > I know this solves the issue (flush the CPU cache before WC access), > but it looks like an abuse? It is counter-intuitive that the buffer > is synced for device when one wants CPU access. I am skeptical about this change. (1) Semantically, a dma-buf is in DMA domain. CPU access from the importer must be surrounded by {begin,end}_cpu_access. This gives the exporter a chance to move the buffer to the CPU domain temporarily. (2) When the exporter itself has other means to do CPU access, it is only reasonable for the exporter to move the buffer to the CPU domain before access, and to the DMA domain after access. The exporter can potentially reuse {begin,end}_cpu_access for that purpose. Because of (1), udmabuf does need to implement the {begin,end}_cpu_access hooks. But "begin" should mean dma_sync_sg_for_cpu and "end" should mean dma_sync_sg_for_device. Because of (2), if userspace wants to continuing accessing through the memfd mapping, it should call udmabuf's {begin,end}_cpu_access to avoid cache issues. > > > + } > > + > > + return 0; > > +} > > + > > +static int end_cpu_udmabuf(struct dma_buf *buf, > > + enum dma_data_direction direction) > > +{ > > + struct udmabuf *ubuf = buf->priv; > > + struct device *dev = ubuf->device->this_device; > > + > > + if (!ubuf->sg) > > + return -EINVAL; > > + > > + dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction); > > + return 0; > > +} > > + > > static const struct dma_buf_ops udmabuf_ops = { > > .cache_sgt_mapping = true, > > .map_dma_buf = map_udmabuf, > > .unmap_dma_buf = unmap_udmabuf, > > .release = release_udmabuf, > > .mmap = mmap_udmabuf, > > + .begin_cpu_access = begin_cpu_udmabuf, > > + .end_cpu_access = end_cpu_udmabuf, > > }; > > > > #define SEALS_WANTED (F_SEAL_SHRINK) > > -- > > 2.24.0.393.g34dc348eaf-goog > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 0a610e09ae23..61b0a2cff874 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -18,6 +18,7 @@ static const size_t size_limit_mb = 64; /* total dmabuf size, in megabytes */ struct udmabuf { pgoff_t pagecount; struct page **pages; + struct sg_table *sg; struct miscdevice *device; }; @@ -98,20 +99,58 @@ static void unmap_udmabuf(struct dma_buf_attachment *at, static void release_udmabuf(struct dma_buf *buf) { struct udmabuf *ubuf = buf->priv; + struct device *dev = ubuf->device->this_device; pgoff_t pg; + if (ubuf->sg) + put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); + for (pg = 0; pg < ubuf->pagecount; pg++) put_page(ubuf->pages[pg]); kfree(ubuf->pages); kfree(ubuf); } +static int begin_cpu_udmabuf(struct dma_buf *buf, + enum dma_data_direction direction) +{ + struct udmabuf *ubuf = buf->priv; + struct device *dev = ubuf->device->this_device; + + if (!ubuf->sg) { + ubuf->sg = get_sg_table(dev, buf, direction); + if (IS_ERR(ubuf->sg)) + return PTR_ERR(ubuf->sg); + } else { + dma_sync_sg_for_device(dev, ubuf->sg->sgl, + ubuf->sg->nents, + direction); + } + + return 0; +} + +static int end_cpu_udmabuf(struct dma_buf *buf, + enum dma_data_direction direction) +{ + struct udmabuf *ubuf = buf->priv; + struct device *dev = ubuf->device->this_device; + + if (!ubuf->sg) + return -EINVAL; + + dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction); + return 0; +} + static const struct dma_buf_ops udmabuf_ops = { .cache_sgt_mapping = true, .map_dma_buf = map_udmabuf, .unmap_dma_buf = unmap_udmabuf, .release = release_udmabuf, .mmap = mmap_udmabuf, + .begin_cpu_access = begin_cpu_udmabuf, + .end_cpu_access = end_cpu_udmabuf, }; #define SEALS_WANTED (F_SEAL_SHRINK)
With the misc device, we should end up using the result of get_arch_dma_ops(..) or dma-direct ops. This can allow us to have WC mappings in the guest after synchronization. Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> --- drivers/dma-buf/udmabuf.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)