[PATCH] drm/nouveau/platform: add IOMMU dependency

[PATCH] drm/nouveau/platform: add IOMMU dependency

[Arnd Bergmann]

The recently added iommu code in the nouveau driver fails to build
when the IOMMU support is disabled:

drivers/gpu/drm/nouveau/nouveau_platform.c: In function 'nouveau_platform_probe_iommu':
drivers/gpu/drm/nouveau/nouveau_platform.c:113:41: error: 'const struct iommu_ops' has no mem

To avoid the build error, this now adds an explicit dependency on the
IOMMU implementation.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 58fd9375c2c5 ("drm/nouveau/platform: probe IOMMU if present")

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 5ab13e7939db..18dfa4af60ea 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -28,6 +28,7 @@ config DRM_NOUVEAU
 config NOUVEAU_PLATFORM_DRIVER
 	bool "Nouveau (NVIDIA) SoC GPUs"
 	depends on DRM_NOUVEAU && ARCH_TEGRA
+	depends on IOMMU
 	default y
 	help
 	  Support for Nouveau platform driver, used for SoC GPUs as found

Re: [PATCH] drm/nouveau/platform: add IOMMU dependency

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Tue, May 19, 2015 at 02:53:26PM +0200, Arnd Bergmann wrote:
> The recently added iommu code in the nouveau driver fails to build
> when the IOMMU support is disabled:
> 
> drivers/gpu/drm/nouveau/nouveau_platform.c: In function 'nouveau_platform_probe_iommu':
> drivers/gpu/drm/nouveau/nouveau_platform.c:113:41: error: 'const struct iommu_ops' has no mem
> 
> To avoid the build error, this now adds an explicit dependency on the
> IOMMU implementation.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 58fd9375c2c5 ("drm/nouveau/platform: probe IOMMU if present")

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm/nouveau/platform: add IOMMU dependency

[Alexandre Courbot]

On 05/19/2015 09:53 PM, Arnd Bergmann wrote:
> The recently added iommu code in the nouveau driver fails to build
> when the IOMMU support is disabled:
>
> drivers/gpu/drm/nouveau/nouveau_platform.c: In function 'nouveau_platform_probe_iommu':
> drivers/gpu/drm/nouveau/nouveau_platform.c:113:41: error: 'const struct iommu_ops' has no mem
>
> To avoid the build error, this now adds an explicit dependency on the
> IOMMU implementation.

I have a local patch to nouveau_platform.c that only calls the IOMMU 
functions if CONFIG_IOMMU is set. Wouldn't this be more suitable as 
IOMMU support is only used by Tegra and thus not beneficial for desktop 
GPUs?

[PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Alexandre Courbot]

The lack of IOMMU API support can make nouveau_platform_probe_iommu()
fail to compile because struct iommu_ops is then empty. Fix this by
skipping IOMMU probe in that case - lack of IOMMU on platform devices
is sub-optimal, but is not an error.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
This is an alternative to https://lkml.org/lkml/2015/5/19/484. Most users
of Nouveau do not care about IOMMU support, so we should not impose that
option on them.

 drm/nouveau/nouveau_platform.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c b/drivers/gpu/drm/nouveau/nouveau_platform.c
index 775277f1edb0..dcfbbfaf1739 100644
--- a/drivers/gpu/drm/nouveau/nouveau_platform.c
+++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
@@ -92,6 +92,8 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_IOMMU_API)
+
 static void nouveau_platform_probe_iommu(struct device *dev,
 					 struct nouveau_platform_gpu *gpu)
 {
@@ -158,6 +160,20 @@ static void nouveau_platform_remove_iommu(struct device *dev,
 	}
 }
 
+#else
+
+static void nouveau_platform_probe_iommu(struct device *dev,
+					 struct nouveau_platform_gpu *gpu)
+{
+}
+
+static void nouveau_platform_remove_iommu(struct device *dev,
+					  struct nouveau_platform_gpu *gpu)
+{
+}
+
+#endif
+
 static int nouveau_platform_probe(struct platform_device *pdev)
 {
 	struct nouveau_platform_gpu *gpu;
-- 
2.4.0

Re: [PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Arnd Bergmann]

On Wednesday 20 May 2015 15:10:24 Alexandre Courbot wrote:
> The lack of IOMMU API support can make nouveau_platform_probe_iommu()
> fail to compile because struct iommu_ops is then empty. Fix this by
> skipping IOMMU probe in that case - lack of IOMMU on platform devices
> is sub-optimal, but is not an error.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> This is an alternative to https://lkml.org/lkml/2015/5/19/484. Most users
> of Nouveau do not care about IOMMU support, so we should not impose that
> option on them.
> 

Yes, good idea.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Alexandre Courbot]

