[PATCH v7 3/5] dma-buf: Add ioctls to allow userspace to flush

[David Herrmann]

Hi

On Thu, Feb 11, 2016 at 6:54 PM, Tiago Vignatti
<tiago.vignatti at 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