drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

[PATCH] drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

Posted by Arnd Bergmann 1 year, 1 month ago

From: Arnd Bergmann <arnd@arndb.de>

When INTEL_VSEC is in a loadable module, XE cannot be built-in any more:

x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
(.text+0x19861bf): undefined reference to `intel_vsec_register'

This could be enforced using a 'depends on INTEL_VSEC || !INTEL_VSEC'
style dependency to allow building with VSEC completely disabled.
My impression here is that this was not actually intended, and that
continuing to support that combination would lead to more build bugs.

Instead, make it a hard dependency as all other INTEL_VSEC users are,
and remove the inline stub alternative. This leads to a dependency
on CONFIG_X86_PLATFORM_DEVICES, so the 'select' has to be removed
to avoid a circular dependency.

Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/xe/Kconfig | 2 +-
 include/linux/intel_vsec.h | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 6c5b665d9384..217b51468497 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -2,6 +2,7 @@
 config DRM_XE
 	tristate "Intel Xe Graphics"
 	depends on DRM && PCI && MMU && (m || (y && KUNIT=y))
+	depends on INTEL_VSEC
 	select INTERVAL_TREE
 	# we need shmfs for the swappable backing store, and in particular
 	# the shmem_readpage() which depends upon tmpfs
@@ -28,7 +29,6 @@ config DRM_XE
 	select INPUT if ACPI
 	select ACPI_VIDEO if X86 && ACPI
 	select ACPI_BUTTON if ACPI
-	select X86_PLATFORM_DEVICES if X86 && ACPI
 	select ACPI_WMI if X86 && ACPI
 	select SYNC_FILE
 	select IOSF_MBI
diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
index b94beab64610..f2d55e686476 100644
--- a/include/linux/intel_vsec.h
+++ b/include/linux/intel_vsec.h
@@ -138,13 +138,6 @@ static inline struct intel_vsec_device *auxdev_to_ivdev(struct auxiliary_device
 	return container_of(auxdev, struct intel_vsec_device, auxdev);
 }
 
-#if IS_ENABLED(CONFIG_INTEL_VSEC)
 void intel_vsec_register(struct pci_dev *pdev,
 			 struct intel_vsec_platform_info *info);
-#else
-static inline void intel_vsec_register(struct pci_dev *pdev,
-				       struct intel_vsec_platform_info *info)
-{
-}
-#endif
 #endif
-- 
2.39.5

Re: [PATCH] drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

Posted by Rodrigo Vivi 1 year, 1 month ago

On Tue, Dec 17, 2024 at 08:18:44AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When INTEL_VSEC is in a loadable module, XE cannot be built-in any more:
> 
> x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
> (.text+0x19861bf): undefined reference to `intel_vsec_register'
> 
> This could be enforced using a 'depends on INTEL_VSEC || !INTEL_VSEC'
> style dependency to allow building with VSEC completely disabled.
> My impression here is that this was not actually intended, and that
> continuing to support that combination would lead to more build bugs.
> 
> Instead, make it a hard dependency as all other INTEL_VSEC users are,
> and remove the inline stub alternative. This leads to a dependency
> on CONFIG_X86_PLATFORM_DEVICES, so the 'select' has to be removed
> to avoid a circular dependency.
> 

I really don't want us to hard lock this X86 dependency here.
What if we add a new DRM_XE_DGFX_PMT_SUPPORT and that
depends on INTEL_VSEC ?

