On Thu, Sep 1, 2011 at 8:19 AM, Daniel Vetter daniel@ffwll.ch wrote:
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.
Ie. add in struct device_dma_parameters?
Well, I guess we still need a way to describe "weird" requirements.. but I guess having the right place to describe them is a good start.
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.
The tricky thing is that we can't rely on all the devices seeing the buffer before the buffer starts getting used. Think video record app.. user launches app and it starts in preview mode. Then they select which format they want to encode to (ex. mpeg4 vs h264). Then they hit 'record'. And finally *now* it starts passing buffers to the encoder, the same buffers that were already circulating between the display and camera.
Well.. it is a hypothetical issue.. if the camera already has the same requirements as the encoder on some platform, then this wouldn't be a problem. So may or may not need to be solved in the first version. Certainly deferring allocating of backing pages helps. (It was also an idea Arnd had.)
BR, -R
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
Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48