diff mbox series

[12/18] iommu/tegra-gart: Add IOMMU_DOMAIN_DMA support

Message ID 516b33118d489e56499ff8c64c019709b744110c.1597931876.git.robin.murphy@arm.com (mailing list archive)
State Not Applicable, archived
Headers
Series Convert arch/arm to use iommu-dma |

Commit Message

Robin Murphy Aug. 20, 2020, 3:08 p.m. UTC 
  Now that arch/arm is wired up for default domains and iommu-dma,
implement the corresponding driver-side support for DMA domains.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-gart.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Dmitry Osipenko Aug. 20, 2020, 8:16 p.m. UTC | #1 
20.08.2020 18:08, Robin Murphy пишет:
> Now that arch/arm is wired up for default domains and iommu-dma,
> implement the corresponding driver-side support for DMA domains.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/tegra-gart.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index fac720273889..e081387080f6 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -9,6 +9,7 @@
>  
>  #define dev_fmt(fmt)	"gart: " fmt
>  
> +#include <linux/dma-iommu.h>
>  #include <linux/io.h>
>  #include <linux/iommu.h>
>  #include <linux/moduleparam.h>
> @@ -145,16 +146,22 @@ static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
>  {
>  	struct iommu_domain *domain;

Hello, Robin!

Tegra20 GART isn't a real IOMMU, but a small relocation aperture. We
would only want to use it for a temporal mappings (managed by GPU
driver) for the time while GPU hardware is busy and working with a
sparse DMA buffers, the driver will take care of unmapping the sparse
buffers once GPU work is finished [1]. In a case of contiguous DMA
buffers, we want to bypass the IOMMU and use buffer's phys address
because GART aperture is small and all buffers simply can't fit into
GART for a complex GPU operations that involve multiple buffers [2][3].
The upstream GPU driver still doesn't support GART, but eventually it
needs to be changed.

[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L489

[2]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L542

[3]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/uapi/patching.c#L90

> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>  		return NULL;

Will a returned NULL tell to IOMMU core that implicit domain shouldn't
be used? Is it possible to leave this driver as-is?
Robin Murphy Aug. 21, 2020, 12:28 a.m. UTC | #2 
On 2020-08-20 21:16, Dmitry Osipenko wrote:
> 20.08.2020 18:08, Robin Murphy пишет:
>> Now that arch/arm is wired up for default domains and iommu-dma,
>> implement the corresponding driver-side support for DMA domains.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/tegra-gart.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
>> index fac720273889..e081387080f6 100644
>> --- a/drivers/iommu/tegra-gart.c
>> +++ b/drivers/iommu/tegra-gart.c
>> @@ -9,6 +9,7 @@
>>   
>>   #define dev_fmt(fmt)	"gart: " fmt
>>   
>> +#include <linux/dma-iommu.h>
>>   #include <linux/io.h>
>>   #include <linux/iommu.h>
>>   #include <linux/moduleparam.h>
>> @@ -145,16 +146,22 @@ static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
>>   {
>>   	struct iommu_domain *domain;
> 
> Hello, Robin!
> 
> Tegra20 GART isn't a real IOMMU, but a small relocation aperture. We
> would only want to use it for a temporal mappings (managed by GPU
> driver) for the time while GPU hardware is busy and working with a
> sparse DMA buffers, the driver will take care of unmapping the sparse
> buffers once GPU work is finished [1]. In a case of contiguous DMA
> buffers, we want to bypass the IOMMU and use buffer's phys address
> because GART aperture is small and all buffers simply can't fit into
> GART for a complex GPU operations that involve multiple buffers [2][3].
> The upstream GPU driver still doesn't support GART, but eventually it
> needs to be changed.
> 
> [1]
> https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L489
> 
> [2]
> https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L542
> 
> [3]
> https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/uapi/patching.c#L90
> 
>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
>> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>>   		return NULL;
> 
> Will a returned NULL tell to IOMMU core that implicit domain shouldn't
> be used? Is it possible to leave this driver as-is?

The aim of this patch was just to make the conversion without functional 
changes wherever possible, i.e. maintain an equivalent to the existing 
ARM behaviour of allocating its own implicit domains for everything. It 
doesn't represent any judgement of whether that was ever appropriate for 
this driver in the first place ;)

Hopefully my other reply already covered the degree of control drivers 
can have with proper default domains, but do shout if anything wasn't clear.

Cheers,
Robin.
Dmitry Osipenko Aug. 23, 2020, 9:42 p.m. UTC | #3 
21.08.2020 03:28, Robin Murphy пишет:
...
>> Will a returned NULL tell to IOMMU core that implicit domain shouldn't
>> be used? Is it possible to leave this driver as-is?
> 
> The aim of this patch was just to make the conversion without functional
> changes wherever possible, i.e. maintain an equivalent to the existing
> ARM behaviour of allocating its own implicit domains for everything. It
> doesn't represent any judgement of whether that was ever appropriate for
> this driver in the first place ;)
> 
> Hopefully my other reply already covered the degree of control drivers
> can have with proper default domains, but do shout if anything wasn't
> clear.

Thank you for the detailed comments! I wasn't watching closely all the
recent iommu/ changes and yours clarification is very helpful!

My current understanding is that the GART driver will need to support
the IOMMU_DOMAIN_IDENTITY and set def_domain_type to
IOMMU_DOMAIN_IDENTITY for all devices.

Meanwhile, today's upstream drivers don't use GART, hence this patch
should be okay. Although, it's a bit unlikely that the IOMMU_DOMAIN_DMA
type will ever be useful for the GART, and thus, I'm still thinking that
will be a bit nicer to keep GART driver as-is for now.
diff mbox series

Patch

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index fac720273889..e081387080f6 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -9,6 +9,7 @@ 
 
 #define dev_fmt(fmt)	"gart: " fmt
 
+#include <linux/dma-iommu.h>
 #include <linux/io.h>
 #include <linux/iommu.h>
 #include <linux/moduleparam.h>
@@ -145,16 +146,22 @@  static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
 	struct iommu_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (domain) {
-		domain->geometry.aperture_start = gart_handle->iovmm_base;
-		domain->geometry.aperture_end = gart_handle->iovmm_end - 1;
-		domain->geometry.force_aperture = true;
+	if (!domain)
+		return NULL;
+
+	if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
+		kfree(domain);
+		return NULL;
 	}
 
+	domain->geometry.aperture_start = gart_handle->iovmm_base;
+	domain->geometry.aperture_end = gart_handle->iovmm_end - 1;
+	domain->geometry.force_aperture = true;
+
 	return domain;
 }