| Message ID | 20170407212514.1487-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/07/2017 02:25 PM, Chris Wilson wrote: > It is expected that processes will pass dma-buf fd between drivers, and > only using the fd themselves for mmaping and synchronisation ioctls. > However, it may be convenient for an application to read/write into the > dma-buf, for instance a test case to check the internal > dma_buf->ops->kmap interface. There may also be a small advantage to > avoiding the mmap() for very simple/small operations. > > Note in particular, synchronisation with the device is left to the > caller with an explicit DMA_BUF_IOCTL_SYNC, rather than done implicitly > inside the read/write, so that the user can avoid synchronisation if > they so choose. > > v2: Lots of little fixes, plus a real llseek() implements so that the > first basic little test cases work! > > Testcase: igt/prime_rw > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Laura Abbott <labbott@redhat.com> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Sean Paul <seanpaul@chromium.org> > --- > drivers/dma-buf/dma-buf.c | 108 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 93 insertions(+), 15 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 1ce7974a28a3..6e6e35c660c7 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -100,28 +100,104 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) > > static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) > { > - struct dma_buf *dmabuf; > - loff_t base; > + struct dma_buf *dmabuf = file->private_data; > > if (!is_dma_buf_file(file)) > return -EBADF; > > - dmabuf = file->private_data; > + return fixed_size_llseek(file, offset, whence, dmabuf->size); > +} > > - /* only support discovering the end of the buffer, > - but also allow SEEK_SET to maintain the idiomatic > - SEEK_END(0), SEEK_CUR(0) pattern */ > - if (whence == SEEK_END) > - base = dmabuf->size; > - else if (whence == SEEK_SET) > - base = 0; > - else > - return -EINVAL; > +static ssize_t dma_buf_read(struct file *file, > + char __user *ubuf, size_t remain, > + loff_t *offset) > +{ > + struct dma_buf *dmabuf = file->private_data; > + unsigned long idx; > + unsigned int start; > + size_t total; > > - if (offset != 0) > - return -EINVAL; > + if (!is_dma_buf_file(file)) > + return -EBADF; > + > + total = 0; > + idx = *offset >> PAGE_SHIFT; > + start = offset_in_page(*offset); > + while (remain) { > + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); > + unsigned int copied; > + void *vaddr; > + > + if (*offset >= dmabuf->size) > + return total; > + > + vaddr = dma_buf_kmap(dmabuf, idx); > + if (!vaddr) > + return total ?: -EIO; > + > + copied = copy_to_user(ubuf, vaddr + start, len); > + dma_buf_kunmap(dmabuf, idx, vaddr); > + > + total += copied ?: len; > + if (copied) { > + *offset += copied; > + return total ?: -EFAULT; > + } > + > + remain -= len; > + *offset += len; > + ubuf += len; > + start = 0; > + idx++; > + } > + > + return total; > +} > + > +static ssize_t dma_buf_write(struct file *file, > + const char __user *ubuf, size_t remain, > + loff_t *offset) > +{ > + struct dma_buf *dmabuf = file->private_data; > + unsigned long idx; > + unsigned int start; > + size_t total; > + > + if (!is_dma_buf_file(file)) > + return -EBADF; > + > + total = 0; > + idx = *offset >> PAGE_SHIFT; > + start = offset_in_page(*offset); > + while (remain) { > + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); > + unsigned int copied; > + void *vaddr; > + > + if (*offset >= dmabuf->size) > + return total; > + > + vaddr = dma_buf_kmap(dmabuf, idx); > + if (!vaddr) > + return total ?: -EIO; > + > + copied = copy_from_user(vaddr + start, ubuf, len); > + dma_buf_kunmap(dmabuf, idx, vaddr); > + > + total += copied ?: len; > + if (copied) { > + *offset += copied; > + return total ?: -EFAULT; > + } > + > + remain -= len; > + *offset += len; > + ubuf += len; > + start = 0; > + idx++; > + } > > - return base + offset; > + return total; > } > Both the read/write are missing the dma_buf_begin_cpu_access calls. When I add those these seem to work well enough for a simple test. I can add a real Tested-by for the next version. Thanks, Laura > /** > @@ -318,6 +394,8 @@ static const struct file_operations dma_buf_fops = { > .release = dma_buf_release, > .mmap = dma_buf_mmap_internal, > .llseek = dma_buf_llseek, > + .read = dma_buf_read, > + .write = dma_buf_write, > .poll = dma_buf_poll, > .unlocked_ioctl = dma_buf_ioctl, > #ifdef CONFIG_COMPAT >
On Wed, Apr 12, 2017 at 11:29:18AM -0700, Laura Abbott wrote: > On 04/07/2017 02:25 PM, Chris Wilson wrote: > > It is expected that processes will pass dma-buf fd between drivers, and > > only using the fd themselves for mmaping and synchronisation ioctls. > > However, it may be convenient for an application to read/write into the > > dma-buf, for instance a test case to check the internal > > dma_buf->ops->kmap interface. There may also be a small advantage to > > avoiding the mmap() for very simple/small operations. > > > > Note in particular, synchronisation with the device is left to the > > caller with an explicit DMA_BUF_IOCTL_SYNC, rather than done implicitly > > inside the read/write, so that the user can avoid synchronisation if > > they so choose. > > > > v2: Lots of little fixes, plus a real llseek() implements so that the > > first basic little test cases work! > > > > Testcase: igt/prime_rw > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Laura Abbott <labbott@redhat.com> > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/dma-buf/dma-buf.c | 108 +++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 93 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index 1ce7974a28a3..6e6e35c660c7 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -100,28 +100,104 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) > > > > static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) > > { > > - struct dma_buf *dmabuf; > > - loff_t base; > > + struct dma_buf *dmabuf = file->private_data; > > > > if (!is_dma_buf_file(file)) > > return -EBADF; > > > > - dmabuf = file->private_data; > > + return fixed_size_llseek(file, offset, whence, dmabuf->size); > > +} > > > > - /* only support discovering the end of the buffer, > > - but also allow SEEK_SET to maintain the idiomatic > > - SEEK_END(0), SEEK_CUR(0) pattern */ > > - if (whence == SEEK_END) > > - base = dmabuf->size; > > - else if (whence == SEEK_SET) > > - base = 0; > > - else > > - return -EINVAL; > > +static ssize_t dma_buf_read(struct file *file, > > + char __user *ubuf, size_t remain, > > + loff_t *offset) > > +{ > > + struct dma_buf *dmabuf = file->private_data; > > + unsigned long idx; > > + unsigned int start; > > + size_t total; > > > > - if (offset != 0) > > - return -EINVAL; > > + if (!is_dma_buf_file(file)) > > + return -EBADF; > > + > > + total = 0; > > + idx = *offset >> PAGE_SHIFT; > > + start = offset_in_page(*offset); > > + while (remain) { > > + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); > > + unsigned int copied; > > + void *vaddr; > > + > > + if (*offset >= dmabuf->size) > > + return total; > > + > > + vaddr = dma_buf_kmap(dmabuf, idx); > > + if (!vaddr) > > + return total ?: -EIO; > > + > > + copied = copy_to_user(ubuf, vaddr + start, len); > > + dma_buf_kunmap(dmabuf, idx, vaddr); > > + > > + total += copied ?: len; > > + if (copied) { > > + *offset += copied; > > + return total ?: -EFAULT; > > + } > > + > > + remain -= len; > > + *offset += len; > > + ubuf += len; > > + start = 0; > > + idx++; > > + } > > + > > + return total; > > +} > > + > > +static ssize_t dma_buf_write(struct file *file, > > + const char __user *ubuf, size_t remain, > > + loff_t *offset) > > +{ > > + struct dma_buf *dmabuf = file->private_data; > > + unsigned long idx; > > + unsigned int start; > > + size_t total; > > + > > + if (!is_dma_buf_file(file)) > > + return -EBADF; > > + > > + total = 0; > > + idx = *offset >> PAGE_SHIFT; > > + start = offset_in_page(*offset); > > + while (remain) { > > + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); > > + unsigned int copied; > > + void *vaddr; > > + > > + if (*offset >= dmabuf->size) > > + return total; > > + > > + vaddr = dma_buf_kmap(dmabuf, idx); > > + if (!vaddr) > > + return total ?: -EIO; > > + > > + copied = copy_from_user(vaddr + start, ubuf, len); > > + dma_buf_kunmap(dmabuf, idx, vaddr); > > + > > + total += copied ?: len; > > + if (copied) { > > + *offset += copied; > > + return total ?: -EFAULT; > > + } > > + > > + remain -= len; > > + *offset += len; > > + ubuf += len; > > + start = 0; > > + idx++; > > + } > > > > - return base + offset; > > + return total; > > } > > > > Both the read/write are missing the dma_buf_begin_cpu_access calls. > When I add those these seem to work well enough for a simple > test. I can add a real Tested-by for the next version. Correct, that was intentional to leave synchronisation to be managed by the caller. It is easier to add synchronisation than take it away. -Chris
On 04/12/2017 12:38 PM, Chris Wilson wrote: > On Wed, Apr 12, 2017 at 11:29:18AM -0700, Laura Abbott wrote: >> On 04/07/2017 02:25 PM, Chris Wilson wrote: >>> It is expected that processes will pass dma-buf fd between drivers, and >>> only using the fd themselves for mmaping and synchronisation ioctls. >>> However, it may be convenient for an application to read/write into the >>> dma-buf, for instance a test case to check the internal >>> dma_buf->ops->kmap interface. There may also be a small advantage to >>> avoiding the mmap() for very simple/small operations. >>> >>> Note in particular, synchronisation with the device is left to the >>> caller with an explicit DMA_BUF_IOCTL_SYNC, rather than done implicitly >>> inside the read/write, so that the user can avoid synchronisation if >>> they so choose. >>> >>> v2: Lots of little fixes, plus a real llseek() implements so that the >>> first basic little test cases work! >>> >>> Testcase: igt/prime_rw >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Laura Abbott <labbott@redhat.com> >>> Cc: Sumit Semwal <sumit.semwal@linaro.org> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Sean Paul <seanpaul@chromium.org> >>> --- >>> drivers/dma-buf/dma-buf.c | 108 +++++++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 93 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 1ce7974a28a3..6e6e35c660c7 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -100,28 +100,104 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) >>> >>> static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) >>> { >>> - struct dma_buf *dmabuf; >>> - loff_t base; >>> + struct dma_buf *dmabuf = file->private_data; >>> >>> if (!is_dma_buf_file(file)) >>> return -EBADF; >>> >>> - dmabuf = file->private_data; >>> + return fixed_size_llseek(file, offset, whence, dmabuf->size); >>> +} >>> >>> - /* only support discovering the end of the buffer, >>> - but also allow SEEK_SET to maintain the idiomatic >>> - SEEK_END(0), SEEK_CUR(0) pattern */ >>> - if (whence == SEEK_END) >>> - base = dmabuf->size; >>> - else if (whence == SEEK_SET) >>> - base = 0; >>> - else >>> - return -EINVAL; >>> +static ssize_t dma_buf_read(struct file *file, >>> + char __user *ubuf, size_t remain, >>> + loff_t *offset) >>> +{ >>> + struct dma_buf *dmabuf = file->private_data; >>> + unsigned long idx; >>> + unsigned int start; >>> + size_t total; >>> >>> - if (offset != 0) >>> - return -EINVAL; >>> + if (!is_dma_buf_file(file)) >>> + return -EBADF; >>> + >>> + total = 0; >>> + idx = *offset >> PAGE_SHIFT; >>> + start = offset_in_page(*offset); >>> + while (remain) { >>> + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); >>> + unsigned int copied; >>> + void *vaddr; >>> + >>> + if (*offset >= dmabuf->size) >>> + return total; >>> + >>> + vaddr = dma_buf_kmap(dmabuf, idx); >>> + if (!vaddr) >>> + return total ?: -EIO; >>> + >>> + copied = copy_to_user(ubuf, vaddr + start, len); >>> + dma_buf_kunmap(dmabuf, idx, vaddr); >>> + >>> + total += copied ?: len; >>> + if (copied) { >>> + *offset += copied; >>> + return total ?: -EFAULT; >>> + } >>> + >>> + remain -= len; >>> + *offset += len; >>> + ubuf += len; >>> + start = 0; >>> + idx++; >>> + } >>> + >>> + return total; >>> +} >>> + >>> +static ssize_t dma_buf_write(struct file *file, >>> + const char __user *ubuf, size_t remain, >>> + loff_t *offset) >>> +{ >>> + struct dma_buf *dmabuf = file->private_data; >>> + unsigned long idx; >>> + unsigned int start; >>> + size_t total; >>> + >>> + if (!is_dma_buf_file(file)) >>> + return -EBADF; >>> + >>> + total = 0; >>> + idx = *offset >> PAGE_SHIFT; >>> + start = offset_in_page(*offset); >>> + while (remain) { >>> + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); >>> + unsigned int copied; >>> + void *vaddr; >>> + >>> + if (*offset >= dmabuf->size) >>> + return total; >>> + >>> + vaddr = dma_buf_kmap(dmabuf, idx); >>> + if (!vaddr) >>> + return total ?: -EIO; >>> + >>> + copied = copy_from_user(vaddr + start, ubuf, len); >>> + dma_buf_kunmap(dmabuf, idx, vaddr); >>> + >>> + total += copied ?: len; >>> + if (copied) { >>> + *offset += copied; >>> + return total ?: -EFAULT; >>> + } >>> + >>> + remain -= len; >>> + *offset += len; >>> + ubuf += len; >>> + start = 0; >>> + idx++; >>> + } >>> >>> - return base + offset; >>> + return total; >>> } >>> >> >> Both the read/write are missing the dma_buf_begin_cpu_access calls. >> When I add those these seem to work well enough for a simple >> test. I can add a real Tested-by for the next version. > > Correct, that was intentional to leave synchronisation to be managed by > the caller. It is easier to add synchronisation than take it away. > -Chris > Ah yes, that makes sense. This is what the sync ioctls were for in the first place :). You can add Tested-by: Laura Abbott <labbott@redhat.com> Thanks, Laura
On Wed, Apr 12, 2017 at 12:57:00PM -0700, Laura Abbott wrote: > On 04/12/2017 12:38 PM, Chris Wilson wrote: > > On Wed, Apr 12, 2017 at 11:29:18AM -0700, Laura Abbott wrote: > >> Both the read/write are missing the dma_buf_begin_cpu_access calls. > >> When I add those these seem to work well enough for a simple > >> test. I can add a real Tested-by for the next version. > > > > Correct, that was intentional to leave synchronisation to be managed by > > the caller. It is easier to add synchronisation than take it away. > > -Chris > > > > Ah yes, that makes sense. This is what the sync ioctls were for in > the first place :). You can add > > Tested-by: Laura Abbott <labbott@redhat.com> Back to the main point of the exercise, does this fulfil the needs for ion/dmabuf testing? We have mmap and now kmap coverage, just by using dmabuf fops - but I can't see a reasonable avenue to do vmap testing. And you will still want vgem for import testing. Again, I can't see a good excuse for vmap... Do you have a set of kselftests in conjunction to userspace testing? Would it be worth making a common mock_dmabuf.ko and/or a dmabuf kselftest framework for producers/consumers to plug into their own kselftests? Please stop me if I'm overengineering! -Chris
On 04/12/2017 01:12 PM, Chris Wilson wrote: > On Wed, Apr 12, 2017 at 12:57:00PM -0700, Laura Abbott wrote: >> On 04/12/2017 12:38 PM, Chris Wilson wrote: >>> On Wed, Apr 12, 2017 at 11:29:18AM -0700, Laura Abbott wrote: >>>> Both the read/write are missing the dma_buf_begin_cpu_access calls. >>>> When I add those these seem to work well enough for a simple >>>> test. I can add a real Tested-by for the next version. >>> >>> Correct, that was intentional to leave synchronisation to be managed by >>> the caller. It is easier to add synchronisation than take it away. >>> -Chris >>> >> >> Ah yes, that makes sense. This is what the sync ioctls were for in >> the first place :). You can add >> >> Tested-by: Laura Abbott <labbott@redhat.com> > > Back to the main point of the exercise, does this fulfil the needs for > ion/dmabuf testing? > > We have mmap and now kmap coverage, just by using dmabuf fops - but I > can't see a reasonable avenue to do vmap testing. > > And you will still want vgem for import testing. Again, I can't see a > good excuse for vmap... > Yes, this should be good for Ion testing. Ion doesn't implement vmap ops right now so that's moot... > Do you have a set of kselftests in conjunction to userspace testing? > Would it be worth making a common mock_dmabuf.ko and/or a dmabuf > kselftest framework for producers/consumers to plug into their own > kselftests? Please stop me if I'm overengineering! > -Chris > ...but in the interest of general testing I think getting some sort of unit test would be good. I don't have anything in kernel yet for Ion so maybe some generic dma_buf test would be useful. I'll see what I come up with. Thanks, Laura
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1ce7974a28a3..6e6e35c660c7 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -100,28 +100,104 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) { - struct dma_buf *dmabuf; - loff_t base; + struct dma_buf *dmabuf = file->private_data; if (!is_dma_buf_file(file)) return -EBADF; - dmabuf = file->private_data; + return fixed_size_llseek(file, offset, whence, dmabuf->size); +} - /* only support discovering the end of the buffer, - but also allow SEEK_SET to maintain the idiomatic - SEEK_END(0), SEEK_CUR(0) pattern */ - if (whence == SEEK_END) - base = dmabuf->size; - else if (whence == SEEK_SET) - base = 0; - else - return -EINVAL; +static ssize_t dma_buf_read(struct file *file, + char __user *ubuf, size_t remain, + loff_t *offset) +{ + struct dma_buf *dmabuf = file->private_data; + unsigned long idx; + unsigned int start; + size_t total; - if (offset != 0) - return -EINVAL; + if (!is_dma_buf_file(file)) + return -EBADF; + + total = 0; + idx = *offset >> PAGE_SHIFT; + start = offset_in_page(*offset); + while (remain) { + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); + unsigned int copied; + void *vaddr; + + if (*offset >= dmabuf->size) + return total; + + vaddr = dma_buf_kmap(dmabuf, idx); + if (!vaddr) + return total ?: -EIO; + + copied = copy_to_user(ubuf, vaddr + start, len); + dma_buf_kunmap(dmabuf, idx, vaddr); + + total += copied ?: len; + if (copied) { + *offset += copied; + return total ?: -EFAULT; + } + + remain -= len; + *offset += len; + ubuf += len; + start = 0; + idx++; + } + + return total; +} + +static ssize_t dma_buf_write(struct file *file, + const char __user *ubuf, size_t remain, + loff_t *offset) +{ + struct dma_buf *dmabuf = file->private_data; + unsigned long idx; + unsigned int start; + size_t total; + + if (!is_dma_buf_file(file)) + return -EBADF; + + total = 0; + idx = *offset >> PAGE_SHIFT; + start = offset_in_page(*offset); + while (remain) { + unsigned int len = min_t(size_t, remain, PAGE_SIZE - start); + unsigned int copied; + void *vaddr; + + if (*offset >= dmabuf->size) + return total; + + vaddr = dma_buf_kmap(dmabuf, idx); + if (!vaddr) + return total ?: -EIO; + + copied = copy_from_user(vaddr + start, ubuf, len); + dma_buf_kunmap(dmabuf, idx, vaddr); + + total += copied ?: len; + if (copied) { + *offset += copied; + return total ?: -EFAULT; + } + + remain -= len; + *offset += len; + ubuf += len; + start = 0; + idx++; + } - return base + offset; + return total; } /** @@ -318,6 +394,8 @@ static const struct file_operations dma_buf_fops = { .release = dma_buf_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, + .read = dma_buf_read, + .write = dma_buf_write, .poll = dma_buf_poll, .unlocked_ioctl = dma_buf_ioctl, #ifdef CONFIG_COMPAT
It is expected that processes will pass dma-buf fd between drivers, and only using the fd themselves for mmaping and synchronisation ioctls. However, it may be convenient for an application to read/write into the dma-buf, for instance a test case to check the internal dma_buf->ops->kmap interface. There may also be a small advantage to avoiding the mmap() for very simple/small operations. Note in particular, synchronisation with the device is left to the caller with an explicit DMA_BUF_IOCTL_SYNC, rather than done implicitly inside the read/write, so that the user can avoid synchronisation if they so choose. v2: Lots of little fixes, plus a real llseek() implements so that the first basic little test cases work! Testcase: igt/prime_rw Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Laura Abbott <labbott@redhat.com> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Sean Paul <seanpaul@chromium.org> --- drivers/dma-buf/dma-buf.c | 108 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 93 insertions(+), 15 deletions(-)