Hello,

On Thursday, September 08, 2011 6:42 PM Laura Abbott wrote:

Hi, a few comments
On Fri, September 2, 2011 6:56 am, Marek Szyprowski wrote:
...

quoted

+
+struct dma_iommu_mapping {
+	/* iommu specific data */
+	struct iommu_domain	*domain;
+
+	void			*bitmap;

In the earlier version of this patch you had this as a genpool instead of
just doing the bitmaps manually. Is there a reason genpool can't be used
to get the iova addresses?

IMHO genpool was a bit overkill in this case and required some additional
patches for aligned allocations. In the next version I also want to extend
this bitmap based allocator to dynamically resize the bitmap for more than
one page if the io address space gets exhausted. 

quoted

+	size_t			bits;
+	unsigned int		order;
+	dma_addr_t		base;
+
+	struct mutex		lock;
+};

<snip>

quoted

+int arm_iommu_attach_device(struct device *dev, dma_addr_t base, size_t
size, int order)
+{
+	unsigned int count = (size >> PAGE_SHIFT) - order;
+	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
+	struct dma_iommu_mapping *mapping;
+	int err = -ENOMEM;
+
+	mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL);
+	if (!mapping)
+		goto err;
+
+	mapping->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
+	if (!mapping->bitmap)
+		goto err2;
+
+	mapping->base = base;
+	mapping->bits = bitmap_size;
+	mapping->order = order;
+	mutex_init(&mapping->lock);
+
+	mapping->domain = iommu_domain_alloc();
+	if (!mapping->domain)
+		goto err3;
+
+	err = iommu_attach_device(mapping->domain, dev);
+	if (err != 0)
+		goto err4;
+
+	dev->archdata.mapping = mapping;
+	set_dma_ops(dev, &iommu_ops);
+
+	printk(KERN_INFO "Attached IOMMU controller to %s device.\n",
dev_name(dev));
+	return 0;
+
+err4:
+	iommu_domain_free(mapping->domain);
+err3:
+	kfree(mapping->bitmap);
+err2:
+	kfree(mapping);
+err:
+	return -ENOMEM;
+}
+EXPORT_SYMBOL(arm_iommu_attach_device);
+
+#endif

Attach makes the assumption that each iommu device will exist in a
separate domain. What if multiple devices want to use the same iommu
domain? The msm iommu implementation has many different iommu devices but
many of these will need the same buffer to be mapped in each context so
currently many devices share the same domain. Without this, the same map
call would need to happen for each device, which creates extra map calls
and overhead.

Ah, right. I forgot about the case when devices need to share one domain.
Moving iommu_domain_alloc out of arm_iommu_attach_device and giving that
function just a pointer to the iommu domain should solve this issue. I will
change this in the next version of the patches.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center