It's desirable to use zero-copy mechanisms when using various graphics buffers. With udmabuf, two cases come to mind:
1) Directly scan-out a guest-mapped buffer 2) Import into VK with VK_EXT_external_memory_dma_buf
However, displays are not generally coherent with the CPU and many GPUs aren't either. So we need to map write-combine in the guest. One way to achieve this is to synchronize on the host.
[warning] -- only compile tested at this point to get feedback
Gurchetan Singh (6): udmabuf: use cache_sgt_mapping option udmabuf: add ability to set access flags on udmabuf udmabuf: enforce access flags udmabuf: add a pointer to the miscdevice in dma-buf private data udmabuf: separate out creating/destroying scatter-table udmabuf: implement begin_cpu_access/end_cpu_access hooks
drivers/dma-buf/udmabuf.c | 103 +++++++++++++++++++++++++++++------ include/uapi/linux/udmabuf.h | 5 +- 2 files changed, 90 insertions(+), 18 deletions(-)
The GEM prime helpers do it, so should we. It's also possible to make it optional later.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org --- drivers/dma-buf/udmabuf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 9635897458a0..b345e91d831a 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -108,12 +108,13 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, }
static const struct dma_buf_ops udmabuf_ops = { - .map_dma_buf = map_udmabuf, - .unmap_dma_buf = unmap_udmabuf, - .release = release_udmabuf, - .map = kmap_udmabuf, - .unmap = kunmap_udmabuf, - .mmap = mmap_udmabuf, + .cache_sgt_mapping = true, + .map_dma_buf = map_udmabuf, + .unmap_dma_buf = unmap_udmabuf, + .release = release_udmabuf, + .map = kmap_udmabuf, + .unmap = kunmap_udmabuf, + .mmap = mmap_udmabuf, };
#define SEALS_WANTED (F_SEAL_SHRINK)
The main use for udmabuf is sending guest memory pages to the host.
It's generally a bad idea to have to separate mappings with different attributes. For example, a WC mapping the guest kernel and cached mapping on the host is problematic.
Add creation time flags so the user has responsibility for the specific use case.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org --- drivers/dma-buf/udmabuf.c | 7 ++++++- include/uapi/linux/udmabuf.h | 5 ++++- 2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index b345e91d831a..4ecf2a94fed3 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -186,7 +186,12 @@ static long udmabuf_create(const struct udmabuf_create_list *head, exp_info.ops = &udmabuf_ops; exp_info.size = ubuf->pagecount << PAGE_SHIFT; exp_info.priv = ubuf; - exp_info.flags = O_RDWR; + + if ((head->flags & UDMABUF_FLAGS_PROT_READ) && + (head->flags & UDMABUF_FLAGS_PROT_WRITE)) + exp_info.flags = O_RDWR; + else if (head->flags & UDMABUF_FLAGS_PROT_WRITE) + exp_info.flags = O_WRONLY;
buf = dma_buf_export(&exp_info); if (IS_ERR(buf)) { diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h index 46b6532ed855..21290cb9696c 100644 --- a/include/uapi/linux/udmabuf.h +++ b/include/uapi/linux/udmabuf.h @@ -5,7 +5,10 @@ #include <linux/types.h> #include <linux/ioctl.h>
-#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_PROT_NONE 0x02 +#define UDMABUF_FLAGS_PROT_READ 0x04 +#define UDMABUF_FLAGS_PROT_WRITE 0x08
struct udmabuf_create { __u32 memfd;
On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
The main use for udmabuf is sending guest memory pages to the host.
It's generally a bad idea to have to separate mappings with different attributes. For example, a WC mapping the guest kernel and cached mapping on the host is problematic.
Add creation time flags so the user has responsibility for the specific use case.
-#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_PROT_NONE 0x02 +#define UDMABUF_FLAGS_PROT_READ 0x04 +#define UDMABUF_FLAGS_PROT_WRITE 0x08
[ didn't look at followup patches yet ]
You can't have readonly/writeonly dmabufs. So that isn't going to fly.
The commit message suggests this is for cache attributes not protection, so having the flags might make sense, but please don't name the flags PROT_* then.
cheers, Gerd
On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann kraxel@redhat.com wrote:
On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
The main use for udmabuf is sending guest memory pages to the host.
It's generally a bad idea to have to separate mappings with different attributes. For example, a WC mapping the guest kernel and cached mapping on the host is problematic.
Add creation time flags so the user has responsibility for the specific use case.
-#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_PROT_NONE 0x02 +#define UDMABUF_FLAGS_PROT_READ 0x04 +#define UDMABUF_FLAGS_PROT_WRITE 0x08
[ didn't look at followup patches yet ]
You can't have readonly/writeonly dmabufs. So that isn't going to fly.
The commit message suggests this is for cache attributes not protection, so having the flags might make sense, but please don't name the flags PROT_* then.
Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like msm_drm.h). And since the dma api doesn't work on x86 [1], we'll have to call drm_cflush_pages in the guest. Since caching is privileged on ARM and not on x86, that *should* get us write-combine guest buffers.
[1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html
cheers, Gerd
On Fri, Aug 02, 2019 at 09:45:15AM -0700, Gurchetan Singh wrote:
On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann kraxel@redhat.com wrote:
On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
The main use for udmabuf is sending guest memory pages to the host.
It's generally a bad idea to have to separate mappings with different attributes. For example, a WC mapping the guest kernel and cached mapping on the host is problematic.
Add creation time flags so the user has responsibility for the specific use case.
-#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_PROT_NONE 0x02 +#define UDMABUF_FLAGS_PROT_READ 0x04 +#define UDMABUF_FLAGS_PROT_WRITE 0x08
[ didn't look at followup patches yet ]
You can't have readonly/writeonly dmabufs. So that isn't going to fly.
The commit message suggests this is for cache attributes not protection, so having the flags might make sense, but please don't name the flags PROT_* then.
Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like msm_drm.h). And since the dma api doesn't work on x86 [1], we'll have to call drm_cflush_pages in the guest. Since caching is privileged on ARM and not on x86, that *should* get us write-combine guest buffers.
[1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html
Ah, so you are aware of the vgem cache synchronization patches.
It's probably a good idea to wait until that is finally settled before following with udmabuf.
cheers, Gerd
Enforce the access flags that were added earlier.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org --- drivers/dma-buf/udmabuf.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 4ecf2a94fed3..134e53d24c2b 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -16,6 +16,7 @@ static const u32 list_limit = 1024; /* udmabuf_create_list->count limit */ static const size_t size_limit_mb = 64; /* total dmabuf size, in megabytes */
struct udmabuf { + u32 flags; pgoff_t pagecount; struct page **pages; }; @@ -37,10 +38,17 @@ static const struct vm_operations_struct udmabuf_vm_ops = { static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) { struct udmabuf *ubuf = buf->priv; + pgprot_t pgprot = vm_get_page_prot(vma->vm_flags);
if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) return -EINVAL;
+ if (ubuf->flags & UDMABUF_FLAGS_PROT_NONE) + return -EINVAL; + + if ((ubuf->flags & UDMABUF_FLAGS_PROT_READ) == 0) + vma->vm_page_prot = pgprot_writecombine(pgprot); + vma->vm_ops = &udmabuf_vm_ops; vma->vm_private_data = ubuf; return 0; @@ -193,6 +201,7 @@ static long udmabuf_create(const struct udmabuf_create_list *head, else if (head->flags & UDMABUF_FLAGS_PROT_WRITE) exp_info.flags = O_WRONLY;
+ ubuf->flags = head->flags; buf = dma_buf_export(&exp_info); if (IS_ERR(buf)) { ret = PTR_ERR(buf);
static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) { struct udmabuf *ubuf = buf->priv;
pgprot_t pgprot = vm_get_page_prot(vma->vm_flags);
if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) return -EINVAL;
if (ubuf->flags & UDMABUF_FLAGS_PROT_NONE)
return -EINVAL;if ((ubuf->flags & UDMABUF_FLAGS_PROT_READ) == 0)
"if ((vma->vm_flags & VM_READ) == 0)" ?
cheers, Gerd
Will be used later.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org --- drivers/dma-buf/udmabuf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 134e53d24c2b..47003abbf4c2 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -19,6 +19,7 @@ struct udmabuf { u32 flags; pgoff_t pagecount; struct page **pages; + struct miscdevice *udmabuf_misc; };
static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf) @@ -128,7 +129,8 @@ static const struct dma_buf_ops udmabuf_ops = { #define SEALS_WANTED (F_SEAL_SHRINK) #define SEALS_DENIED (F_SEAL_WRITE)
-static long udmabuf_create(const struct udmabuf_create_list *head, +static long udmabuf_create(struct miscdevice *udmabuf_misc, + struct udmabuf_create_list *head, const struct udmabuf_create_item *list) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); @@ -202,6 +204,7 @@ static long udmabuf_create(const struct udmabuf_create_list *head, exp_info.flags = O_WRONLY;
ubuf->flags = head->flags; + ubuf->udmabuf_misc = udmabuf_misc; buf = dma_buf_export(&exp_info); if (IS_ERR(buf)) { ret = PTR_ERR(buf); @@ -239,7 +242,7 @@ static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) list.offset = create.offset; list.size = create.size;
- return udmabuf_create(&head, &list); + return udmabuf_create(filp->private_data, &head, &list); }
static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) @@ -258,7 +261,7 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) if (IS_ERR(list)) return PTR_ERR(list);
- ret = udmabuf_create(&head, list); + ret = udmabuf_create(filp->private_data, &head, list); kfree(list); return ret; }
On Wed, Jul 31, 2019 at 07:25:15PM -0700, Gurchetan Singh wrote:
Will be used later.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org
drivers/dma-buf/udmabuf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 134e53d24c2b..47003abbf4c2 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -19,6 +19,7 @@ struct udmabuf { u32 flags; pgoff_t pagecount; struct page **pages;
- struct miscdevice *udmabuf_misc;
I'd name this "dev" or "device".
cheers, Gerd
Reused later.
Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org --- drivers/dma-buf/udmabuf.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 47003abbf4c2..5f8bee1862de 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -55,10 +55,10 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) return 0; }
-static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, - enum dma_data_direction direction) +static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, + enum dma_data_direction direction) { - struct udmabuf *ubuf = at->dmabuf->priv; + struct udmabuf *ubuf = buf->priv; struct sg_table *sg; int ret;
@@ -70,7 +70,7 @@ static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, GFP_KERNEL); if (ret < 0) goto err; - if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction)) { + if (!dma_map_sg(dev, sg->sgl, sg->nents, direction)) { ret = -EINVAL; goto err; } @@ -82,13 +82,25 @@ static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, return ERR_PTR(ret); }
+static void put_sg_table(struct device *dev, struct sg_table *sg, + enum dma_data_direction direction) +{ + dma_unmap_sg(dev, sg->sgl, sg->nents, direction); + sg_free_table(sg); + kfree(sg); +} + +static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, + enum dma_data_direction direction) +{ + return get_sg_table(at->dev, at->dmabuf, direction); +} + static void unmap_udmabuf(struct dma_buf_attachment *at, struct sg_table *sg, enum dma_data_direction direction) { - dma_unmap_sg(at->dev, sg->sgl, sg->nents, direction); - sg_free_table(sg); - kfree(sg); + return put_sg_table(at->dev, sg, direction); }
static void release_udmabuf(struct dma_buf *buf)
With the misc device, we should end up using dma direct ops. This can allow us to have WC mappings in the guest after some synchronization, if we disallow cached mappings in the host.
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 5f8bee1862de..52de7ba1e712 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -19,6 +19,7 @@ struct udmabuf { u32 flags; pgoff_t pagecount; struct page **pages; + struct sg_table *sg; struct miscdevice *udmabuf_misc; };
@@ -106,8 +107,12 @@ 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->udmabuf_misc->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); @@ -128,6 +133,38 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, kunmap(vaddr); }
+static int begin_cpu_udmabuf(struct dma_buf *buf, + enum dma_data_direction direction) +{ + struct udmabuf *ubuf = buf->priv; + struct device *dev = ubuf->udmabuf_misc->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->udmabuf_misc->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, @@ -136,6 +173,8 @@ static const struct dma_buf_ops udmabuf_ops = { .map = kmap_udmabuf, .unmap = kunmap_udmabuf, .mmap = mmap_udmabuf, + .begin_cpu_access = begin_cpu_udmabuf, + .end_cpu_access = end_cpu_udmabuf, };
#define SEALS_WANTED (F_SEAL_SHRINK)
dri-devel@lists.freedesktop.org
-
Gerd Hoffmann -
Gurchetan Singh