Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
@ 2019-08-26 13:31 YueHaibing
  2019-08-26 15:18 ` Dmitry Osipenko
  2019-08-29 11:39 ` Hans Verkuil
  0 siblings, 2 replies; 9+ messages in thread
From: YueHaibing @ 2019-08-26 13:31 UTC (permalink / raw)
  To: digetx, mchehab, gregkh, thierry.reding, jonathanh, robin.murphy,
	hverkuil-cisco
  Cc: linux-media, linux-tegra, devel, linux-kernel, iommu, YueHaibing

If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
to m will set IOMMU_IOVA to m, this fails the building of
TEGRA_HOST1X and DRM_TEGRA which is y like this:

drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
cdma.c:(.text+0x698): undefined reference to `__free_iova'

drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/staging/media/tegra-vde/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
index ba49ea5..a41d30c 100644
--- a/drivers/staging/media/tegra-vde/Kconfig
+++ b/drivers/staging/media/tegra-vde/Kconfig
@@ -1,9 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 config TEGRA_VDE
 	tristate "NVIDIA Tegra Video Decoder Engine driver"
-	depends on ARCH_TEGRA || COMPILE_TEST
+	depends on ARCH_TEGRA
 	select DMA_SHARED_BUFFER
-	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
+	select IOMMU_IOVA if IOMMU_SUPPORT
 	select SRAM
 	help
 	    Say Y here to enable support for the NVIDIA Tegra video decoder
-- 
2.7.4



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-26 13:31 [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST YueHaibing
@ 2019-08-26 15:18 ` Dmitry Osipenko
  2019-08-26 15:28   ` Dmitry Osipenko
  2019-08-29 11:39 ` Hans Verkuil
  1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Osipenko @ 2019-08-26 15:18 UTC (permalink / raw)
  To: YueHaibing, mchehab, gregkh, thierry.reding, jonathanh,
	robin.murphy, hverkuil-cisco
  Cc: linux-media, linux-tegra, devel, linux-kernel, iommu

Hello Yue,

26.08.2019 16:31, YueHaibing пишет:
> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> to m will set IOMMU_IOVA to m, this fails the building of
> TEGRA_HOST1X and DRM_TEGRA which is y like this:
> 
> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> cdma.c:(.text+0x698): undefined reference to `__free_iova'
> 
> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
> index ba49ea5..a41d30c 100644
> --- a/drivers/staging/media/tegra-vde/Kconfig
> +++ b/drivers/staging/media/tegra-vde/Kconfig
> @@ -1,9 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  config TEGRA_VDE
>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
> -	depends on ARCH_TEGRA || COMPILE_TEST
> +	depends on ARCH_TEGRA
>  	select DMA_SHARED_BUFFER
> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> +	select IOMMU_IOVA if IOMMU_SUPPORT
>  	select SRAM
>  	help
>  	    Say Y here to enable support for the NVIDIA Tegra video decoder
> 

What about this variant:

	select IOMMU_IOVA if (IOMMU_SUPPORT && !COMPILE_TEST)

which should fix the building and preserve compile-testing.

It shouldn't matter at all whether IOVA is enabled or not for
compile-testing of the driver.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-26 15:18 ` Dmitry Osipenko
@ 2019-08-26 15:28   ` Dmitry Osipenko
  0 siblings, 0 replies; 9+ messages in thread
From: Dmitry Osipenko @ 2019-08-26 15:28 UTC (permalink / raw)
  To: YueHaibing, mchehab, gregkh, thierry.reding, jonathanh,
	robin.murphy, hverkuil-cisco
  Cc: linux-media, linux-tegra, devel, linux-kernel, iommu

26.08.2019 18:18, Dmitry Osipenko пишет:
> Hello Yue,
> 
> 26.08.2019 16:31, YueHaibing пишет:
>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
>> to m will set IOMMU_IOVA to m, this fails the building of
>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
>>
>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
>>
>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
>> index ba49ea5..a41d30c 100644
>> --- a/drivers/staging/media/tegra-vde/Kconfig
>> +++ b/drivers/staging/media/tegra-vde/Kconfig
>> @@ -1,9 +1,9 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  config TEGRA_VDE
>>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
>> -	depends on ARCH_TEGRA || COMPILE_TEST
>> +	depends on ARCH_TEGRA
>>  	select DMA_SHARED_BUFFER
>> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
>> +	select IOMMU_IOVA if IOMMU_SUPPORT
>>  	select SRAM
>>  	help
>>  	    Say Y here to enable support for the NVIDIA Tegra video decoder
>>
> 
> What about this variant:
> 
> 	select IOMMU_IOVA if (IOMMU_SUPPORT && !COMPILE_TEST)
> 
> which should fix the building and preserve compile-testing.
> 
> It shouldn't matter at all whether IOVA is enabled or not for
> compile-testing of the driver.
> 

Ah, actually this won't work if TEGRA_VDE=y and IOMMU_IOVA=m. I'm still
not sure that disabling compile-testing is a good solution, maybe
DRM_TEGRA and TEGRA_HOST1X should be fixed instead. Although, I'm fine
with both variants.

Acked-by: Dmitry Osipenko <digetx@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-26 13:31 [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST YueHaibing
  2019-08-26 15:18 ` Dmitry Osipenko
