media: staging: ipu3: Enable IOVA API only when IOMMU support is enabled
diff mbox series

Message ID 20190722134749.21580-1-yuehaibing@huawei.com
State New
Headers show
Series
  • media: staging: ipu3: Enable IOVA API only when IOMMU support is enabled
Related show

Commit Message

YueHaibing July 22, 2019, 1:47 p.m. UTC
If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
is m, then the building fails like this:

drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
iommu.c:(.text+0x41): undefined reference to `alloc_iova'
iommu.c:(.text+0x56): undefined reference to `__free_iova'

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/staging/media/ipu3/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sakari Ailus July 24, 2019, 10:30 a.m. UTC | #1
Hi Yue,

On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
> is m, then the building fails like this:
> 
> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
> iommu.c:(.text+0x56): undefined reference to `__free_iova'
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/staging/media/ipu3/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
> index 4b51c67..b7df18f 100644
> --- a/drivers/staging/media/ipu3/Kconfig
> +++ b/drivers/staging/media/ipu3/Kconfig
> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>  	depends on PCI && VIDEO_V4L2
>  	depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>  	depends on X86
> -	select IOMMU_IOVA
> +	select IOMMU_IOVA if IOMMU_SUPPORT

This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
independently of IOMMU_SUPPORT.

Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
declared in its Kconfig entry. I wonder if adding that would be the right
way to fix this.

Cc'ing the IOMMU list.

>  	select VIDEOBUF2_DMA_SG
>  	help
>  	  This is the Video4Linux2 driver for Intel IPU3 image processing unit,
Robin Murphy July 24, 2019, 1:49 p.m. UTC | #2
On 24/07/2019 11:30, Sakari Ailus wrote:
> Hi Yue,
> 
> On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
>> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
>> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
>> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
>> is m, then the building fails like this:
>>
>> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
>> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
>> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>   drivers/staging/media/ipu3/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
>> index 4b51c67..b7df18f 100644
>> --- a/drivers/staging/media/ipu3/Kconfig
>> +++ b/drivers/staging/media/ipu3/Kconfig
>> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>>   	depends on PCI && VIDEO_V4L2
>>   	depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>>   	depends on X86
>> -	select IOMMU_IOVA
>> +	select IOMMU_IOVA if IOMMU_SUPPORT
> 
> This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
> independently of IOMMU_SUPPORT.
> 
> Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
> declared in its Kconfig entry. I wonder if adding that would be the right
> way to fix this.
> 
> Cc'ing the IOMMU list.

Right, I also had the impression that we'd made the IOVA library 
completely standalone. And what does the IPU3 driver's Kconfig have to 
do with some *other* driver failing to link anyway?

Robin.

> 
>>   	select VIDEOBUF2_DMA_SG
>>   	help
>>   	  This is the Video4Linux2 driver for Intel IPU3 image processing unit,
>
YueHaibing July 24, 2019, 2:03 p.m. UTC | #3
On 2019/7/24 21:49, Robin Murphy wrote:
> On 24/07/2019 11:30, Sakari Ailus wrote:
>> Hi Yue,
>>
>> On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
>>> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
>>> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
>>> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
>>> is m, then the building fails like this:
>>>
>>> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
>>> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
>>> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>>   drivers/staging/media/ipu3/Kconfig | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
>>> index 4b51c67..b7df18f 100644
>>> --- a/drivers/staging/media/ipu3/Kconfig
>>> +++ b/drivers/staging/media/ipu3/Kconfig
>>> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>>>       depends on PCI && VIDEO_V4L2
>>>       depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>>>       depends on X86
>>> -    select IOMMU_IOVA
>>> +    select IOMMU_IOVA if IOMMU_SUPPORT
>>
>> This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
>> independently of IOMMU_SUPPORT.
>>
>> Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
>> declared in its Kconfig entry. I wonder if adding that would be the right
>> way to fix this.
>>
>> Cc'ing the IOMMU list.
> 
> Right, I also had the impression that we'd made the IOVA library completely standalone. And what does the IPU3 driver's Kconfig have to do with some *other* driver failing to link anyway?

Oh, I misunderstand that IOMMU_IOVA is depend on IOMMU_SUPPORT, thank you for clarification.

I will try to fix this in tegra-vde.

> 
> Robin.
> 
>>
>>>       select VIDEOBUF2_DMA_SG
>>>       help
>>>         This is the Video4Linux2 driver for Intel IPU3 image processing unit,
>>
> 
> .
>
Dmitry Osipenko July 24, 2019, 2:09 p.m. UTC | #4
24.07.2019 17:03, Yuehaibing пишет:
> On 2019/7/24 21:49, Robin Murphy wrote:
>> On 24/07/2019 11:30, Sakari Ailus wrote:
>>> Hi Yue,
>>>
>>> On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
>>>> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
>>>> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
>>>> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
>>>> is m, then the building fails like this:
>>>>
>>>> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
>>>> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
>>>> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>> ---
>>>>   drivers/staging/media/ipu3/Kconfig | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
>>>> index 4b51c67..b7df18f 100644
>>>> --- a/drivers/staging/media/ipu3/Kconfig
>>>> +++ b/drivers/staging/media/ipu3/Kconfig
>>>> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>>>>       depends on PCI && VIDEO_V4L2
>>>>       depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>>>>       depends on X86
>>>> -    select IOMMU_IOVA
>>>> +    select IOMMU_IOVA if IOMMU_SUPPORT
>>>
>>> This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
>>> independently of IOMMU_SUPPORT.
>>>
>>> Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
>>> declared in its Kconfig entry. I wonder if adding that would be the right
>>> way to fix this.
>>>
>>> Cc'ing the IOMMU list.
IOMMU_SUPPORT is optional for the Tegra-VDE driver.

>> Right, I also had the impression that we'd made the IOVA library completely standalone. And what does the IPU3 driver's Kconfig have to do with some *other* driver failing to link anyway?

I can see it failing if IPU3 is compiled as a loadable module, while
Tegra-VDE is a built-in driver. Hence IOVA lib should be also a kernel
module and thus the IOVA symbols will be missing during of linkage of
the VDE driver.

> Oh, I misunderstand that IOMMU_IOVA is depend on IOMMU_SUPPORT, thank you for clarification.
> 
> I will try to fix this in tegra-vde.

Probably IOVA could be selected independently of IOMMU_SUPPORT, but IOVA
library isn't needed for the VDE driver if IOMMU_SUPPORT is disabled.
Robin Murphy July 24, 2019, 2:23 p.m. UTC | #5
On 24/07/2019 15:09, Dmitry Osipenko wrote:
> 24.07.2019 17:03, Yuehaibing пишет:
>> On 2019/7/24 21:49, Robin Murphy wrote:
>>> On 24/07/2019 11:30, Sakari Ailus wrote:
>>>> Hi Yue,
>>>>
>>>> On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
>>>>> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
>>>>> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
>>>>> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
>>>>> is m, then the building fails like this:
>>>>>
>>>>> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
>>>>> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
>>>>> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device driver")
>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>>> ---
>>>>>    drivers/staging/media/ipu3/Kconfig | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
>>>>> index 4b51c67..b7df18f 100644
>>>>> --- a/drivers/staging/media/ipu3/Kconfig
>>>>> +++ b/drivers/staging/media/ipu3/Kconfig
>>>>> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>>>>>        depends on PCI && VIDEO_V4L2
>>>>>        depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>>>>>        depends on X86
>>>>> -    select IOMMU_IOVA
>>>>> +    select IOMMU_IOVA if IOMMU_SUPPORT
>>>>
>>>> This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
>>>> independently of IOMMU_SUPPORT.
>>>>
>>>> Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
>>>> declared in its Kconfig entry. I wonder if adding that would be the right
>>>> way to fix this.
>>>>
>>>> Cc'ing the IOMMU list.
> IOMMU_SUPPORT is optional for the Tegra-VDE driver.
> 
>>> Right, I also had the impression that we'd made the IOVA library completely standalone. And what does the IPU3 driver's Kconfig have to do with some *other* driver failing to link anyway?
> 
> I can see it failing if IPU3 is compiled as a loadable module, while
> Tegra-VDE is a built-in driver. Hence IOVA lib should be also a kernel
> module and thus the IOVA symbols will be missing during of linkage of
> the VDE driver.
> 
>> Oh, I misunderstand that IOMMU_IOVA is depend on IOMMU_SUPPORT, thank you for clarification.
>>
>> I will try to fix this in tegra-vde.
> 
> Probably IOVA could be selected independently of IOMMU_SUPPORT, but IOVA
> library isn't needed for the VDE driver if IOMMU_SUPPORT is disabled.

Oh, I think I get the problem now - tegra-vde/iommu.c is built 
unconditionally and relies on the static inline stubs for IOMMU and IOVA 
calls if !IOMMU_SUPPORT, but in a compile-test config where IOVA=m for 
other reasons, it then picks up the real declarations from linux/iova.h 
instead of the stubs, and things go downhill from there. So there is a 
real issue, but indeed it's Tegra-VDE which needs to be restructured to 
cope with such configurations, and not IPU3's (or anyone else who may 
select IOVA=m in future) job to work around it.

Robin.
Dmitry Osipenko July 24, 2019, 2:28 p.m. UTC | #6
24.07.2019 17:23, Robin Murphy пишет:
> On 24/07/2019 15:09, Dmitry Osipenko wrote:
>> 24.07.2019 17:03, Yuehaibing пишет:
>>> On 2019/7/24 21:49, Robin Murphy wrote:
>>>> On 24/07/2019 11:30, Sakari Ailus wrote:
>>>>> Hi Yue,
>>>>>
>>>>> On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
>>>>>> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
>>>>>> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
>>>>>> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but
>>>>>> IOMMU_IOVA
>>>>>> is m, then the building fails like this:
>>>>>>
>>>>>> drivers/staging/media/tegra-vde/iommu.o: In function
>>>>>> `tegra_vde_iommu_map':
>>>>>> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
>>>>>> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>>>>>>
>>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>>> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top
>>>>>> level pci device driver")
>>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>>>> ---
>>>>>>    drivers/staging/media/ipu3/Kconfig | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/media/ipu3/Kconfig
>>>>>> b/drivers/staging/media/ipu3/Kconfig
>>>>>> index 4b51c67..b7df18f 100644
>>>>>> --- a/drivers/staging/media/ipu3/Kconfig
>>>>>> +++ b/drivers/staging/media/ipu3/Kconfig
>>>>>> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
>>>>>>        depends on PCI && VIDEO_V4L2
>>>>>>        depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
>>>>>>        depends on X86
>>>>>> -    select IOMMU_IOVA
>>>>>> +    select IOMMU_IOVA if IOMMU_SUPPORT
>>>>>
>>>>> This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
>>>>> independently of IOMMU_SUPPORT.
>>>>>
>>>>> Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but
>>>>> that's not
>>>>> declared in its Kconfig entry. I wonder if adding that would be the
>>>>> right
>>>>> way to fix this.
>>>>>
>>>>> Cc'ing the IOMMU list.
>> IOMMU_SUPPORT is optional for the Tegra-VDE driver.
>>
>>>> Right, I also had the impression that we'd made the IOVA library
>>>> completely standalone. And what does the IPU3 driver's Kconfig have
>>>> to do with some *other* driver failing to link anyway?
>>
>> I can see it failing if IPU3 is compiled as a loadable module, while
>> Tegra-VDE is a built-in driver. Hence IOVA lib should be also a kernel
>> module and thus the IOVA symbols will be missing during of linkage of
>> the VDE driver.
>>
>>> Oh, I misunderstand that IOMMU_IOVA is depend on IOMMU_SUPPORT, thank
>>> you for clarification.
>>>
>>> I will try to fix this in tegra-vde.
>>
>> Probably IOVA could be selected independently of IOMMU_SUPPORT, but IOVA
>> library isn't needed for the VDE driver if IOMMU_SUPPORT is disabled.
> 
> Oh, I think I get the problem now - tegra-vde/iommu.c is built
> unconditionally and relies on the static inline stubs for IOMMU and IOVA
> calls if !IOMMU_SUPPORT, but in a compile-test config where IOVA=m for
> other reasons, it then picks up the real declarations from linux/iova.h
> instead of the stubs, and things go downhill from there. So there is a
> real issue, but indeed it's Tegra-VDE which needs to be restructured to
> cope with such configurations, and not IPU3's (or anyone else who may
> select IOVA=m in future) job to work around it.

I guess it could be:

	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)

as a workaround.

Patch
diff mbox series

diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
index 4b51c67..b7df18f 100644
--- a/drivers/staging/media/ipu3/Kconfig
+++ b/drivers/staging/media/ipu3/Kconfig
@@ -4,7 +4,7 @@  config VIDEO_IPU3_IMGU
 	depends on PCI && VIDEO_V4L2
 	depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
 	depends on X86
-	select IOMMU_IOVA
+	select IOMMU_IOVA if IOMMU_SUPPORT
 	select VIDEOBUF2_DMA_SG
 	help
 	  This is the Video4Linux2 driver for Intel IPU3 image processing unit,