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."
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
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 idiomaticSEEK_END(0), SEEK_CUR(0) pattern */if (whence == SEEK_END)base = dmabuf->size;else if (whence == SEEK_SET)base = 0;elsereturn -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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter