[PATCH 3/6] dma-buf: Add ioctls to allow userspace to flush

[Tiago Vignatti]

On 08/27/2015 05:03 AM, Chris Wilson wrote:
> On Wed, Aug 26, 2015 at 08:29:15PM -0300, Tiago Vignatti wrote:
>> +#ifndef _DMA_BUF_UAPI_H_
>> +#define _DMA_BUF_UAPI_H_
>> +
>> +enum dma_buf_sync_flags {
>> +DMA_BUF_SYNC_READ = (1 << 0),
>> +DMA_BUF_SYNC_WRITE = (2 << 0),
>> +DMA_BUF_SYNC_RW = (3 << 0),
>> +DMA_BUF_SYNC_START = (0 << 2),
>> +DMA_BUF_SYNC_END = (1 << 2),
>> +
>> +DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
>> +DMA_BUF_SYNC_END
>> +};
>> +
>> +/* begin/end dma-buf functions used for userspace mmap. */
>> +struct dma_buf_sync {
>> +enum dma_buf_sync_flags flags;
>
> It is better to use explicitly sized types. And since this is not 64b
> padded, probably best to add that padding now.

ahh, I've changed it to use enum instead. If I rollback to use defines 
then do you think works better? Like this:

struct dma_buf_sync {
   __u64 flags;
};

#define DMA_BUF_SYNC_READ  (1 << 0)
#define DMA_BUF_SYNC_WRITE (2 << 0)
#define DMA_BUF_SYNC_RW(3 << 0)
#define DMA_BUF_SYNC_START (0 << 2)
#define DMA_BUF_SYNC_END   (1 << 2)
#define DMA_BUF_SYNC_VALID_FLAGS_MASK \
   (DMA_BUF_SYNC_RW | DMA_BUF_SYNC_END)

#define DMA_BUF_BASE'b'
#define DMA_BUF_IOCTL_SYNC  _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)

[PATCH 3/6] dma-buf: Add ioctls to allow userspace to flush

[Tiago Vignatti]

On 08/27/2015 09:06 AM, Hwang, Dongseong wrote:
> On Thu, Aug 27, 2015 at 2:29 AM, Tiago Vignatti
>> +#define DMA_BUF_BASE   'b'
>
> 'b' is occupied by vme and qat driver;
> https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-number.txt#L201

I believe this is alright, as noted in that txt file: "Because of the 
large number of drivers, many drivers share a partial letter with other 
drivers".

> is it bad idea for drm.h to include these definition?
> http://lxr.free-electrons.com/source/include/uapi/drm/drm.h#L684

this is not a drm code and other type of device drivers might want to 
use it as well.

Tiago

[PATCH 3/6] dma-buf: Add ioctls to allow userspace to flush

[Hwang, Dongseong]

On Thu, Aug 27, 2015 at 2:29 AM, Tiago Vignatti
 wrote:
> From: Daniel Vetter 
>
> The userspace might need some sort of cache coherency management e.g. when CPU
> and GPU domains are being accessed through dma-buf at the same time. To
> circumvent this problem there are begin/end coherency markers, that forward
> directly to existing dma-buf device drivers vfunc hooks. Userspace can make 
> use
> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be
> used like following:
>  - mmap dma-buf fd
>  - for each drawing/upload cycle in CPU 1. SYNC_START ioctl, 2. read/write
>to mmap area 3. SYNC_END ioctl. This can be repeated as often as you
>want (with the new data being consumed by the GPU or say scanout 
> device)
>  - munamp once you don't need the buffer any more
>
> v2 (Tiago): Fix header file type names (u64 -> __u64)
> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end
> dma-buf functions. Check for overflows in start/length.
> v4 (Tiago): use 2d regions for sync.
> v5 (Tiago): forget about 2d regions (v4); use _IOW in DMA_BUF_IOCTL_SYNC and
> remove range information from struct dma_buf_sync.
>
> Cc: Sumit Semwal 
> Signed-off-by: Daniel Vetter 
> Signed-off-by: Tiago Vignatti 
> ---
>  Documentation/dma-buf-sharing.txt | 12 +++
>  drivers/dma-buf/dma-buf.c | 43 
> +++
>  include/uapi/linux/dma-buf.h  | 41 +
>  3 files changed, 96 insertions(+)
>  create mode 100644 include/uapi/linux/dma-buf.h
>
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 4f4a84b..ec0ab1d 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,6 +352,18 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd.
>
> +   Also, the userspace might need some sort of cache coherency management 
> e.g.
> +   when CPU and GPU domains are being accessed through dma-buf at the same
> +   time. To circumvent this problem there are begin/end coherency markers, 
> that
> +   forward directly to existing dma-buf device drivers vfunc hooks. Userspace
> +   can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The
> +   sequence would be used like following:
> + - mmap dma-buf fd
> + - for each drawing/upload cycle in CPU 1. SYNC_START ioctl, 2. 
> read/write
> +   to mmap area 3. SYNC_END ioctl. This can be repeated as often as you
> +   want (with the new data being consumed by the GPU or say scanout 
> device)
> + - munamp once you don't need the buffer any more
> +
>  2. Supporting existing mmap interfaces in importers
>
> Similar to the motivation for kernel cpu access it is again important that
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index b2ac13b..9a298bd 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -34,6 +34,8 @@
>  #include 
>  #include 
>
> +#include 
> +
>  static inline int is_dma_buf_file(struct file *);
>
>  struct dma_buf_list {
> @@ -251,11 +253,52 @@ out:
> return events;
>  }
>
> +static long dma_buf_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> +   struct dma_buf *dmabuf;
> +   struct dma_buf_sync sync;
> +   enum dma_data_direction direction;
> +
> +   dmabuf = file->private_data;
> +
> +   if (!is_dma_buf_file(file))
> +   return -EINVAL;
> +
> +   switch (cmd) {
> +   case DMA_BUF_IOCTL_SYNC:
> +   if (copy_from_user(&sync, (void __user *) arg, sizeof(sync)))
> +   return -EFAULT;
> +
> +   if (sync.flags & DMA_BUF_SYNC_RW)
> +   direction = DMA_BIDIRECTIONAL;
> +   else if (sync.flags & DMA_BUF_SYNC_READ)
> +   direction = DMA_FROM_DEVICE;
> +   else if (sync.flags & DMA_BUF_SYNC_WRITE)
> +   direction = DMA_TO_DEVICE;
> +   else
> +   return -EINVAL;
> +
> +   if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
> +   return -EINVAL;
> +
> +   if (sync.flags & DMA_BUF_SYNC_END)
> +   dma_buf_end_cpu_access(dmabuf, direction);
> +   else
> +   dma_buf_begin_cpu_access(dmabuf, direction);
> +
> +   return 0;
> +   default:
> +   return -ENOTTY;
> +   }
> +}
> +
>  static const struct file_operations dma_buf_fops = {
> .release= dma_buf_release,
> .mmap   = dma_buf_mmap_internal,
> .llseek = dma_buf_llseek,
> .poll   = dma_buf_poll,
> +   .unlocked_ioctl = dma_buf_ioctl,
>  };
>
>  /*
> diff --git a/include/uapi/linux/dma-buf.h

[PATCH 3/6] dma-buf: Add ioctls to allow userspace to flush

[Chris Wilson]

On Wed, Aug 26, 2015 at 08:29:15PM -0300, Tiago Vignatti wrote:
> +#ifndef _DMA_BUF_UAPI_H_
> +#define _DMA_BUF_UAPI_H_
> +
> +enum dma_buf_sync_flags {
> + DMA_BUF_SYNC_READ = (1 << 0),
> + DMA_BUF_SYNC_WRITE = (2 << 0),
> + DMA_BUF_SYNC_RW = (3 << 0),
> + DMA_BUF_SYNC_START = (0 << 2),
> + DMA_BUF_SYNC_END = (1 << 2),
> +
> + DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
> + DMA_BUF_SYNC_END
> +};
> +
> +/* begin/end dma-buf functions used for userspace mmap. */
> +struct dma_buf_sync {
> + enum dma_buf_sync_flags flags;

It is better to use explicitly sized types. And since this is not 64b
padded, probably best to add that padding now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/6] dma-buf: Add ioctls to allow userspace to flush

