| Message ID | 1450820214-12509-4-git-send-email-tiago.vignatti@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Tue, Dec 22, 2015 at 10:36 PM, 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) > - munmap 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. > v6 (Tiago): use __u64 structured padded flags instead enum. Adjust > documentation about the recommendation on using sync ioctls. > v7 (Tiago): Alex' nit on flags definition and being even more wording in the > doc about sync usage. > > 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 | 21 ++++++++++++++++++- > drivers/dma-buf/dma-buf.c | 43 +++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/dma-buf.h | 38 ++++++++++++++++++++++++++++++++++ > 3 files changed, 101 insertions(+), 1 deletion(-) > 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..32ac32e 100644 > --- a/Documentation/dma-buf-sharing.txt > +++ b/Documentation/dma-buf-sharing.txt > @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases: > handles, too). So it's beneficial to support this in a similar fashion on > dma-buf to have a good transition path for existing Android userspace. > > - No special interfaces, userspace simply calls mmap on the dma-buf fd. > + No special interfaces, userspace simply calls mmap on the dma-buf fd, making > + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always* > + used when the access happens. This is discussed next paragraphs. > + > + Some systems 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) > + - munmap once you don't need the buffer any more > + > + Therefore, for correctness and optimal performance, systems with the memory > + cache shared by the GPU and CPU i.e. the "coherent" and also the > + "incoherent" are always required to use SYNC_START and SYNC_END before and > + after, respectively, when accessing the mapped address. > > 2. Supporting existing mmap interfaces in importers > > 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; Why? This can never happen, and you better not use dma_buf_ioctl() outside of dma_buf_fops.. I guess it's simply copied from the other fop callbacks, but I don't see why. dma_buf_poll() doesn't do it, neither should this, or one of the other 3 callbacks. > + > + 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; This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or EINVAL. I recommend changing it to: switch (sync.flags & DMA_BUF_SYNC_RW) { case DMA_BUF_SYNC_READ: direction = DMA_FROM_DEVICE; break; case DMA_BUF_SYNC_WRITE: direction = DMA_TO_DEVICE; break; case DMA_BUF_SYNC_READ: direction = DMA_BIDIRECTIONAL; break; default: return -EINVAL; } > + > + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) > + return -EINVAL; Why isn't this done immediately after copy_from_user()? > + > + if (sync.flags & DMA_BUF_SYNC_END) > + dma_buf_end_cpu_access(dmabuf, direction); > + else > + dma_buf_begin_cpu_access(dmabuf, direction); Why are SYNC_START and SYNC_END exclusive? It feels very natural to me to invoke both at the same time (especially if two objects are stored in the same dma-buf). > + > + 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..4a9b36b > --- /dev/null > +++ b/include/uapi/linux/dma-buf.h > @@ -0,0 +1,38 @@ > +/* > + * 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_ > + > +/* begin/end dma-buf functions used for userspace mmap. */ > +struct dma_buf_sync { > + __u64 flags; > +}; Please add '#include <linux/types.h>', otherwise this header cannot be compiled on its own (missing __u64). > + > +#define DMA_BUF_SYNC_READ (1 << 0) > +#define DMA_BUF_SYNC_WRITE (2 << 0) > +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) > +#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) Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? Thanks David > + > +#endif
On Tue, Feb 09, 2016 at 10:26:25AM +0100, David Herrmann wrote: > Hi > > On Tue, Dec 22, 2015 at 10:36 PM, 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) > > - munmap 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. > > v6 (Tiago): use __u64 structured padded flags instead enum. Adjust > > documentation about the recommendation on using sync ioctls. > > v7 (Tiago): Alex' nit on flags definition and being even more wording in the > > doc about sync usage. > > > > 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 | 21 ++++++++++++++++++- > > drivers/dma-buf/dma-buf.c | 43 +++++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/dma-buf.h | 38 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 101 insertions(+), 1 deletion(-) > > 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..32ac32e 100644 > > --- a/Documentation/dma-buf-sharing.txt > > +++ b/Documentation/dma-buf-sharing.txt > > @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases: > > handles, too). So it's beneficial to support this in a similar fashion on > > dma-buf to have a good transition path for existing Android userspace. > > > > - No special interfaces, userspace simply calls mmap on the dma-buf fd. > > + No special interfaces, userspace simply calls mmap on the dma-buf fd, making > > + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always* > > + used when the access happens. This is discussed next paragraphs. > > + > > + Some systems 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) > > + - munmap once you don't need the buffer any more > > + > > + Therefore, for correctness and optimal performance, systems with the memory > > + cache shared by the GPU and CPU i.e. the "coherent" and also the > > + "incoherent" are always required to use SYNC_START and SYNC_END before and > > + after, respectively, when accessing the mapped address. > > > > 2. Supporting existing mmap interfaces in importers > > > > 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; > > Why? This can never happen, and you better not use dma_buf_ioctl() > outside of dma_buf_fops.. > I guess it's simply copied from the other fop callbacks, but I don't > see why. dma_buf_poll() doesn't do it, neither should this, or one of > the other 3 callbacks. > > > + > > + 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; > > This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or > EINVAL. I recommend changing it to: > > switch (sync.flags & DMA_BUF_SYNC_RW) { > case DMA_BUF_SYNC_READ: > direction = DMA_FROM_DEVICE; > break; > case DMA_BUF_SYNC_WRITE: > direction = DMA_TO_DEVICE; > break; > case DMA_BUF_SYNC_READ: > direction = DMA_BIDIRECTIONAL; > break; > default: > return -EINVAL; > } > > > + > > + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) > > + return -EINVAL; > > Why isn't this done immediately after copy_from_user()? > > > + > > + if (sync.flags & DMA_BUF_SYNC_END) > > + dma_buf_end_cpu_access(dmabuf, direction); > > + else > > + dma_buf_begin_cpu_access(dmabuf, direction); > > Why are SYNC_START and SYNC_END exclusive? It feels very natural to me > to invoke both at the same time (especially if two objects are stored > in the same dma-buf). > > > + > > + 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..4a9b36b > > --- /dev/null > > +++ b/include/uapi/linux/dma-buf.h > > @@ -0,0 +1,38 @@ > > +/* > > + * 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_ > > + > > +/* begin/end dma-buf functions used for userspace mmap. */ > > +struct dma_buf_sync { > > + __u64 flags; > > +}; > > Please add '#include <linux/types.h>', otherwise this header cannot be > compiled on its own (missing __u64). > > > + > > +#define DMA_BUF_SYNC_READ (1 << 0) > > +#define DMA_BUF_SYNC_WRITE (2 << 0) > > +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) > > +#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) > > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? Oops, looks like more work is needed. Thanks for your review. I dropped patches 3-5 again meanwhile from drm-misc. -Daniel
On Tue, Feb 09, 2016 at 11:20:20AM +0100, Daniel Vetter wrote: > On Tue, Feb 09, 2016 at 10:26:25AM +0100, David Herrmann wrote: > > Hi > > > > On Tue, Dec 22, 2015 at 10:36 PM, 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) > > > - munmap 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. > > > v6 (Tiago): use __u64 structured padded flags instead enum. Adjust > > > documentation about the recommendation on using sync ioctls. > > > v7 (Tiago): Alex' nit on flags definition and being even more wording in the > > > doc about sync usage. > > > > > > 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 | 21 ++++++++++++++++++- > > > drivers/dma-buf/dma-buf.c | 43 +++++++++++++++++++++++++++++++++++++++ > > > include/uapi/linux/dma-buf.h | 38 ++++++++++++++++++++++++++++++++++ > > > 3 files changed, 101 insertions(+), 1 deletion(-) > > > 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..32ac32e 100644 > > > --- a/Documentation/dma-buf-sharing.txt > > > +++ b/Documentation/dma-buf-sharing.txt > > > @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases: > > > handles, too). So it's beneficial to support this in a similar fashion on > > > dma-buf to have a good transition path for existing Android userspace. > > > > > > - No special interfaces, userspace simply calls mmap on the dma-buf fd. > > > + No special interfaces, userspace simply calls mmap on the dma-buf fd, making > > > + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always* > > > + used when the access happens. This is discussed next paragraphs. > > > + > > > + Some systems 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) > > > + - munmap once you don't need the buffer any more > > > + > > > + Therefore, for correctness and optimal performance, systems with the memory > > > + cache shared by the GPU and CPU i.e. the "coherent" and also the > > > + "incoherent" are always required to use SYNC_START and SYNC_END before and > > > + after, respectively, when accessing the mapped address. > > > > > > 2. Supporting existing mmap interfaces in importers > > > > > > 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; > > > > Why? This can never happen, and you better not use dma_buf_ioctl() > > outside of dma_buf_fops.. > > I guess it's simply copied from the other fop callbacks, but I don't > > see why. dma_buf_poll() doesn't do it, neither should this, or one of > > the other 3 callbacks. > > > > > + > > > + 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; > > > > This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or > > EINVAL. I recommend changing it to: > > > > switch (sync.flags & DMA_BUF_SYNC_RW) { > > case DMA_BUF_SYNC_READ: > > direction = DMA_FROM_DEVICE; > > break; > > case DMA_BUF_SYNC_WRITE: > > direction = DMA_TO_DEVICE; > > break; > > case DMA_BUF_SYNC_READ: > > direction = DMA_BIDIRECTIONAL; > > break; > > default: > > return -EINVAL; > > } > > > > > + > > > + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) > > > + return -EINVAL; > > > > Why isn't this done immediately after copy_from_user()? > > > > > + > > > + if (sync.flags & DMA_BUF_SYNC_END) > > > + dma_buf_end_cpu_access(dmabuf, direction); > > > + else > > > + dma_buf_begin_cpu_access(dmabuf, direction); > > > > Why are SYNC_START and SYNC_END exclusive? It feels very natural to me > > to invoke both at the same time (especially if two objects are stored > > in the same dma-buf). > > > > > + > > > + 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..4a9b36b > > > --- /dev/null > > > +++ b/include/uapi/linux/dma-buf.h > > > @@ -0,0 +1,38 @@ > > > +/* > > > + * 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_ > > > + > > > +/* begin/end dma-buf functions used for userspace mmap. */ > > > +struct dma_buf_sync { > > > + __u64 flags; > > > +}; > > > > Please add '#include <linux/types.h>', otherwise this header cannot be > > compiled on its own (missing __u64). > > > > > + > > > +#define DMA_BUF_SYNC_READ (1 << 0) > > > +#define DMA_BUF_SYNC_WRITE (2 << 0) > > > +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) > > > +#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) > > > > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? > > Oops, looks like more work is needed. Thanks for your review. > > I dropped patches 3-5 again meanwhile from drm-misc. Correction: Only dropped this patch, since the internal interfaces stayed the same. -Daniel
Thanks for reviewing, David. Please take a look in my comments in-line. On 02/09/2016 07:26 AM, David Herrmann wrote: > > On Tue, Dec 22, 2015 at 10:36 PM, 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) >> - munmap 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. >> v6 (Tiago): use __u64 structured padded flags instead enum. Adjust >> documentation about the recommendation on using sync ioctls. >> v7 (Tiago): Alex' nit on flags definition and being even more wording in the >> doc about sync usage. >> >> 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 | 21 ++++++++++++++++++- >> drivers/dma-buf/dma-buf.c | 43 +++++++++++++++++++++++++++++++++++++++ >> include/uapi/linux/dma-buf.h | 38 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 101 insertions(+), 1 deletion(-) >> 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..32ac32e 100644 >> --- a/Documentation/dma-buf-sharing.txt >> +++ b/Documentation/dma-buf-sharing.txt >> @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases: >> handles, too). So it's beneficial to support this in a similar fashion on >> dma-buf to have a good transition path for existing Android userspace. >> >> - No special interfaces, userspace simply calls mmap on the dma-buf fd. >> + No special interfaces, userspace simply calls mmap on the dma-buf fd, making >> + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always* >> + used when the access happens. This is discussed next paragraphs. >> + >> + Some systems 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) >> + - munmap once you don't need the buffer any more >> + >> + Therefore, for correctness and optimal performance, systems with the memory >> + cache shared by the GPU and CPU i.e. the "coherent" and also the >> + "incoherent" are always required to use SYNC_START and SYNC_END before and >> + after, respectively, when accessing the mapped address. >> >> 2. Supporting existing mmap interfaces in importers >> >> 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; > > Why? This can never happen, and you better not use dma_buf_ioctl() > outside of dma_buf_fops.. > I guess it's simply copied from the other fop callbacks, but I don't > see why. dma_buf_poll() doesn't do it, neither should this, or one of > the other 3 callbacks. yup. I fixed now. >> + >> + 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; > > This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or > EINVAL. I recommend changing it to: > > switch (sync.flags & DMA_BUF_SYNC_RW) { > case DMA_BUF_SYNC_READ: > direction = DMA_FROM_DEVICE; > break; > case DMA_BUF_SYNC_WRITE: > direction = DMA_TO_DEVICE; > break; > case DMA_BUF_SYNC_READ: > direction = DMA_BIDIRECTIONAL; > break; > default: > return -EINVAL; > } hmm I can't really get what's wrong with my snip. Why bogus? Can you double-check actually your suggestion, cause that's wrong with _READ being repeated. >> + >> + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) >> + return -EINVAL; > > Why isn't this done immediately after copy_from_user()? done. >> + >> + if (sync.flags & DMA_BUF_SYNC_END) >> + dma_buf_end_cpu_access(dmabuf, direction); >> + else >> + dma_buf_begin_cpu_access(dmabuf, direction); > > Why are SYNC_START and SYNC_END exclusive? It feels very natural to me > to invoke both at the same time (especially if two objects are stored > in the same dma-buf). Can you open a bit and teach how two objects would be stored in the same dma-buf? I didn't care about this case and if we want that, we'd need also to change the sequence of accesses as described in the dma-buf-sharing.txt I'm proposing in this patch. >> + >> + 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..4a9b36b >> --- /dev/null >> +++ b/include/uapi/linux/dma-buf.h >> @@ -0,0 +1,38 @@ >> +/* >> + * 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_ >> + >> +/* begin/end dma-buf functions used for userspace mmap. */ >> +struct dma_buf_sync { >> + __u64 flags; >> +}; > > Please add '#include <linux/types.h>', otherwise this header cannot be > compiled on its own (missing __u64). done. >> + >> +#define DMA_BUF_SYNC_READ (1 << 0) >> +#define DMA_BUF_SYNC_WRITE (2 << 0) >> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) >> +#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) > > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? yup. I've changed it to _IOWR now. Tiago
On Thu, Feb 11, 2016 at 12:54 PM, Tiago Vignatti <tiago.vignatti@intel.com> wrote: > > Thanks for reviewing, David. Please take a look in my comments in-line. > > > On 02/09/2016 07:26 AM, David Herrmann wrote: >> >> >> On Tue, Dec 22, 2015 at 10:36 PM, 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) >>> - munmap 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. >>> v6 (Tiago): use __u64 structured padded flags instead enum. Adjust >>> documentation about the recommendation on using sync ioctls. >>> v7 (Tiago): Alex' nit on flags definition and being even more wording in >>> the >>> doc about sync usage. >>> >>> 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 | 21 ++++++++++++++++++- >>> drivers/dma-buf/dma-buf.c | 43 >>> +++++++++++++++++++++++++++++++++++++++ >>> include/uapi/linux/dma-buf.h | 38 >>> ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 101 insertions(+), 1 deletion(-) >>> 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..32ac32e 100644 >>> --- a/Documentation/dma-buf-sharing.txt >>> +++ b/Documentation/dma-buf-sharing.txt >>> @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object >>> has 2 main use-cases: >>> handles, too). So it's beneficial to support this in a similar >>> fashion on >>> dma-buf to have a good transition path for existing Android >>> userspace. >>> >>> - No special interfaces, userspace simply calls mmap on the dma-buf fd. >>> + No special interfaces, userspace simply calls mmap on the dma-buf fd, >>> making >>> + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is >>> *always* >>> + used when the access happens. This is discussed next paragraphs. >>> + >>> + Some systems 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) >>> + - munmap once you don't need the buffer any more >>> + >>> + Therefore, for correctness and optimal performance, systems with the >>> memory >>> + cache shared by the GPU and CPU i.e. the "coherent" and also the >>> + "incoherent" are always required to use SYNC_START and SYNC_END >>> before and >>> + after, respectively, when accessing the mapped address. >>> >>> 2. Supporting existing mmap interfaces in importers >>> >>> 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; >> >> >> Why? This can never happen, and you better not use dma_buf_ioctl() >> outside of dma_buf_fops.. >> I guess it's simply copied from the other fop callbacks, but I don't >> see why. dma_buf_poll() doesn't do it, neither should this, or one of >> the other 3 callbacks. > > > yup. I fixed now. > >>> + >>> + 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; >> >> >> This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or >> EINVAL. I recommend changing it to: >> >> switch (sync.flags & DMA_BUF_SYNC_RW) { >> case DMA_BUF_SYNC_READ: >> direction = DMA_FROM_DEVICE; >> break; >> case DMA_BUF_SYNC_WRITE: >> direction = DMA_TO_DEVICE; >> break; >> case DMA_BUF_SYNC_READ: >> direction = DMA_BIDIRECTIONAL; >> break; >> default: >> return -EINVAL; >> } > > > hmm I can't really get what's wrong with my snip. Why bogus? Can you > double-check actually your suggestion, cause that's wrong with _READ being > repeated. > It should be something like: switch (sync.flags & DMA_BUF_SYNC_RW) { case DMA_BUF_SYNC_READ: direction = DMA_FROM_DEVICE; break; case DMA_BUF_SYNC_WRITE: direction = DMA_TO_DEVICE; break; case DMA_BUF_SYNC_RW: direction = DMA_BIDIRECTIONAL; break; default: return -EINVAL; } In your code, the first if case: + if (sync.flags & DMA_BUF_SYNC_RW) Will catch read, write and rw. Alex > >>> + >>> + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) >>> + return -EINVAL; >> >> >> Why isn't this done immediately after copy_from_user()? > > > done. > > >>> + >>> + if (sync.flags & DMA_BUF_SYNC_END) >>> + dma_buf_end_cpu_access(dmabuf, direction); >>> + else >>> + dma_buf_begin_cpu_access(dmabuf, direction); >> >> >> Why are SYNC_START and SYNC_END exclusive? It feels very natural to me >> to invoke both at the same time (especially if two objects are stored >> in the same dma-buf). > > > Can you open a bit and teach how two objects would be stored in the same > dma-buf? I didn't care about this case and if we want that, we'd need also > to change the sequence of accesses as described in the dma-buf-sharing.txt > I'm proposing in this patch. > > >>> + >>> + 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..4a9b36b >>> --- /dev/null >>> +++ b/include/uapi/linux/dma-buf.h >>> @@ -0,0 +1,38 @@ >>> +/* >>> + * 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_ >>> + >>> +/* begin/end dma-buf functions used for userspace mmap. */ >>> +struct dma_buf_sync { >>> + __u64 flags; >>> +}; >> >> >> Please add '#include <linux/types.h>', otherwise this header cannot be >> compiled on its own (missing __u64). > > > done. > > >>> + >>> +#define DMA_BUF_SYNC_READ (1 << 0) >>> +#define DMA_BUF_SYNC_WRITE (2 << 0) >>> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) >>> +#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) >> >> >> Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, >> right? > > > yup. I've changed it to _IOWR now. > > Tiago > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi On Thu, Feb 11, 2016 at 6:54 PM, Tiago Vignatti <tiago.vignatti@intel.com> wrote: > On 02/09/2016 07:26 AM, David Herrmann wrote: >>> + >>> + 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; >> >> >> This looks bogus. It always ends up being "DMA_BIDIRECTIONAL" or >> EINVAL. I recommend changing it to: >> >> switch (sync.flags & DMA_BUF_SYNC_RW) { >> case DMA_BUF_SYNC_READ: >> direction = DMA_FROM_DEVICE; >> break; >> case DMA_BUF_SYNC_WRITE: >> direction = DMA_TO_DEVICE; >> break; >> case DMA_BUF_SYNC_READ: >> direction = DMA_BIDIRECTIONAL; >> break; >> default: >> return -EINVAL; >> } > > > hmm I can't really get what's wrong with my snip. Why bogus? Can you > double-check actually your suggestion, cause that's wrong with _READ being > repeated. You did this: if (sync.flags & DMA_BUF_SYNC_RW) ...but what you meant is this: if ((sync.flags & DMA_BUF_SYNC_RW) == DMA_BUF_SYNC_RW) Feel free to fix it with this simple change. I just thought a switch() statement would be easier to read. And yes, I screwed up the third 'case' statement, which should read DMA_BUF_SYNC_RW rather than DMA_BUF_SYNC_READ. Sorry for that. >>> + >>> + if (sync.flags & DMA_BUF_SYNC_END) >>> + dma_buf_end_cpu_access(dmabuf, direction); >>> + else >>> + dma_buf_begin_cpu_access(dmabuf, direction); >> >> >> Why are SYNC_START and SYNC_END exclusive? It feels very natural to me >> to invoke both at the same time (especially if two objects are stored >> in the same dma-buf). > > > Can you open a bit and teach how two objects would be stored in the same > dma-buf? I didn't care about this case and if we want that, we'd need also > to change the sequence of accesses as described in the dma-buf-sharing.txt > I'm proposing in this patch. Just store two frames next to each other in the same BO. Create two DRM-FBs with different offsets, covering one frame each. Now you can just switch between the two FBs, backed by the same object. I'm not saying that this is a good idea. I just wondered why the START/END was exclusive, rather than inclusive. But.. I guess it is cheap enough that someone can just call ioctl(END) followed by ioctl(START). >>> + >>> +#define DMA_BUF_SYNC_READ (1 << 0) >>> +#define DMA_BUF_SYNC_WRITE (2 << 0) >>> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) >>> +#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) >> >> >> Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, >> right? > > > yup. I've changed it to _IOWR now. Well, I'd have used _IOC_NONE, rather than READ/WRITE, but I just checked and it seems vfs doesn't even enforce them. So... eh... I don't care. Thanks David
On Thu, Feb 11, 2016 at 03:54:38PM -0200, Tiago Vignatti wrote: > > Thanks for reviewing, David. Please take a look in my comments in-line. > > > On 02/09/2016 07:26 AM, David Herrmann wrote: > > > > On Tue, Dec 22, 2015 at 10:36 PM, Tiago Vignatti > > <tiago.vignatti@intel.com> wrote: > >> From: Daniel Vetter <daniel.vetter@ffwll.ch> <snip> > >> + > >> +#define DMA_BUF_SYNC_READ (1 << 0) > >> +#define DMA_BUF_SYNC_WRITE (2 << 0) > >> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) > >> +#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) > > > > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? > > yup. I've changed it to _IOWR now. AFAICS the ioctl only does copy_from_user() so _IOW seemed perfectly correct to me.
Hi On Thu, Feb 11, 2016 at 7:08 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Feb 11, 2016 at 03:54:38PM -0200, Tiago Vignatti wrote: >> >> Thanks for reviewing, David. Please take a look in my comments in-line. >> >> >> On 02/09/2016 07:26 AM, David Herrmann wrote: >> > >> > On Tue, Dec 22, 2015 at 10:36 PM, Tiago Vignatti >> > <tiago.vignatti@intel.com> wrote: >> >> From: Daniel Vetter <daniel.vetter@ffwll.ch> > <snip> >> >> + >> >> +#define DMA_BUF_SYNC_READ (1 << 0) >> >> +#define DMA_BUF_SYNC_WRITE (2 << 0) >> >> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) >> >> +#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) >> > >> > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? >> >> yup. I've changed it to _IOWR now. > > AFAICS the ioctl only does copy_from_user() so _IOW seemed perfectly > correct to me. AFAIK, the _IOC_DIR() arguments in ioctls specify the mode of the operation, not the way the arguments are used. So if read(2) was an ioctl, it would be annotated as _IOC_READ (even though it _writes_ into the passed buffer). write(2) would be annotated as _IOC_WRITE (even though it _reads_ the buffer). As such, they correspond to the file-access mode, whether you opened it readable and/or writable. Anyway, turns out VFS does not verify those. As such, you can specify whatever you want. I just checked with different existing ioctls throughout the kernel (`git grep _IOC_DIR`), and they seem to match what I describe. But I don't care much. I guess _IORW is fine either way. David
On Thu, Feb 11, 2016 at 07:19:45PM +0100, David Herrmann wrote: > Hi > > On Thu, Feb 11, 2016 at 7:08 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Thu, Feb 11, 2016 at 03:54:38PM -0200, Tiago Vignatti wrote: > >> > >> Thanks for reviewing, David. Please take a look in my comments in-line. > >> > >> > >> On 02/09/2016 07:26 AM, David Herrmann wrote: > >> > > >> > On Tue, Dec 22, 2015 at 10:36 PM, Tiago Vignatti > >> > <tiago.vignatti@intel.com> wrote: > >> >> From: Daniel Vetter <daniel.vetter@ffwll.ch> > > <snip> > >> >> + > >> >> +#define DMA_BUF_SYNC_READ (1 << 0) > >> >> +#define DMA_BUF_SYNC_WRITE (2 << 0) > >> >> +#define DMA_BUF_SYNC_RW (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) > >> >> +#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) > >> > > >> > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right? > >> > >> yup. I've changed it to _IOWR now. > > > > AFAICS the ioctl only does copy_from_user() so _IOW seemed perfectly > > correct to me. > > AFAIK, the _IOC_DIR() arguments in ioctls specify the mode of the > operation, not the way the arguments are used. So if read(2) was an > ioctl, it would be annotated as _IOC_READ (even though it _writes_ > into the passed buffer). write(2) would be annotated as _IOC_WRITE > (even though it _reads_ the buffer). As such, they correspond to the > file-access mode, whether you opened it readable and/or writable. > > Anyway, turns out VFS does not verify those. As such, you can specify > whatever you want. I just checked with different existing ioctls > throughout the kernel (`git grep _IOC_DIR`), and they seem to match > what I describe. Not sure which ones you checked because I don't think I've ever seen that interpretation in the kernel. Though I suppose often it sort of matches, eg. when the ioctl just gets/sets some value. Any relationship to some hardware operation is mostly coincidental (well, except when the hardware really just does some kind of read/write operation). And for lost ioctls there is no hardware interaction whatsoever. As far as checking the file access mode goes, well lots of ioctls totally ignore it. About the direction you're right. _IOW means userspace writes to the kernel, and _IOR means userspace reads from the kernel. Which is exactly what the code did. Anyway ioctl-numbers.txt says this: _IO an ioctl with no parameters _IOW an ioctl with write parameters (copy_from_user) _IOR an ioctl with read parameters (copy_to_user) _IOWR an ioctl with both write and read parameters. so I win ;)
diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt index 4f4a84b..32ac32e 100644 --- a/Documentation/dma-buf-sharing.txt +++ b/Documentation/dma-buf-sharing.txt @@ -350,7 +350,26 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases: handles, too). So it's beneficial to support this in a similar fashion on dma-buf to have a good transition path for existing Android userspace. - No special interfaces, userspace simply calls mmap on the dma-buf fd. + No special interfaces, userspace simply calls mmap on the dma-buf fd, making + sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always* + used when the access happens. This is discussed next paragraphs. + + Some systems 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) + - munmap once you don't need the buffer any more + + Therefore, for correctness and optimal performance, systems with the memory + cache shared by the GPU and CPU i.e. the "coherent" and also the + "incoherent" are always required to use SYNC_START and SYNC_END before and + after, respectively, when accessing the mapped address. 2. Supporting existing mmap interfaces in importers 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..4a9b36b --- /dev/null +++ b/include/uapi/linux/dma-buf.h @@ -0,0 +1,38 @@ +/* + * 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_ + +/* begin/end dma-buf functions used for userspace mmap. */ +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 (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE) +#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) + +#endif