| Message ID | 20190722134749.21580-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: ipu3: Enable IOVA API only when IOMMU support is enabled | expand |
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,
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, >
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, >> > > . >
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.
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.
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.
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,
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(-)