@ 2019-08-29 11:39 ` Hans Verkuil
  2019-08-29 12:40   ` Thierry Reding
  1 sibling, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2019-08-29 11:39 UTC (permalink / raw)
  To: YueHaibing, digetx, mchehab, gregkh, thierry.reding, jonathanh,
	robin.murphy
  Cc: linux-media, linux-tegra, devel, linux-kernel, iommu

On 8/26/19 3:31 PM, YueHaibing wrote:
> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> to m will set IOMMU_IOVA to m, this fails the building of
> TEGRA_HOST1X and DRM_TEGRA which is y like this:
> 
> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> cdma.c:(.text+0x698): undefined reference to `__free_iova'
> 
> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
> index ba49ea5..a41d30c 100644
> --- a/drivers/staging/media/tegra-vde/Kconfig
> +++ b/drivers/staging/media/tegra-vde/Kconfig
> @@ -1,9 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  config TEGRA_VDE
>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
> -	depends on ARCH_TEGRA || COMPILE_TEST
> +	depends on ARCH_TEGRA

What happens if you drop this change,

>  	select DMA_SHARED_BUFFER
> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> +	select IOMMU_IOVA if IOMMU_SUPPORT

but keep this change?

iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
work when compile testing this tegra-vde driver.

Haven't tried it, but making sure that compile testing keep working is
really important.

Regards,

	Hans

>  	select SRAM
>  	help
>  	    Say Y here to enable support for the NVIDIA Tegra video decoder
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-29 11:39 ` Hans Verkuil
@ 2019-08-29 12:40   ` Thierry Reding
  2019-08-29 13:58     ` Dmitry Osipenko
  0 siblings, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2019-08-29 12:40 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: YueHaibing, digetx, mchehab, gregkh, jonathanh, robin.murphy,
	linux-media, linux-tegra, devel, linux-kernel, iommu

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

On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
> On 8/26/19 3:31 PM, YueHaibing wrote:
> > If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> > to m will set IOMMU_IOVA to m, this fails the building of
> > TEGRA_HOST1X and DRM_TEGRA which is y like this:
> > 
> > drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> > cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> > cdma.c:(.text+0x698): undefined reference to `__free_iova'
> > 
> > drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> > drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> > drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> > 
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> > Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> >  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
> > index ba49ea5..a41d30c 100644
> > --- a/drivers/staging/media/tegra-vde/Kconfig
> > +++ b/drivers/staging/media/tegra-vde/Kconfig
> > @@ -1,9 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  config TEGRA_VDE
> >  	tristate "NVIDIA Tegra Video Decoder Engine driver"
> > -	depends on ARCH_TEGRA || COMPILE_TEST
> > +	depends on ARCH_TEGRA
> 
> What happens if you drop this change,
> 
> >  	select DMA_SHARED_BUFFER
> > -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> > +	select IOMMU_IOVA if IOMMU_SUPPORT
> 
> but keep this change?
> 
> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
> work when compile testing this tegra-vde driver.
> 
> Haven't tried it, but making sure that compile testing keep working is
> really important.

Yeah, that variant seems to work for me. I think it's also more correct
because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
then IOVA is not needed either, so the dummies will do just fine.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-29 12:40   ` Thierry Reding
@ 2019-08-29 13:58     ` Dmitry Osipenko
  2019-08-29 15:49       ` Thierry Reding
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Osipenko @ 2019-08-29 13:58 UTC (permalink / raw)
  To: Thierry Reding, Hans Verkuil, YueHaibing
  Cc: mchehab, gregkh, jonathanh, robin.murphy, linux-media,
	linux-tegra, devel, linux-kernel, iommu

