On Fri, May 4, 2012 at 3:33 PM, Konrad Rzeszutek Wilk konrad.wilk@oracle.com wrote:
On Fri, May 04, 2012 at 02:46:54PM +0100, Dave Airlie wrote:
From: Dave Airlie airlied@redhat.com
This adds the ability for ttm common code to take an SG table and use it as the backing for a slave TTM object.
I took a look at both patches from the perspective of the TTM DMA pool code and it looks fine (thought I would prefer to test this first - but I don't have the hardware).
What I am curious is that you are allowing to map two or more of SG pages to different PCI devices. And for that you are using the nouveau_gem_map_dma_buf, which does this (roughly)
ttm->sg = sg_alloc_table(). for_each_sg() sg->dma_address[i] = dma_map(..)
So I am seeing two potential issues: 1). ttm->sg = sg_alloc() is called on every new other device. In other words, if you end up with three of these attachment PCI devices you are going to cause an memory leak of the sg_alloc() and over-write the ttm->sg every time. But I might be misreading how the import code works.
I think you are mixing up the export and import sides.
The export side takes a TTM object, and create an sg table mapped into the importers address space.
The import side takes an exported sg_table and wraps an object around it.
So for multiple importers, there are multiple sg_tables. For one import there should only be one sg_table per object.
2). You are going to put in sg->dma_address() in of different PCI devices. Meaning - the dma address might have a different value depending on whether there is an different IOMMU for each of the devices. On x86 that is usually not the case (the odd ball being the IBM high-end boxes that have an Calgary-X IOMMU for PCI buses). What this means is that the DMA address you are programming in the sub-sequent PCI devices might the DMA address for a previous PCI device.
But both of these issues are only a problem if the slave cards have a different PCI device. If they are the same - then I think you are OK (except the multiple calls to import which would cause a memory leak).
Yeah we need to stop a slave calling map_dma_buf multiple times, but we block that by keeping a list of currently imported buffer handles.
Dave.