Tegra-DRM: Tegra30: DMA mapping API (was: About dma mapping apis)

Tegra-DRM: Tegra30: DMA mapping API (was: About dma mapping apis)

[Hiroshi Doyu]

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

> 2. I took a quick glance of tegra-smmu.c, I noticed that there is a
> dts property named dma-window, which it's value is: <0x0
> 0x40000000>. Does this means the iova which SMMU handles is from 0 -
> 1G? IIRC, SMMU handles 4G address space...

It tries to avoid overwrapping 1-1 iovirt-phys mapping, which is
necessary for some drivers right now.

> 3. Right now in my drm driver, arm_iommu_attach_device failed with
> an error code: -22(Invalid Argument). I think this is caused by
> these codes in function smmu_iommu_attach_dev:
> 
> map = (unsigned long)dev->platform_data;
>         if (!map)
>                 return -EINVAL;
> 
> So, does this mean the drm_device should have a "platform_data"(right now it is NULL so -EINVAL returned)? 
> I skimmed the codes of tegra-smmu.c, seems this "platform_data" has specific usage and should be composed in some rules.
> Could you explain a little bit about this?

You can skip that check with the following patch.

From 42a03ff88ed133db613389d2cc4c4ad33cd5d7e3 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Tue, 28 Feb 2012 15:33:17 +0200
Subject: [PATCH 1/1] iommu/tegra: smmu: Enable all SWGRP by default

This should be revisited along with successors, which would
introduce different configurations.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index bf33c03..3bc5c91 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -38,6 +38,8 @@
 #include <mach/smmu.h>
 #include <mach/tegra_smmu.h>
 
+#define SKIP_SWGRP_CHECK /* FIXME: SWGRP should be passed from DT. */
+
 /* bitmap of the page sizes currently supported */
 #define SMMU_IOMMU_PGSIZES	(SZ_4K)
 
@@ -316,11 +318,15 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
+#if !defined(SKIP_SWGRP_CHECK)
 			if (WARN_ON(val & mask))
 				goto err_hw_busy;
+#endif
 			val |= mask;
 		} else {
+#if !defined(SKIP_SWGRP_CHECK)
 			WARN_ON((val & mask) == mask);
+#endif
 			val &= ~mask;
 		}
 		smmu_write(smmu, val, offs);
@@ -701,9 +707,15 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 		return -ENOMEM;
 	client->dev = dev;
 	client->as = as;
+
+#ifdef SKIP_SWGRP_CHECK
+	/* Enable all SWGRP blindly by default */
+	map = (1 << HWGRP_COUNT) - 1;
+#else
 	map = (unsigned long)dev->platform_data;
 	if (!map)
 		return -EINVAL;
+#endif
 
 	err = smmu_client_enable_hwgrp(client, map);
 	if (err)
-- 
1.7.5.4

Re: Tegra-DRM: Tegra30: DMA mapping API (was: About dma mapping apis)

[markz]

Thank you. I'll give it a try.

Mark
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
> 
> > 2. I took a quick glance of tegra-smmu.c, I noticed that there is a
> > dts property named dma-window, which it's value is: <0x0
> > 0x40000000>. Does this means the iova which SMMU handles is from 0 -
> > 1G? IIRC, SMMU handles 4G address space...
> 
> It tries to avoid overwrapping 1-1 iovirt-phys mapping, which is
> necessary for some drivers right now.
> 
> > 3. Right now in my drm driver, arm_iommu_attach_device failed with
> > an error code: -22(Invalid Argument). I think this is caused by
> > these codes in function smmu_iommu_attach_dev:
> > 
> > map = (unsigned long)dev->platform_data;
> >         if (!map)
> >                 return -EINVAL;
> > 
> > So, does this mean the drm_device should have a "platform_data"(right now it is NULL so -EINVAL returned)? 
> > I skimmed the codes of tegra-smmu.c, seems this "platform_data" has specific usage and should be composed in some rules.
> > Could you explain a little bit about this?
> 
> You can skip that check with the following patch.
> 
> From 42a03ff88ed133db613389d2cc4c4ad33cd5d7e3 Mon Sep 17 00:00:00 2001
> From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 28 Feb 2012 15:33:17 +0200
> Subject: [PATCH 1/1] iommu/tegra: smmu: Enable all SWGRP by default
> 
> This should be revisited along with successors, which would
> introduce different configurations.
> 
> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index bf33c03..3bc5c91 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -38,6 +38,8 @@
>  #include <mach/smmu.h>
>  #include <mach/tegra_smmu.h>
>  
> +#define SKIP_SWGRP_CHECK /* FIXME: SWGRP should be passed from DT. */
> +
>  /* bitmap of the page sizes currently supported */
>  #define SMMU_IOMMU_PGSIZES	(SZ_4K)
>  
> @@ -316,11 +318,15 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
>  		offs = HWGRP_ASID_REG(i);
>  		val = smmu_read(smmu, offs);
>  		if (on) {
> +#if !defined(SKIP_SWGRP_CHECK)
>  			if (WARN_ON(val & mask))
>  				goto err_hw_busy;
> +#endif
>  			val |= mask;
>  		} else {
> +#if !defined(SKIP_SWGRP_CHECK)
>  			WARN_ON((val & mask) == mask);
> +#endif
>  			val &= ~mask;
>  		}
>  		smmu_write(smmu, val, offs);
> @@ -701,9 +707,15 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
>  		return -ENOMEM;
>  	client->dev = dev;
>  	client->as = as;
> +
> +#ifdef SKIP_SWGRP_CHECK
> +	/* Enable all SWGRP blindly by default */
> +	map = (1 << HWGRP_COUNT) - 1;
> +#else
>  	map = (unsigned long)dev->platform_data;
>  	if (!map)
>  		return -EINVAL;
> +#endif
>  
>  	err = smmu_client_enable_hwgrp(client, map);
>  	if (err)

