about mmap dma-buf and sync

[Thomas Hellstrom]

Hi, Daniel,

On 08/24/2015 05:52 PM, Daniel Stone wrote:
> Hi Thomas,
>
> On 24 August 2015 at 10:50, Thomas Hellstrom <thomas at shipmail.org> wrote:
>> In any case, Ideally I'd want the struct dma_buf_sync look something like
>>
>> enum dma_buf_sync_flags {
>>         DMA_BUF_SYNC_READ = (1 << 0),
>>         DMA_BUF_SYNC_WRITE = (2 << 0),
>>         DMA_BUF_SYNC_RW = (3 << 0),
>>         DMA_BUF_SYNC_START = (0 << 2),
>>         DMA_BUF_SYNC_END = (1 << 2),
>>
>>         DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
>>                 DMA_BUF_SYNC_END
>> };
>>
>>
>> /* begin/end dma-buf functions used for userspace mmap. */
>> struct dma_buf_sync {
>>         enum dma_buf_sync_flags flags;
>>         __u64 x;
>>         __u64 width;
>>
>>         /* 2D Sync parameters */
>>         __u64 stride;
>>         __u64 y;
>>         __u64 height;
>>
>> }
>>
>>
>> So again, why use a 2D sync in the first place when it's probably possible to batch 1D syncs in the driver?
>> It all boils down to the number of syscalls required, and also the fact that it's probably not trivial to batch sync calls in the read-write case. One can, again, argue that there might be 3D cases or other obscure sync cases,
>> but in that case, let's implement them in the future if the overhead becomes unbearable. If the sync mechanism gets established as
>> mandatory, the problem will solve itself. And at least now we cover the most common case and we don't need to add a new multiplexing
>> mechanism in the sync IOCTL since we already have one in the IOCTL mechanism. We can simply add a new sync IOCTL for new obscure sync cases if desired.
> I still don't think this ameliorates the need for batching: consider
> the case where you update two disjoint screen regions and want them
> both flushed. Either you issue two separate sync calls (which can be
> disadvantageous performance-wise on some hardware / setups), or you
> accumulate the regions and only flush later. So either two ioctls (one
> in the style of dirtyfb and one to perform the sync/flush; you can
> shortcut to assume the full buffer was damaged if the former is
> missing), or one like this:
>
> struct dma_buf_sync_2d {
>         enum dma_buf_sync_flags flags;
>
>         __u64 stride_bytes;
>         __u32 bytes_per_pixel;
>         __u32 num_regions;
>
>         struct {
>                 __u64 x;
>                 __u64 y;
>                 __u64 width;
>                 __u64 height;
>         } regions[];
> };
>
> Cheers,
> Daniel

Fine with me, although perhaps bytes_per_pixel is a bit redundant?

/Thomas