[Tiago Vignatti]

From: Daniel Vetter 

The userspace might need some sort of cache coherency management e.g. when CPU
and GPU domains are being accessed through dma-buf at the same time. To
circumvent this problem there are begin/end coherency markers, that forward
directly to existing dma-buf device drivers vfunc hooks. Userspace can make use
of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be
used like following:
 - mmap dma-buf fd
 - for each drawing/upload cycle in CPU 1. SYNC_START ioctl, 2. read/write
   to mmap area 3. SYNC_END ioctl. This can be repeated as often as you
   want (with the new data being consumed by the GPU or say scanout device)
 - munamp once you don't need the buffer any more

v2 (Tiago): Fix header file type names (u64 -> __u64)
v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end
dma-buf functions. Check for overflows in start/length.
v4 (Tiago): use 2d regions for sync.
v5 (Tiago): forget about 2d regions (v4); use _IOW in DMA_BUF_IOCTL_SYNC and
remove range information from struct dma_buf_sync.

Cc: Sumit Semwal 
Signed-off-by: Daniel Vetter 
Signed-off-by: Tiago Vignatti 
---
 Documentation/dma-buf-sharing.txt | 12 +++
 drivers/dma-buf/dma-buf.c | 43 +++
 include/uapi/linux/dma-buf.h  | 41 +
 3 files changed, 96 insertions(+)
 create mode 100644 include/uapi/linux/dma-buf.h

