Hi
On Thu, Feb 11, 2016 at 6:54 PM, Tiago Vignatti
<tiago.vignatti@xxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
