| Message ID | 20190919150853.18181-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: Implement simple read/write vfs ops | expand |
On Thu, Sep 19, 2019 at 5:09 PM Chris Wilson <chris@chris-wilson.co.uk> 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. > > "It is easier to add synchronisation later, than it is to take it away." Hm for mmap I think the explicit sync makes sense (we might even want to do it in userspace). Not so sure it's a great idea for read/write ... I guess we'd need to see what people have/had in mind for the userspace for this to decide. Only other thought I have on this is that many dma-buf exporters don't bother with the kmap/kunmap interface (probably fewer than those who bother with kernel vmap and mmap), maybe we want at least a vmap fallback for this. Or maybe just use that as an excuse to get more people to wire up the kmap stuff :-) -Daniel > 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> > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > Tested-by: Laura Abbott <labbott@redhat.com> > --- > Dusting this off again as it would be nice as a user to operate on dmabuf > agnostic to the underyling driver. We have mmap, so naturally we would > like to have read/write as well! > > --- > 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 433d91d710e4..a19fc881a99c 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -135,28 +135,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; > } > > /** > @@ -413,6 +489,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 > -- > 2.23.0 >
Quoting Daniel Vetter (2019-09-19 16:28:41) > On Thu, Sep 19, 2019 at 5:09 PM Chris Wilson <chris@chris-wilson.co.uk> 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. > > > > "It is easier to add synchronisation later, than it is to take it away." > > Hm for mmap I think the explicit sync makes sense (we might even want > to do it in userspace). Not so sure it's a great idea for read/write > ... I guess we'd need to see what people have/had in mind for the > userspace for this to decide. There's an O_SYNC flag for open(2): O_SYNC Write operations on the file will complete according to the requirements of synchronized I/O file integrity completion (by contrast with the synchronized I/O data integrity completion provided by O_DSYNC.) By the time write(2) (or similar) returns, the output data and associated file metadata have been transferred to the underly‐ ing hardware (i.e., as though each write(2) was followed by a call to fsync(2)). That seems applicable here? -Chris
On Thu, Sep 19, 2019 at 5:53 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Daniel Vetter (2019-09-19 16:28:41) > > On Thu, Sep 19, 2019 at 5:09 PM Chris Wilson <chris@chris-wilson.co.uk> 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. > > > > > > "It is easier to add synchronisation later, than it is to take it away." > > > > Hm for mmap I think the explicit sync makes sense (we might even want > > to do it in userspace). Not so sure it's a great idea for read/write > > ... I guess we'd need to see what people have/had in mind for the > > userspace for this to decide. > > There's an O_SYNC flag for open(2): > > O_SYNC Write operations on the file will complete according to the > requirements of synchronized I/O file integrity completion (by > contrast with the synchronized I/O data integrity completion > provided by O_DSYNC.) > > By the time write(2) (or similar) returns, the output data and > associated file metadata have been transferred to the underly‐ > ing hardware (i.e., as though each write(2) was followed by a > call to fsync(2)). > > That seems applicable here? Hm yeah, sounds like a neat idea to use O_SYNC to decide whether writes/reads flushes or not. It's a bit a stretch since !O_SYNC would also give you un-coherent reads (or could at least). -Daniel
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 433d91d710e4..a19fc881a99c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -135,28 +135,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; } /** @@ -413,6 +489,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