| Message ID | 1440631758-30303-4-git-send-email-tiago.vignatti@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Thu, Aug 27, 2015 at 2:29 AM, Tiago Vignatti <tiago.vignatti@intel.com> wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > 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 <sumit.semwal@linaro.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com> > --- > 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 <linux/poll.h> > #include <linux/reservation.h> > > +#include <uapi/linux/dma-buf.h> > + > 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 0000000..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 redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published by > + * the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#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; > +}; > + > +#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 is it bad idea for drm.h to include these definition? http://lxr.free-electrons.com/source/include/uapi/drm/drm.h#L684 Br, DS > +#define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) > + > +#endif > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > --------------------------------------------------------------------- > Intel Finland Oy > Registered Address: PL 281, 00181 Helsinki > Business Identity Code: 0357606 - 4 > Domiciled in Helsinki > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies.
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
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)
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 <linux/poll.h> #include <linux/reservation.h> +#include <uapi/linux/dma-buf.h> + 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 0000000..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 redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#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; +}; + +#define DMA_BUF_BASE 'b' +#define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) + +#endif