Re: Tegra-DRM: Tegra30: DMA mapping API (was: About dma mapping apis)

[markz]

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?

> > 2. I took a quick glance of tegra-smmu.c, I noticed that there is a
> > dts property named dma-window, which it's value is: <0x0
> > 0x40000000>. Does this means the iova which SMMU handles is from 0 -
> > 1G? IIRC, SMMU handles 4G address space...
> 
> It tries to avoid overwrapping 1-1 iovirt-phys mapping, which is
> necessary for some drivers right now.
> 

Sorry I didn't get it. What is overwrapping 1-1 iovirt-phys mapping?

> > 3. Right now in my drm driver, arm_iommu_attach_device failed with
> > an error code: -22(Invalid Argument). I think this is caused by
> > these codes in function smmu_iommu_attach_dev:
> > 
> > map = (unsigned long)dev->platform_data;
> >         if (!map)
> >                 return -EINVAL;
> > 
> > So, does this mean the drm_device should have a "platform_data"(right now it is NULL so -EINVAL returned)? 
> > I skimmed the codes of tegra-smmu.c, seems this "platform_data" has specific usage and should be composed in some rules.
> > Could you explain a little bit about this?
> 
> You can skip that check with the following patch.
> 

Yes, it works. Right now the framebuffer works on my T30.
Thank you!

> From 42a03ff88ed133db613389d2cc4c4ad33cd5d7e3 Mon Sep 17 00:00:00 2001
> From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 28 Feb 2012 15:33:17 +0200
> Subject: [PATCH 1/1] iommu/tegra: smmu: Enable all SWGRP by default
> 
> This should be revisited along with successors, which would
> introduce different configurations.
> 
> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index bf33c03..3bc5c91 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -38,6 +38,8 @@
>  #include <mach/smmu.h>
>  #include <mach/tegra_smmu.h>
>  
> +#define SKIP_SWGRP_CHECK /* FIXME: SWGRP should be passed from DT. */
> +
>  /* bitmap of the page sizes currently supported */
>  #define SMMU_IOMMU_PGSIZES	(SZ_4K)
>  
> @@ -316,11 +318,15 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
>  		offs = HWGRP_ASID_REG(i);
>  		val = smmu_read(smmu, offs);
>  		if (on) {
> +#if !defined(SKIP_SWGRP_CHECK)
>  			if (WARN_ON(val & mask))
>  				goto err_hw_busy;
> +#endif
>  			val |= mask;
>  		} else {
> +#if !defined(SKIP_SWGRP_CHECK)
>  			WARN_ON((val & mask) == mask);
> +#endif
>  			val &= ~mask;
>  		}
>  		smmu_write(smmu, val, offs);
> @@ -701,9 +707,15 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
>  		return -ENOMEM;
>  	client->dev = dev;
>  	client->as = as;
> +
> +#ifdef SKIP_SWGRP_CHECK
> +	/* Enable all SWGRP blindly by default */
> +	map = (1 << HWGRP_COUNT) - 1;
> +#else
>  	map = (unsigned long)dev->platform_data;
>  	if (!map)
>  		return -EINVAL;
> +#endif
>  
>  	err = smmu_client_enable_hwgrp(client, map);
>  	if (err)

Re: Tegra-DRM: Tegra30: DMA mapping API (was: About dma mapping apis)

[Hiroshi Doyu]

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
 
> > > 2. I took a quick glance of tegra-smmu.c, I noticed that there is a
> > > dts property named dma-window, which it's value is: <0x0
> > > 0x40000000>. Does this means the iova which SMMU handles is from 0 -
> > > 1G? IIRC, SMMU handles 4G address space...
> > 
> > It tries to avoid overwrapping 1-1 iovirt-phys mapping, which is
> > necessary for some drivers right now.
> > 
> 
> Sorry I didn't get it. What is overwrapping 1-1 iovirt-phys mapping?

To create the mapping,
Physical Address 0x12345678 == IO virtual Address 0x12345678
Then, even if device drivers are not aware of IOMMU, it should work.

> > > 3. Right now in my drm driver, arm_iommu_attach_device failed with
> > > an error code: -22(Invalid Argument). I think this is caused by
> > > these codes in function smmu_iommu_attach_dev:
> > > 
> > > map = (unsigned long)dev->platform_data;
> > >         if (!map)
> > >                 return -EINVAL;
> > > 
> > > So, does this mean the drm_device should have a "platform_data"(right now it is NULL so -EINVAL returned)? 
> > > I skimmed the codes of tegra-smmu.c, seems this "platform_data" has specific usage and should be composed in some rules.
> > > Could you explain a little bit about this?
> > 
> > You can skip that check with the following patch.
> > 
> Yes, it works. Right now the framebuffer works on my T30.

Great!

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.