On Wed, May 20, 2015 at 4:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 May 2015 15:10:24 Alexandre Courbot wrote:
>> The lack of IOMMU API support can make nouveau_platform_probe_iommu()
>> fail to compile because struct iommu_ops is then empty. Fix this by
>> skipping IOMMU probe in that case - lack of IOMMU on platform devices
>> is sub-optimal, but is not an error.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> This is an alternative to https://lkml.org/lkml/2015/5/19/484. Most users
>> of Nouveau do not care about IOMMU support, so we should not impose that
>> option on them.
>>
>
> Yes, good idea.
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks. Dave, are you ok with this patch? If so, can you take it?

Re: [PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

On Wed, May 20, 2015 at 03:10:24PM +0900, Alexandre Courbot wrote:
> The lack of IOMMU API support can make nouveau_platform_probe_iommu()
> fail to compile because struct iommu_ops is then empty. Fix this by
> skipping IOMMU probe in that case - lack of IOMMU on platform devices
> is sub-optimal, but is not an error.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> This is an alternative to https://lkml.org/lkml/2015/5/19/484. Most users
> of Nouveau do not care about IOMMU support, so we should not impose that
> option on them.
> 
>  drm/nouveau/nouveau_platform.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c b/drivers/gpu/drm/nouveau/nouveau_platform.c
> index 775277f1edb0..dcfbbfaf1739 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_platform.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
> @@ -92,6 +92,8 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_IOMMU_API)
> +
>  static void nouveau_platform_probe_iommu(struct device *dev,
>  					 struct nouveau_platform_gpu *gpu)
>  {
> @@ -158,6 +160,20 @@ static void nouveau_platform_remove_iommu(struct device *dev,
>  	}
>  }
>  
> +#else
> +
> +static void nouveau_platform_probe_iommu(struct device *dev,
> +					 struct nouveau_platform_gpu *gpu)
> +{
> +}
> +
> +static void nouveau_platform_remove_iommu(struct device *dev,
> +					  struct nouveau_platform_gpu *gpu)
> +{
> +}
> +
> +#endif
> +

Since these are all static functions, perhaps an "if (IS_ENABLED(...))"
would work here? That way you'd get compile coverage of the code in all
cases.

But perhaps that doesn't work for IOMMU. I have a vague memory of
running across something like this before and IOMMU has this quirk of
defining struct iommu_ops as empty if IOMMU_API is deselected so you'll
probably get compiler errors unless you actually preprocess the code
out.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Arnd Bergmann]

On Wednesday 20 May 2015 13:32:33 Thierry Reding wrote:
> 
> Since these are all static functions, perhaps an "if (IS_ENABLED(...))"
> would work here? That way you'd get compile coverage of the code in all
> cases.

I had the same thought at first.
 
> But perhaps that doesn't work for IOMMU. I have a vague memory of
> running across something like this before and IOMMU has this quirk of
> defining struct iommu_ops as empty if IOMMU_API is deselected so you'll
> probably get compiler errors unless you actually preprocess the code
> out.

Exactly.

	Arnd

Re: [PATCH] drm/nouveau/platform: fix compilation if !CONFIG_IOMMU

[Alexandre Courbot]

On Wed, May 20, 2015 at 9:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 May 2015 13:32:33 Thierry Reding wrote:
>>
>> Since these are all static functions, perhaps an "if (IS_ENABLED(...))"
>> would work here? That way you'd get compile coverage of the code in all
>> cases.
>
> I had the same thought at first.
>
>> But perhaps that doesn't work for IOMMU. I have a vague memory of
>> running across something like this before and IOMMU has this quirk of
>> defining struct iommu_ops as empty if IOMMU_API is deselected so you'll
>> probably get compiler errors unless you actually preprocess the code
>> out.
>
> Exactly.

That's precisely the issue here, so not covering this code is exactly
what we want if !CONFIG_IOMMU.