29.08.2019 15:40, Thierry Reding пишет:
> On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
>> On 8/26/19 3:31 PM, YueHaibing wrote:
>>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
>>> to m will set IOMMU_IOVA to m, this fails the building of
>>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
>>>
>>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
>>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
>>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
>>>
>>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
>>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
>>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
>>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
>>> index ba49ea5..a41d30c 100644
>>> --- a/drivers/staging/media/tegra-vde/Kconfig
>>> +++ b/drivers/staging/media/tegra-vde/Kconfig
>>> @@ -1,9 +1,9 @@
>>>  # SPDX-License-Identifier: GPL-2.0
>>>  config TEGRA_VDE
>>>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
>>> -	depends on ARCH_TEGRA || COMPILE_TEST
>>> +	depends on ARCH_TEGRA
>>
>> What happens if you drop this change,
>>
>>>  	select DMA_SHARED_BUFFER
>>> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
>>> +	select IOMMU_IOVA if IOMMU_SUPPORT
>>
>> but keep this change?
>>
>> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
>> work when compile testing this tegra-vde driver.
>>
>> Haven't tried it, but making sure that compile testing keep working is
>> really important.

The driver's code compilation works okay, it's the linkage stage which
fails during compile-testing.

> Yeah, that variant seems to work for me. I think it's also more correct
> because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
> IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
> then IOVA is not needed either, so the dummies will do just fine.

Am I understanding correctly that you're suggesting to revert [1][2] and
get back to the other problem?

[1]
https://lore.kernel.org/linux-media/dd547b44-7abb-371f-aeee-a82b96f824e2@gmail.com/T/
[2] https://patchwork.ozlabs.org/patch/1136619/

If we want to keep compile testing, I guess the only reasonable variant
right now is to select IOMMU_IOVA unconditionally in all of the drivers
(vde, host1x, drm and etc) and then just ignore that IOVA will be
compiled-and-unused if IOMMU_SUPPORT=n (note that IOMMU_SUPPORT=y in all
of default kernel configurations).

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-29 13:58     ` Dmitry Osipenko
@ 2019-08-29 15:49       ` Thierry Reding
  2019-09-06 13:38         ` Dmitry Osipenko
  0 siblings, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2019-08-29 15:49 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Hans Verkuil, YueHaibing, mchehab, gregkh, jonathanh,
	robin.murphy, linux-media, linux-tegra, devel, linux-kernel,
	iommu

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

On Thu, Aug 29, 2019 at 04:58:22PM +0300, Dmitry Osipenko wrote:
> 29.08.2019 15:40, Thierry Reding пишет:
> > On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
> >> On 8/26/19 3:31 PM, YueHaibing wrote:
> >>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> >>> to m will set IOMMU_IOVA to m, this fails the building of
> >>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
> >>>
> >>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> >>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> >>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
> >>>
> >>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> >>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> >>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> >>>
> >>> Reported-by: Hulk Robot <hulkci@huawei.com>
> >>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> >>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
> >>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> >>> ---
> >>>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
> >>> index ba49ea5..a41d30c 100644
> >>> --- a/drivers/staging/media/tegra-vde/Kconfig
> >>> +++ b/drivers/staging/media/tegra-vde/Kconfig
> >>> @@ -1,9 +1,9 @@
> >>>  # SPDX-License-Identifier: GPL-2.0
> >>>  config TEGRA_VDE
> >>>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
> >>> -	depends on ARCH_TEGRA || COMPILE_TEST
> >>> +	depends on ARCH_TEGRA
> >>
> >> What happens if you drop this change,
> >>
> >>>  	select DMA_SHARED_BUFFER
> >>> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> >>> +	select IOMMU_IOVA if IOMMU_SUPPORT
> >>
> >> but keep this change?
> >>
> >> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
> >> work when compile testing this tegra-vde driver.
> >>
> >> Haven't tried it, but making sure that compile testing keep working is
> >> really important.
> 
> The driver's code compilation works okay, it's the linkage stage which
> fails during compile-testing.
> 
> > Yeah, that variant seems to work for me. I think it's also more correct
> > because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
> > IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
> > then IOVA is not needed either, so the dummies will do just fine.
> 
> Am I understanding correctly that you're suggesting to revert [1][2] and
> get back to the other problem?
> 
> [1]
> https://lore.kernel.org/linux-media/dd547b44-7abb-371f-aeee-a82b96f824e2@gmail.com/T/
> [2] https://patchwork.ozlabs.org/patch/1136619/
> 
> If we want to keep compile testing, I guess the only reasonable variant
> right now is to select IOMMU_IOVA unconditionally in all of the drivers
> (vde, host1x, drm and etc) and then just ignore that IOVA will be
> compiled-and-unused if IOMMU_SUPPORT=n (note that IOMMU_SUPPORT=y in all
> of default kernel configurations).

Agreed. I think we should just select IOMMU_IOVA unconditionally. We
really do want IOMMU_SUPPORT always as well, but it might be nice to be
able to switch it off for testing or so. In the cases that really matter
we will be enabling both IOMMU_SUPPORT and IOMMU_IOVA anyway, so might
as well select IOMMU_IOVA always. It's not terribly big and I can't
imagine anyone wanting to run a kernel without IOMMU_SUPPORT for
anything other than testing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-08-29 15:49       ` Thierry Reding
@ 2019-09-06 13:38         ` Dmitry Osipenko
  2019-09-06 14:28           ` Yuehaibing
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Osipenko @ 2019-09-06 13:38 UTC (permalink / raw)
  To: Thierry Reding, Hans Verkuil, YueHaibing
  Cc: mchehab, gregkh, jonathanh, robin.murphy, linux-media,
	linux-tegra, devel, linux-kernel, iommu

