Re: [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Tomasz Stanislawski]

Hi Sumit,

On 12/26/2011 10:23 AM, Sumit Semwal wrote:
> This is the first step in defining a dma buffer sharing mechanism.
>
> A new buffer object dma_buf is added, with operations and API to allow easy
> sharing of this buffer object across devices.
>
> The framework allows:
> - creation of a buffer object, its association with a file pointer, and
>     associated allocator-defined operations on that buffer. This operation is
>     called the 'export' operation.
> - different devices to 'attach' themselves to this exported buffer object, to
>    facilitate backing storage negotiation, using dma_buf_attach() API.
> - the exported buffer object to be shared with the other entity by asking for
>     its 'file-descriptor (fd)', and sharing the fd across.
> - a received fd to get the buffer object back, where it can be accessed using
>     the associated exporter-defined operations.
> - the exporter and user to share the scatterlist associated with this buffer
>     object using map_dma_buf and unmap_dma_buf operations.

[snip]

> +/**
> + * struct dma_buf_attachment - holds device-buffer attachment data
> + * @dmabuf: buffer for this attachment.
> + * @dev: device attached to the buffer.
> + * @node: list of dma_buf_attachment.
> + * @priv: exporter specific attachment data.
> + *
> + * This structure holds the attachment information between the dma_buf buffer
> + * and its user device(s). The list contains one attachment struct per device
> + * attached to the buffer.
> + */
> +struct dma_buf_attachment {
> +	struct dma_buf *dmabuf;
> +	struct device *dev;
> +	struct list_head node;
> +	void *priv;
> +};
> +
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> +							struct device *dev);
> +void dma_buf_detach(struct dma_buf *dmabuf,
> +				struct dma_buf_attachment *dmabuf_attach);
> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
> +			size_t size, int flags);
> +int dma_buf_fd(struct dma_buf *dmabuf);
> +struct dma_buf *dma_buf_get(int fd);
> +void dma_buf_put(struct dma_buf *dmabuf);
> +
> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> +					enum dma_data_direction);
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *);

I think that you should add enum dma_data_direction as an argument
unmap function. It was mentioned that the dma_buf_attachment should keep
cached and mapped sg_table for performance reasons. The field
dma_buf_attachment::priv seams to be a natural place to keep this sg_table.
To map a buffer the exporter calls dma_map_sg. It needs dma direction
as an argument. The problem is that dma_unmap_sg also needs this
argument but dma direction is not available neither in
dma_buf_unmap_attachment nor in unmap callback. Therefore the exporter
is forced to embed returned sg_table into a bigger structure where dma 
direction is remembered. Refer to function vb2_dc_dmabuf_ops_map at
link below as an example:

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793/focus=43797

Regards,
Tomasz Stanislawski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/