and our if statement changes to
if (IS_ENABLED(DRM_XE_DGFX_PMT_SUPPORT)

We could even leave this enabled by default, but at least
it is an easy path to someone willing to run experiments
without depending on X86 I believe...

Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>

> Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/xe/Kconfig | 2 +-
>  include/linux/intel_vsec.h | 7 -------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> index 6c5b665d9384..217b51468497 100644
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -2,6 +2,7 @@
>  config DRM_XE
>  	tristate "Intel Xe Graphics"
>  	depends on DRM && PCI && MMU && (m || (y && KUNIT=y))
> +	depends on INTEL_VSEC
>  	select INTERVAL_TREE
>  	# we need shmfs for the swappable backing store, and in particular
>  	# the shmem_readpage() which depends upon tmpfs
> @@ -28,7 +29,6 @@ config DRM_XE
>  	select INPUT if ACPI
>  	select ACPI_VIDEO if X86 && ACPI
>  	select ACPI_BUTTON if ACPI
> -	select X86_PLATFORM_DEVICES if X86 && ACPI
>  	select ACPI_WMI if X86 && ACPI
>  	select SYNC_FILE
>  	select IOSF_MBI
> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
> index b94beab64610..f2d55e686476 100644
> --- a/include/linux/intel_vsec.h
> +++ b/include/linux/intel_vsec.h
> @@ -138,13 +138,6 @@ static inline struct intel_vsec_device *auxdev_to_ivdev(struct auxiliary_device
>  	return container_of(auxdev, struct intel_vsec_device, auxdev);
>  }
>  
> -#if IS_ENABLED(CONFIG_INTEL_VSEC)
>  void intel_vsec_register(struct pci_dev *pdev,
>  			 struct intel_vsec_platform_info *info);
> -#else
> -static inline void intel_vsec_register(struct pci_dev *pdev,
> -				       struct intel_vsec_platform_info *info)
> -{
> -}
> -#endif
>  #endif
> -- 
> 2.39.5
>

Re: [PATCH] drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

Posted by Arnd Bergmann 1 year, 1 month ago

On Tue, Dec 17, 2024, at 19:52, Rodrigo Vivi wrote:
> On Tue, Dec 17, 2024 at 08:18:44AM +0100, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> When INTEL_VSEC is in a loadable module, XE cannot be built-in any more:
>> 
>> x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
>> (.text+0x19861bf): undefined reference to `intel_vsec_register'
>> 
>> This could be enforced using a 'depends on INTEL_VSEC || !INTEL_VSEC'
>> style dependency to allow building with VSEC completely disabled.
>> My impression here is that this was not actually intended, and that
>> continuing to support that combination would lead to more build bugs.
>> 
>> Instead, make it a hard dependency as all other INTEL_VSEC users are,
>> and remove the inline stub alternative. This leads to a dependency
>> on CONFIG_X86_PLATFORM_DEVICES, so the 'select' has to be removed
>> to avoid a circular dependency.
>> 
>
> I really don't want us to hard lock this X86 dependency here.
> What if we add a new DRM_XE_DGFX_PMT_SUPPORT and that
> depends on INTEL_VSEC ?

Yes, that should work if it gets phrased correctly.
Something like

config DRM_XE_DGFX_PMT_SUPPORT
       bool "X86 PMT support"
       depends on DRM_XE && INTEL_VSEC
       depends on DRM_XE=m || INTEL_VSEC=y
       depends on X86 || COMPILE_TEST



     Arnd

Re: [PATCH] drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

Posted by Lucas De Marchi 1 year, 1 month ago

On Tue, Dec 17, 2024 at 08:28:59PM +0100, Arnd Bergmann wrote:
>On Tue, Dec 17, 2024, at 19:52, Rodrigo Vivi wrote:
>> On Tue, Dec 17, 2024 at 08:18:44AM +0100, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> When INTEL_VSEC is in a loadable module, XE cannot be built-in any more:
>>>
>>> x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
>>> (.text+0x19861bf): undefined reference to `intel_vsec_register'
>>>
>>> This could be enforced using a 'depends on INTEL_VSEC || !INTEL_VSEC'
>>> style dependency to allow building with VSEC completely disabled.
>>> My impression here is that this was not actually intended, and that
>>> continuing to support that combination would lead to more build bugs.

why? if xe is built-in, vsec needs to be built-in as well. If xe is a
module, vsec can be either built-in or a module.

>>>
>>> Instead, make it a hard dependency as all other INTEL_VSEC users are,
>>> and remove the inline stub alternative. This leads to a dependency
>>> on CONFIG_X86_PLATFORM_DEVICES, so the 'select' has to be removed
>>> to avoid a circular dependency.
>>>
>>
>> I really don't want us to hard lock this X86 dependency here.
>> What if we add a new DRM_XE_DGFX_PMT_SUPPORT and that
>> depends on INTEL_VSEC ?
>
>Yes, that should work if it gets phrased correctly.
>Something like
>
>config DRM_XE_DGFX_PMT_SUPPORT
>       bool "X86 PMT support"
>       depends on DRM_XE && INTEL_VSEC
>       depends on DRM_XE=m || INTEL_VSEC=y
>       depends on X86 || COMPILE_TEST


that looks good to me.

thanks
Lucas De Marchi

Re: [PATCH] drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency

Posted by Rodrigo Vivi 1 year, 1 month ago

On Tue, Dec 17, 2024 at 08:28:59PM +0100, Arnd Bergmann wrote:
> On Tue, Dec 17, 2024, at 19:52, Rodrigo Vivi wrote:
> > On Tue, Dec 17, 2024 at 08:18:44AM +0100, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> When INTEL_VSEC is in a loadable module, XE cannot be built-in any more:
> >> 
> >> x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
> >> (.text+0x19861bf): undefined reference to `intel_vsec_register'
> >> 
> >> This could be enforced using a 'depends on INTEL_VSEC || !INTEL_VSEC'
> >> style dependency to allow building with VSEC completely disabled.
> >> My impression here is that this was not actually intended, and that
> >> continuing to support that combination would lead to more build bugs.
> >> 
> >> Instead, make it a hard dependency as all other INTEL_VSEC users are,
> >> and remove the inline stub alternative. This leads to a dependency
> >> on CONFIG_X86_PLATFORM_DEVICES, so the 'select' has to be removed
> >> to avoid a circular dependency.
> >> 
> >
> > I really don't want us to hard lock this X86 dependency here.
> > What if we add a new DRM_XE_DGFX_PMT_SUPPORT and that
> > depends on INTEL_VSEC ?
> 
> Yes, that should work if it gets phrased correctly.
> Something like
> 
> config DRM_XE_DGFX_PMT_SUPPORT
>        bool "X86 PMT support"

I'd say bool "Enable PMT support for Intel DGFX"

the X86 PMT sounds more the cpu package pmt which is enabled out of Xe scope

hmm, I'm thinking we shouldn't also add
depends on CONFIG_INTEL_PMT_TELEMETRY

Dave, thoughts?

Cc: David E. Box <david.e.box@linux.intel.com>

>        depends on DRM_XE && INTEL_VSEC
>        depends on DRM_XE=m || INTEL_VSEC=y
>        depends on X86 || COMPILE_TEST

and also
	default y

> 
> 
> 
>      Arnd