On Thu, Aug 11, 2011 at 05:02:21AM -0500, Clark, Rob wrote:
On Wed, Aug 10, 2011 at 7:11 AM, Sumit Semwal sumit.semwal@ti.com wrote:
+/**
- struct dma_buf_ops - operations possible on struct dmabuf
- @get_scatterlist: returns list of scatter pages allocated, increases
- usecount of the buffer
- @put_scatterlist: decreases usecount of buffer, might deallocate scatter
- pages
- @mmap: map this buffer
- @read: read from this buffer
- @write: write to this buffer
- @release: release this buffer; to be called after the last dma_buf_put
- @sync_sg_for_cpu: sync the sg list for cpu
- @sync_sg_for_device: synch the sg list for device
- */
+struct dma_buf_ops {
- /* allow buffer to not be pinned when DMA is not happening */
- struct scatterlist * (*get_scatterlist)(struct dma_buf *);
- void (*put_scatterlist)(struct dma_buf *, struct scatterlist *);
One thought, {get,put}_scatterlist() gives the allocating driver half of what it needs to implement a sync object.. ie. if receiving driver does a "get" before DMA, and "put" on completion, this tells the allocating driver that the buffer is in use. If we added an 'op' to get_scatterlist which was READ &/| WRITE this would even tell the allocator the nature of the DMA which could later be used to implement a wait(op).
Another thought.. at the risk of re-opening a can of worms.. an 'attrib' flag/mask as a param to get_scatterlist() could allow the allocator to defer backing the buffer w/ real memory until the first get_scatterlist() call, for now attrib flag could be discontig, contig, and weird (as Hans suggested) and we can try to define more precisely "weird" later but for now can have platform specific meaning perhaps. This gives a sort of "poor mans negotiation" which might help on some platforms for now..
You've probably discussed this all at the recent summit I've totally missed, but why not add a struct device * pointer argument to get_scatterlist? This way we could shovel all the platform specific complexity into platform specific code.
For devices that have different dma requirements depending upon function (i.e. contigous for scanout, iommu-mapped with large pages for overlay image) we could use a bunch of subdevices to map all the different requirements.
Another similar idea could also solve the backing storage negotiation problem: A dma_buf object is at first just an abstract handle (with a fixed size), then you can attach users with dma_buf_attach_device. Then, after all users have themselves attached, the first get_scatterlist call actually allocates the backing storage for the buffer. A subsequent attach_device would then return -EBUSY.
This way we don't have a special device that allocates a buffer but instead all the knowledge about dma buffer handling sits in the platform specific dma code. With that design get_scatterlist should imo then also return an already mapped sg list, i.e. what dma_map_sg would return. Probably rename them to dma_map_buffer/dma_unmap_buffer.
In short, I imagine an api like this (neglecting the actual buffer sharing part and reference counting boiler plate):
struct dma_buf * dma_buf_create(size_t size); int dma_buf_attach_device(struct dma_buf *buf, struct device *device); /* might return -EBUSY to signal that the backing storage is * already allocated and incompatible with the device's * requirements */ struct scatterlist *dma_buf_map(struct dma_buf *buf, struct device *device, int *ents); /* not allowed without a preceeding attach_device */ void dma_buf_unmap(struct dma_buf *buf, struct device *device, struct scatterlist *sg, int ents);
Yours, Daniel