29.08.2019 18:49, Thierry Reding пишет:
> On Thu, Aug 29, 2019 at 04:58:22PM +0300, Dmitry Osipenko wrote:
>> 29.08.2019 15:40, Thierry Reding пишет:
>>> On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
>>>> On 8/26/19 3:31 PM, YueHaibing wrote:
>>>>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
>>>>> to m will set IOMMU_IOVA to m, this fails the building of
>>>>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
>>>>>
>>>>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
>>>>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
>>>>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
>>>>>
>>>>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
>>>>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
>>>>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
>>>>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>>> ---
>>>>>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
>>>>> index ba49ea5..a41d30c 100644
>>>>> --- a/drivers/staging/media/tegra-vde/Kconfig
>>>>> +++ b/drivers/staging/media/tegra-vde/Kconfig
>>>>> @@ -1,9 +1,9 @@
>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>>  config TEGRA_VDE
>>>>>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
>>>>> -	depends on ARCH_TEGRA || COMPILE_TEST
>>>>> +	depends on ARCH_TEGRA
>>>>
>>>> What happens if you drop this change,
>>>>
>>>>>  	select DMA_SHARED_BUFFER
>>>>> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
>>>>> +	select IOMMU_IOVA if IOMMU_SUPPORT
>>>>
>>>> but keep this change?
>>>>
>>>> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
>>>> work when compile testing this tegra-vde driver.
>>>>
>>>> Haven't tried it, but making sure that compile testing keep working is
>>>> really important.
>>
>> The driver's code compilation works okay, it's the linkage stage which
>> fails during compile-testing.
>>
>>> Yeah, that variant seems to work for me. I think it's also more correct
>>> because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
>>> IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
>>> then IOVA is not needed either, so the dummies will do just fine.
>>
>> Am I understanding correctly that you're suggesting to revert [1][2] and
>> get back to the other problem?
>>
>> [1]
>> https://lore.kernel.org/linux-media/dd547b44-7abb-371f-aeee-a82b96f824e2@gmail.com/T/
>> [2] https://patchwork.ozlabs.org/patch/1136619/
>>
>> If we want to keep compile testing, I guess the only reasonable variant
>> right now is to select IOMMU_IOVA unconditionally in all of the drivers
>> (vde, host1x, drm and etc) and then just ignore that IOVA will be
>> compiled-and-unused if IOMMU_SUPPORT=n (note that IOMMU_SUPPORT=y in all
>> of default kernel configurations).
> 
> Agreed. I think we should just select IOMMU_IOVA unconditionally. We
> really do want IOMMU_SUPPORT always as well, but it might be nice to be
> able to switch it off for testing or so. In the cases that really matter
> we will be enabling both IOMMU_SUPPORT and IOMMU_IOVA anyway, so might
> as well select IOMMU_IOVA always. It's not terribly big and I can't
> imagine anyone wanting to run a kernel without IOMMU_SUPPORT for
> anything other than testing.

Hello Yue,

Could you please make an updated version of the fix in accordance to the above comments?

Alternatively, we can go with the current patch and temporarily remove the compile-testing. I'll make
patches to properly re-add compile-testing sometime later then.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
  2019-09-06 13:38         ` Dmitry Osipenko
@ 2019-09-06 14:28           ` Yuehaibing
  0 siblings, 0 replies; 9+ messages in thread
