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;elsereturn -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);elsedma_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