Re: dma-buf non-coherent mmap

[Lucas Stach <l.stach@pengutronix.de>]

Hi Thomas,

I inlined the patch, to discuss more easily.

Am Montag, den 04.11.2013, 08:53 +0100 schrieb Thomas Hellstrom:
> enum dma_buf_data_direction {
>         DMA_BUF_BIDIRECTIONAL,
>         DMA_BUF_TO_DEVICE,
>         DMA_BUF_FROM_DEVICE
> };

I don't think the DMA API semantic makes much sense here. Doing a sync
from device is at least a bit complicated, as we would have to track
down the device in which domain the buffer is currently residing for
write. This may be doable by bouncing the sync op from the exporter to
the right attachment.

The other way around is even more messier: a DMA-BUF is by definition
shared between multiple devices, so which of them should you sync to?
All of them?

I think the API used for userspace access should only be concerned with
making sure the CPU gets a coherent view to the resource at the point of
sharing i.e. sysmem. All DMA-BUF users should make sure that they sync
their writes to the point of sharing strictly before signaling that they
are done with the buffer.
Maybe this is just a naming problem, but IMHO the API should make it
really clear that the sync only makes sure that the data reaches the
point of sharing.

> 
> /**
>  * struct dma_buf_sync_arg - Argument to 
>  *
>  * @start_x:   Upper left X coordinate of area to be synced.
>  * Unit is format-dependent
>  * @start_y:   Upper left Y coordinate of area to be synced.
>  * Unit is format-dependent.
>  * @width:     Width of area to be synced. Grows to the right.
>  * Unit is format-dependent.
>  * @height:    Height of area to be synced. Grows downwards.
>  * Unit is format-dependent.
While I see why you would need this I don't really like the concept of a
two dimensional resource pushed into the API. You already need the
linear flag to make this work with 1D resources and I don't see what
happens when you encounter 3D or array resources.

Don't take this as a strong rejection as I see why this is useful, but I
don't like how the interface looks like right now.

>  * @direction: Intended transfer direction of data.
>  * @flags:     Flags to tune the synchronizing behaviour.
>  */
> 
> struct dma_buf_sync_region_arg {
>         __u32 buf_identifier;
>         __u32 start_x;
>         __u32 start_y;
>         __u32 width;
>         __u32 height;
>         enum dma_buf_data_direction direction;
>         __u32 flags;
>         __u32 pad;
> };
>         
> /**
>  * Force sync any outstanding rendering of the exporter before
> returning.
>  * This is strictly a performance hint. The user may omit this flag to
>  * speed up execution of the call, if it is known that previous
> rendering
>  * affecting (by read or write) the synchronized area has already
> finished. 
>  */
> #define DMA_BUF_SYNC_FLAG_SYNC  (1 << 0);
> 
> /**
>  * Treat @start_x as byte offset into buffer and @width as byte
>  * synchronization length. The values of @start_y and @height are
> ignored.
>  * (Separate ioctl?)
>  */
> #define DMA_BUF_SYNC_FLAG_LINEAR (1 << 1);
> 
> /**
>  * Allow the implementation to coalesce sync_for_device calls, until
> either
>  * a) An explicit flush
>  * b) A sync for cpu call with DMA_BUF_BIDIRECTIONAL or
> DMA_BUF_TO_DEVICE
>  *
>  * Note: An implementation may choose to ignore this flag.
>  */
> #define DMA_BUF_SYNC_FLAG_COALESCE (1 << 2); 
> 
> /**
>  * IOCTLS-
>  *
>  * Kernel waits should, if possible, be performed interruptible, and
> the
>  * ioctl may sett errno to EINTR if the ioctl needs to be restarted.
>  * To be discussed: Any sync operation may not affect areas outside
> the
>  * region indicated. (Good for vmwgfx, but plays ill with cache line
> alignment)
>  */
> 
> /**
>  * Sync for CPU.
>  */
> #define DMA_BUF_SYNC_REGION_FOR_CPU             \ 
> _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync_region_arg)
> 
> /**
>  * Sync for device. This is the default state of a dma-buf. 
>  */
> #define DMA_BUF_SYNC_REGION_FOR_DEVICE                          \
> _IOW(DMA_BUF_BASE, 1, struct dma_buf_sync_region_arg)
> 
> /**
>  * Flush any coalesced SYNC_REGION_FOR_DEVICE
>  */
> #define DMA_BUF_SYNC_REGION_FLUSH               \
> _IOW(DMA_BUF_BASE, 2, __u32)
> 
What is the use-case for this? Why don't you think that usespace could
be smart enough to coalesce the flush on its own?

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |