diff mbox series

[08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

Message ID 20190916150412.10025-9-thierry.reding@gmail.com
State Superseded
Headers show
Series drm/nouveau: Enable GP10B by default | expand

Commit Message

Thierry Reding Sept. 16, 2019, 3:04 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

If the GPU is already attached to an IOMMU, don't detach it and setup an
explicit IOMMU domain. Since Nouveau can now properly handle the case of
the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

Comments

Robin Murphy Sept. 16, 2019, 3:29 p.m. UTC | #1 
Hi Thierry,

On 16/09/2019 16:04, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the GPU is already attached to an IOMMU, don't detach it and setup an
> explicit IOMMU domain. Since Nouveau can now properly handle the case of
> the DMA API being backed by an IOMMU, just continue using the DMA API.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> index d0d52c1d4aee..fc652aaa41c7 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> @@ -23,10 +23,6 @@
>   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
>   #include "priv.h"
>   
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> -#include <asm/dma-iommu.h>
> -#endif
> -
>   static int
>   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
>   {
> @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
>   	unsigned long pgsize_bitmap;
>   	int ret;
>   
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> -	if (dev->archdata.mapping) {
> -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> -
> -		arm_iommu_detach_device(dev);
> -		arm_iommu_release_mapping(mapping);
> -	}
> -#endif
> +	/*
> +	 * Skip explicit IOMMU initialization if the GPU is already attached
> +	 * to an IOMMU domain. This can happen if the DMA API is backed by an
> +	 * IOMMU.
> +	 */
> +	if (iommu_get_domain_for_dev(dev))
> +		return;

Beware of "iommu.passthrough=1" - you could get a valid default domain 
here yet still have direct/SWIOTLB DMA ops. I guess you probably want to 
double-check the domain type as well.

Robin.

>   
>   	if (!tdev->func->iommu_bit)
>   		return;
>
Thierry Reding Sept. 16, 2019, 3:57 p.m. UTC | #2 
On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
> Hi Thierry,
> 
> On 16/09/2019 16:04, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > If the GPU is already attached to an IOMMU, don't detach it and setup an
> > explicit IOMMU domain. Since Nouveau can now properly handle the case of
> > the DMA API being backed by an IOMMU, just continue using the DMA API.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
> >   1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > index d0d52c1d4aee..fc652aaa41c7 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > @@ -23,10 +23,6 @@
> >   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
> >   #include "priv.h"
> > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > -#include <asm/dma-iommu.h>
> > -#endif
> > -
> >   static int
> >   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
> >   {
> > @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
> >   	unsigned long pgsize_bitmap;
> >   	int ret;
> > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > -	if (dev->archdata.mapping) {
> > -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > -
> > -		arm_iommu_detach_device(dev);
> > -		arm_iommu_release_mapping(mapping);
> > -	}
> > -#endif
> > +	/*
> > +	 * Skip explicit IOMMU initialization if the GPU is already attached
> > +	 * to an IOMMU domain. This can happen if the DMA API is backed by an
> > +	 * IOMMU.
> > +	 */
> > +	if (iommu_get_domain_for_dev(dev))
> > +		return;
> 
> Beware of "iommu.passthrough=1" - you could get a valid default domain here
> yet still have direct/SWIOTLB DMA ops. I guess you probably want to
> double-check the domain type as well.

Good point. An earlier version of this patch had an additional check for
IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.

Thierry
Robin Murphy Sept. 16, 2019, 4:15 p.m. UTC | #3 
On 16/09/2019 16:57, Thierry Reding wrote:
> On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
>> Hi Thierry,
>>
>> On 16/09/2019 16:04, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> If the GPU is already attached to an IOMMU, don't detach it and setup an
>>> explicit IOMMU domain. Since Nouveau can now properly handle the case of
>>> the DMA API being backed by an IOMMU, just continue using the DMA API.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>    .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
>>>    1 file changed, 7 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> index d0d52c1d4aee..fc652aaa41c7 100644
>>> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
>>> @@ -23,10 +23,6 @@
>>>    #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
>>>    #include "priv.h"
>>> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
>>> -#include <asm/dma-iommu.h>
>>> -#endif
>>> -
>>>    static int
>>>    nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
>>>    {
>>> @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
>>>    	unsigned long pgsize_bitmap;
>>>    	int ret;
>>> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
>>> -	if (dev->archdata.mapping) {
>>> -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
>>> -
>>> -		arm_iommu_detach_device(dev);
>>> -		arm_iommu_release_mapping(mapping);
>>> -	}
>>> -#endif
>>> +	/*
>>> +	 * Skip explicit IOMMU initialization if the GPU is already attached
>>> +	 * to an IOMMU domain. This can happen if the DMA API is backed by an
>>> +	 * IOMMU.
>>> +	 */
>>> +	if (iommu_get_domain_for_dev(dev))
>>> +		return;
>>
>> Beware of "iommu.passthrough=1" - you could get a valid default domain here
>> yet still have direct/SWIOTLB DMA ops. I guess you probably want to
>> double-check the domain type as well.
> 
> Good point. An earlier version of this patch had an additional check for
> IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
> DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
> IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
> appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.

Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be 
sufficient to answer "is the DMA layer managing my address space for 
me?" unless and until some massive API change happens (which I certainly 
don't foresee).

Robin.
Thierry Reding Sept. 17, 2019, 7:59 a.m. UTC | #4 
On Mon, Sep 16, 2019 at 05:15:25PM +0100, Robin Murphy wrote:
> On 16/09/2019 16:57, Thierry Reding wrote:
> > On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
> > > Hi Thierry,
> > > 
> > > On 16/09/2019 16:04, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > If the GPU is already attached to an IOMMU, don't detach it and setup an
> > > > explicit IOMMU domain. Since Nouveau can now properly handle the case of
> > > > the DMA API being backed by an IOMMU, just continue using the DMA API.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >    .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++------------
> > > >    1 file changed, 7 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > > > index d0d52c1d4aee..fc652aaa41c7 100644
> > > > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > > > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
> > > > @@ -23,10 +23,6 @@
> > > >    #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
> > > >    #include "priv.h"
> > > > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > > > -#include <asm/dma-iommu.h>
> > > > -#endif
> > > > -
> > > >    static int
> > > >    nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
> > > >    {
> > > > @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
> > > >    	unsigned long pgsize_bitmap;
> > > >    	int ret;
> > > > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> > > > -	if (dev->archdata.mapping) {
> > > > -		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > > > -
> > > > -		arm_iommu_detach_device(dev);
> > > > -		arm_iommu_release_mapping(mapping);
> > > > -	}
> > > > -#endif
> > > > +	/*
> > > > +	 * Skip explicit IOMMU initialization if the GPU is already attached
> > > > +	 * to an IOMMU domain. This can happen if the DMA API is backed by an
> > > > +	 * IOMMU.
> > > > +	 */
> > > > +	if (iommu_get_domain_for_dev(dev))
> > > > +		return;
> > > 
> > > Beware of "iommu.passthrough=1" - you could get a valid default domain here
> > > yet still have direct/SWIOTLB DMA ops. I guess you probably want to
> > > double-check the domain type as well.
> > 
> > Good point. An earlier version of this patch had an additional check for
> > IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
> > DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
> > IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
> > appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.
> 
> Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be
> sufficient to answer "is the DMA layer managing my address space for me?"
> unless and until some massive API change happens (which I certainly don't
> foresee).

Might be a good idea to roll that up into a function to have a standard
way for drivers to check for this rather than open-coding the same
condition everywhere (and maybe get things wrong). As an additional
advantage, if that massive API change ever does happen we don't have to
go and update all the callers.

Something like this perhaps?

	static inline bool iommu_managed(struct device *dev)
	{
		struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

		return domain && domain->type != IOMMU_DOMAIN_UNMANAGED;
	}

Thierry
diff mbox series

Patch

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index d0d52c1d4aee..fc652aaa41c7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -23,10 +23,6 @@ 
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
 #include "priv.h"
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
 static int
 nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
 {
@@ -109,14 +105,13 @@  nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
 	unsigned long pgsize_bitmap;
 	int ret;
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
+	/*
+	 * Skip explicit IOMMU initialization if the GPU is already attached
+	 * to an IOMMU domain. This can happen if the DMA API is backed by an
+	 * IOMMU.
+	 */
+	if (iommu_get_domain_for_dev(dev))
+		return;
 
 	if (!tdev->func->iommu_bit)
 		return;