From: Yuehaibing @ 2019-09-06 14:28 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding, Hans Verkuil
  Cc: mchehab, gregkh, jonathanh, robin.murphy, linux-media,
	linux-tegra, devel, linux-kernel, iommu



On 2019/9/6 21:38, Dmitry Osipenko wrote:
> 29.08.2019 18:49, Thierry Reding пишет:
>> On Thu, Aug 29, 2019 at 04:58:22PM +0300, Dmitry Osipenko wrote:
>>> 29.08.2019 15:40, Thierry Reding пишет:
>>>> On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
>>>>> On 8/26/19 3:31 PM, YueHaibing wrote:
>>>>>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
>>>>>> to m will set IOMMU_IOVA to m, this fails the building of
>>>>>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
>>>>>>
>>>>>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
>>>>>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
>>>>>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
>>>>>>
>>>>>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
>>>>>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
>>>>>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
>>>>>>
>>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
>>>>>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
>>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>>>>> ---
>>>>>>  drivers/staging/media/tegra-vde/Kconfig | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/media/tegra-vde/Kconfig b/drivers/staging/media/tegra-vde/Kconfig
>>>>>> index ba49ea5..a41d30c 100644
>>>>>> --- a/drivers/staging/media/tegra-vde/Kconfig
>>>>>> +++ b/drivers/staging/media/tegra-vde/Kconfig
>>>>>> @@ -1,9 +1,9 @@
>>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>>>  config TEGRA_VDE
>>>>>>  	tristate "NVIDIA Tegra Video Decoder Engine driver"
>>>>>> -	depends on ARCH_TEGRA || COMPILE_TEST
>>>>>> +	depends on ARCH_TEGRA
>>>>>
>>>>> What happens if you drop this change,
>>>>>
>>>>>>  	select DMA_SHARED_BUFFER
>>>>>> -	select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
>>>>>> +	select IOMMU_IOVA if IOMMU_SUPPORT
>>>>>
>>>>> but keep this change?
>>>>>
>>>>> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
>>>>> work when compile testing this tegra-vde driver.
>>>>>
>>>>> Haven't tried it, but making sure that compile testing keep working is
>>>>> really important.
>>>
>>> The driver's code compilation works okay, it's the linkage stage which
>>> fails during compile-testing.
>>>
>>>> Yeah, that variant seems to work for me. I think it's also more correct
>>>> because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
>>>> IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
>>>> then IOVA is not needed either, so the dummies will do just fine.
>>>
>>> Am I understanding correctly that you're suggesting to revert [1][2] and
>>> get back to the other problem?
>>>
>>> [1]
>>> https://lore.kernel.org/linux-media/dd547b44-7abb-371f-aeee-a82b96f824e2@gmail.com/T/
>>> [2] https://patchwork.ozlabs.org/patch/1136619/
>>>
>>> If we want to keep compile testing, I guess the only reasonable variant
>>> right now is to select IOMMU_IOVA unconditionally in all of the drivers
>>> (vde, host1x, drm and etc) and then just ignore that IOVA will be
>>> compiled-and-unused if IOMMU_SUPPORT=n (note that IOMMU_SUPPORT=y in all
>>> of default kernel configurations).
>>
>> Agreed. I think we should just select IOMMU_IOVA unconditionally. We
>> really do want IOMMU_SUPPORT always as well, but it might be nice to be
>> able to switch it off for testing or so. In the cases that really matter
>> we will be enabling both IOMMU_SUPPORT and IOMMU_IOVA anyway, so might
>> as well select IOMMU_IOVA always. It's not terribly big and I can't
>> imagine anyone wanting to run a kernel without IOMMU_SUPPORT for
>> anything other than testing.
> 
> Hello Yue,
> 
> Could you please make an updated version of the fix in accordance to the above comments?
> 
> Alternatively, we can go with the current patch and temporarily remove the compile-testing. I'll make
> patches to properly re-add compile-testing sometime later then.

I prefer to do this choice, thanks.

> 
> .
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 13:31 [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST YueHaibing
2019-08-26 15:18 ` Dmitry Osipenko
2019-08-26 15:28   ` Dmitry Osipenko
2019-08-29 11:39 ` Hans Verkuil
2019-08-29 12:40   ` Thierry Reding
2019-08-29 13:58     ` Dmitry Osipenko
2019-08-29 15:49       ` Thierry Reding
2019-09-06 13:38         ` Dmitry Osipenko
2019-09-06 14:28           ` Yuehaibing

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox