On 11/04/2013 11:22 AM, Lucas Stach wrote:
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.
Good point. I think the options need to remain, but we could rename
DEVICE to STORAGE, or whatever In our case,
sync_for_cpu(DMA_BUF_TO_STORAGE) would be a no-op, whereas,
sync_for_cpu(DMA_BUF_BIDIRECTIONAL) would trigger a DMA read.
/**
* 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.
I understand. I don't think 1D only syncing options would suffice
performance-wise, as
the use-case I'm most afraid of is people trying to share 2D contents
between user-space
processes.
What do you think about supplying descriptor objects that could either
describe a
linear area, 2D area or whatever? Whenever (if) a new descriptor type is
added to the interface we could
have a legacy implementation that implements it in terms of 1D and 2D syncs.
* @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?
Right. A thinko. We could clearly leave that out.
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
