Re: Tegra-DRM: Tegra30: DMA mapping API

Re: Tegra-DRM: Tegra30: DMA mapping API

[Hiroshi Doyu]

Hi Joerg,

On Tue, 10 Jul 2012 12:24:17 +0200
Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, 10 Jul 2012 10:14:01 +0200
> markz <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Mon, 2012-07-09 at 23:28 +0800, Hiroshi Doyu wrote:
> > > Hi Mark, adding linux-tegra on Cc:.... 
> > > 
> > > Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Mon, 9 Jul 2012 14:13:13 +0200:
> > > 
> > > > > Hi Mark, Cc: Thierry & Ken,
> > > > >
> > > > > Can GEM allow DRM to use contigious memory? That could be easier.
> > > > >
> > > > > Otherwise, I think that you need to make drv->dev iommu'able before
> > > > > calling DMA API if you want to use IOMMU. To make drv->dev iommu'able,
> > > > > you need to call arm_iommu_create_mapping() and arm_iommu_attach_device().
> > > > 
> > > > Thank you. Yes, we need to call these functions otherwise iommu will not be enabled at all.
> > > > So my question is:
> > > > 1. in arm_iommu_create_mapping, we need to specify the base address
> > > > of the iova. Is these an API which can get this base address? I
> > > > mean, SMMU serves a lot of clients and maybe every client will
> > > > specify a range of iova which it needs. So as the center point of
> > > > these clients, does SMMU module has a function which can be used by
> > > > client to get this base address?
> > > 
> > > The SMMU patch was sent but it depends on the following framework.
> > > 
> > >   [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware
> > >     https://lkml.org/lkml/2012/1/19/170
> > > 
> > 
> > Got it. Seems this patch hasn't been integrated yet. Besides, SMMU
> > module should do some implementations for this set/get attribute
> > functions as well, right?
> 
> Yes, that's below:
> http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html

Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
for GART-like hardware"?

Tegra DRM seems to need that feature eventually.

Re: Tegra-DRM: Tegra30: DMA mapping API

[Joerg Roedel]

Hi,

On Tue, Jul 10, 2012 at 01:26:39PM +0300, Hiroshi Doyu wrote:
> > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> 
> Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
> for GART-like hardware"?
> 
> Tegra DRM seems to need that feature eventually.

I just rebased and updated the patch-set. A new version is sent out. If
no major objections come up I queue it for the next merge window.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: Tegra-DRM: Tegra30: DMA mapping API

[Hiroshi Doyu]

Hi Joerg,

On Wed, 11 Jul 2012 12:58:27 +0200
Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote:

> Hi,
> 
> On Tue, Jul 10, 2012 at 01:26:39PM +0300, Hiroshi Doyu wrote:
> > > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> > 
> > Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
> > for GART-like hardware"?
> > 
> > Tegra DRM seems to need that feature eventually.
> 
> I just rebased and updated the patch-set. A new version is sent out. If
> no major objections come up I queue it for the next merge window.

Thank you for your prompt action. This would help us a lot.

Re: Tegra-DRM: Tegra30: DMA mapping API

[markz]

On Wed, 2012-07-11 at 19:42 +0800, Hiroshi Doyu wrote:
> Hi Joerg,
> 
> On Wed, 11 Jul 2012 12:58:27 +0200
> Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote:
> 
> > Hi,
> > 
> > On Tue, Jul 10, 2012 at 01:26:39PM +0300, Hiroshi Doyu wrote:
> > > > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> > > 
> > > Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
> > > for GART-like hardware"?
> > > 
> > > Tegra DRM seems to need that feature eventually.
> > 
> > I just rebased and updated the patch-set. A new version is sent out. If
> > no major objections come up I queue it for the next merge window.
> 
> Thank you for your prompt action. This would help us a lot.

That's great. So Hiroshi-san, about your patch: 
http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
, what is your plan to merge this?
After Joerg's patch and yours are merged into linux-next, I can submit
my drm patch into review process.

Re: Tegra-DRM: Tegra30: DMA mapping API

[Hiroshi Doyu]

On Thu, 12 Jul 2012 05:04:33 +0200
markz <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> On Wed, 2012-07-11 at 19:42 +0800, Hiroshi Doyu wrote:
> > Hi Joerg,
> > 
> > On Wed, 11 Jul 2012 12:58:27 +0200
> > Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote:
> > 
> > > Hi,
> > > 
> > > On Tue, Jul 10, 2012 at 01:26:39PM +0300, Hiroshi Doyu wrote:
> > > > > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> > > > 
> > > > Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
> > > > for GART-like hardware"?
> > > > 
> > > > Tegra DRM seems to need that feature eventually.
> > > 
> > > I just rebased and updated the patch-set. A new version is sent out. If
> > > no major objections come up I queue it for the next merge window.
> > 
> > Thank you for your prompt action. This would help us a lot.
> 
> That's great. So Hiroshi-san, about your patch: 
> http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> , what is your plan to merge this?

That's also under review along with the framework as below:

[PATCH 6/7] iommu/tegra: Implement DOMAIN_ATTR_GEOMETRY attribute  
http://lists.linuxfoundation.org/pipermail/iommu/2012-July/004432.html

> After Joerg's patch and yours are merged into linux-next, I can submit
> my drm patch into review process.

You can also send them now for early review with some note mentioning
that your patches depend on the above "[PATCH 6/7] iommu/tegra:
Implement DOMAIN_ATTR_GEOMETRY attribute" in the commit log.

Re: Tegra-DRM: Tegra30: DMA mapping API

[markz]

On Thu, 2012-07-12 at 11:33 +0800, Hiroshi Doyu wrote:
> On Thu, 12 Jul 2012 05:04:33 +0200
> markz <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Wed, 2012-07-11 at 19:42 +0800, Hiroshi Doyu wrote:
> > > Hi Joerg,
> > > 
> > > On Wed, 11 Jul 2012 12:58:27 +0200
> > > Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Tue, Jul 10, 2012 at 01:26:39PM +0300, Hiroshi Doyu wrote:
> > > > > > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> > > > > 
> > > > > Do you have any plan to merge the above "IOMMU: Make IOMMU-API ready
> > > > > for GART-like hardware"?
> > > > > 
> > > > > Tegra DRM seems to need that feature eventually.
> > > > 
> > > > I just rebased and updated the patch-set. A new version is sent out. If
> > > > no major objections come up I queue it for the next merge window.
> > > 
> > > Thank you for your prompt action. This would help us a lot.
> > 
> > That's great. So Hiroshi-san, about your patch: 
> > http://lists.linuxfoundation.org/pipermail/iommu/2012-January/003531.html
> > , what is your plan to merge this?
> 
> That's also under review along with the framework as below:
> 
> [PATCH 6/7] iommu/tegra: Implement DOMAIN_ATTR_GEOMETRY attribute  
> http://lists.linuxfoundation.org/pipermail/iommu/2012-July/004432.html
> 
> > After Joerg's patch and yours are merged into linux-next, I can submit
> > my drm patch into review process.
> 
> You can also send them now for early review with some note mentioning
> that your patches depend on the above "[PATCH 6/7] iommu/tegra:
> Implement DOMAIN_ATTR_GEOMETRY attribute" in the commit log.

Thank you. I merged this patch set locally but seems it has problems:
The requirement of the drm driver is: It needs a parameter named "base"
while calling function "arm_iommu_create_mapping" to reserve a block of
iova from GART/SMMU.
This patch set defines function "iommu_domain_get_attr" &
"iommu_domain_set_attr", but drm driver can't use them:
1. These 2 functions need a iommu_domain param which means the
iommu_domain should be created before calling them. But the iommu_domain
is created in function "arm_iommu_create_mapping".
2. It's not a good practice to make drm driver calls iommu apis
directly.

Actually, I'm curious that why function "arm_iommu_create_mapping" needs
this "base" param. AIUI, drm driver just needs to tell IOMMU the size of
the iova address to reserve, then the base address can be checked out in
a lot of ways. For example: 
1. Via "dma_map_sg" interface -- dma_map_sg defines we can check out the
mapped iova address from "dma_address" member in scatterlist structure.
2. Use this iommu get/set attr interface. Of course, we need some dma
mapping api wrappers on them.

Mark

Re: Tegra-DRM: Tegra30: DMA mapping API

[Hiroshi Doyu]

On Thu, 12 Jul 2012 08:49:05 +0200
markz <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
.....
> > > After Joerg's patch and yours are merged into linux-next, I can submit
> > > my drm patch into review process.
> > 
> > You can also send them now for early review with some note mentioning
> > that your patches depend on the above "[PATCH 6/7] iommu/tegra:
> > Implement DOMAIN_ATTR_GEOMETRY attribute" in the commit log.
> 
> Thank you. I merged this patch set locally but seems it has problems:
> The requirement of the drm driver is: It needs a parameter named "base"
> while calling function "arm_iommu_create_mapping" to reserve a block of
> iova from GART/SMMU.
> This patch set defines function "iommu_domain_get_attr" &
> "iommu_domain_set_attr", but drm driver can't use them:
> 1. These 2 functions need a iommu_domain param which means the
> iommu_domain should be created before calling them. But the iommu_domain
> is created in function "arm_iommu_create_mapping".
> 2. It's not a good practice to make drm driver calls iommu apis
> directly.
> 
> Actually, I'm curious that why function "arm_iommu_create_mapping" needs
> this "base" param. AIUI, drm driver just needs to tell IOMMU the size of
> the iova address to reserve, then the base address can be checked out in
> a lot of ways. For example: 
> 1. Via "dma_map_sg" interface -- dma_map_sg defines we can check out the
> mapped iova address from "dma_address" member in scatterlist structure.
> 2. Use this iommu get/set attr interface. Of course, we need some dma
> mapping api wrappers on them.

arm_iommu_attach_device() should be called independently from DRM
driver in advance. In DRM driver, "struct device" should be connected
to map. Usually this can be done in board files, but with DT, we need
new framework to deal with this at once. Especially in Tegra, we have
many IOMMU'able devices, and it's not good idea to do respectively.

For temporary solution, you can call arm_iommu_create_mapping() and
arm_iommu_attach_device() in advance before DRM uses DMA API.

Re: Tegra-DRM: Tegra30: DMA mapping API

[markz]

On Thu, 2012-07-12 at 15:11 +0800, Hiroshi Doyu wrote:
> On Thu, 12 Jul 2012 08:49:05 +0200
> markz <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> .....
> > > > After Joerg's patch and yours are merged into linux-next, I can submit
> > > > my drm patch into review process.
> > > 
> > > You can also send them now for early review with some note mentioning
> > > that your patches depend on the above "[PATCH 6/7] iommu/tegra:
> > > Implement DOMAIN_ATTR_GEOMETRY attribute" in the commit log.
> > 
> > Thank you. I merged this patch set locally but seems it has problems:
> > The requirement of the drm driver is: It needs a parameter named "base"
> > while calling function "arm_iommu_create_mapping" to reserve a block of
> > iova from GART/SMMU.
> > This patch set defines function "iommu_domain_get_attr" &
> > "iommu_domain_set_attr", but drm driver can't use them:
> > 1. These 2 functions need a iommu_domain param which means the
> > iommu_domain should be created before calling them. But the iommu_domain
> > is created in function "arm_iommu_create_mapping".
> > 2. It's not a good practice to make drm driver calls iommu apis
> > directly.
> > 
> > Actually, I'm curious that why function "arm_iommu_create_mapping" needs
> > this "base" param. AIUI, drm driver just needs to tell IOMMU the size of
> > the iova address to reserve, then the base address can be checked out in
> > a lot of ways. For example: 
> > 1. Via "dma_map_sg" interface -- dma_map_sg defines we can check out the
> > mapped iova address from "dma_address" member in scatterlist structure.
> > 2. Use this iommu get/set attr interface. Of course, we need some dma
> > mapping api wrappers on them.
> 
> arm_iommu_attach_device() should be called independently from DRM
> driver in advance. In DRM driver, "struct device" should be connected
> to map. Usually this can be done in board files, but with DT, we need
> new framework to deal with this at once. Especially in Tegra, we have
> many IOMMU'able devices, and it's not good idea to do respectively.
> 

arm_iommu_attach_device() needs "struct dma_iommu_mapping *mapping"
which is the return value of arm_iommu_create_mapping(). So we should
call arm_iommu_create_mapping in advance.
"In DRM driver, "struct device" should be connected to map". What does
this "map" means? How "struct device" connects to map? Why we need this?

> For temporary solution, you can call arm_iommu_create_mapping() and
> arm_iommu_attach_device() in advance before DRM uses DMA API.

Yes, this is what I do right now.