| Message ID | 20241217071852.2261858-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/xe/vsec: enforce CONFIG_INTEL_VSEC dependency | expand |
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 >
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
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
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
On Tue, Dec 17, 2024, at 21:14, Lucas De Marchi wrote: > 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. "depends on INTEL_VSEC" enforces that hard dependency. The difference with "depends on INTEL_VSEC || !INTEL_VSEC" is that it also allows XE to be either built-in or a module if INTEL_VSEC is turned off, as it would be the case on non-x86. Arnd
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