diff --git a/Documentation/dma-buf-sharing.txt 
b/Documentation/dma-buf-sharing.txt
index 4f4a84b..ec0ab1d 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -352,6 +352,18 @@ Being able to mmap an export dma-buf buffer object has 2 
main use-cases:

No special interfaces, userspace simply calls mmap on the dma-buf fd.

+   Also, the userspace might need some sort of cache coherency management e.g.
+   when CPU and GPU domains are being accessed through dma-buf at the same
+   time. To circumvent this problem there are begin/end coherency markers, that
+   forward directly to existing dma-buf device drivers vfunc hooks. Userspace
+   can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The
+   sequence would be used like following:
+ - mmap dma-buf fd
+ - for each drawing/upload cycle in CPU 1. SYNC_START ioctl, 2. read/write
+   to mmap area 3. SYNC_END ioctl. This can be repeated as often as you
+   want (with the new data being consumed by the GPU or say scanout device)
+ - munamp once you don't need the buffer any more
+
 2. Supporting existing mmap interfaces in importers

Similar to the motivation for kernel cpu access it is again important that
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index b2ac13b..9a298bd 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -34,6 +34,8 @@
 #include 
 #include 

+#include 
+
 static inline int is_dma_buf_file(struct file *);

 struct dma_buf_list {
@@ -251,11 +253,52 @@ out:
return events;
 }

+static long dma_buf_ioctl(struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+   struct dma_buf *dmabuf;
+   struct dma_buf_sync sync;
+   enum dma_data_direction direction;
+
+   dmabuf = file->private_data;
+
+   if (!is_dma_buf_file(file))
+   return -EINVAL;
+
+   switch (cmd) {
+   case DMA_BUF_IOCTL_SYNC:
+   if (copy_from_user(&sync, (void __user *) arg, sizeof(sync)))
+   return -EFAULT;
+
+   if (sync.flags & DMA_BUF_SYNC_RW)
+   direction = DMA_BIDIRECTIONAL;
+   else if (sync.flags & DMA_BUF_SYNC_READ)
+   direction = DMA_FROM_DEVICE;
+   else if (sync.flags & DMA_BUF_SYNC_WRITE)
+   direction = DMA_TO_DEVICE;
+   else
+   return -EINVAL;
+
+   if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
+   return -EINVAL;
+
+   if (sync.flags & DMA_BUF_SYNC_END)
+   dma_buf_end_cpu_access(dmabuf, direction);
+   else
+   dma_buf_begin_cpu_access(dmabuf, direction);
+
+   return 0;
+   default:
+   return -ENOTTY;
+   }
+}
+
 static const struct file_operations dma_buf_fops = {
.release= dma_buf_release,
.mmap   = dma_buf_mmap_internal,
.llseek = dma_buf_llseek,
.poll   = dma_buf_poll,
+   .unlocked_ioctl = dma_buf_ioctl,
 };

 /*
diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
new file mode 100644
index 000..262f7a7
--- /dev/null
+++ b/include/uapi/linux/dma-buf.h
@@ -0,0 +1,41 @@
+/*
+ * Framework for buffer objects that can be shared across devices/subsystems.
+ *
+ * Copyright(C) 2015 Intel Ltd
+ *
+ * This program is free software; you can redi