[v1] drm/xe/vsec: fix CONFIG_INTEL_VSEC dependency

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

Posted by Arnd Bergmann 7 months ago

From: Arnd Bergmann <arnd@arndb.de>

The XE driver can be built with or without VSEC support, but fails to link as
built-in if vsec is in a loadable module:

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

The normal fix for this is to add a 'depends on INTEL_VSEC || !INTEL_VSEC',
forcing XE to be a loadable module as well, but that causes a circular
dependency:

        symbol DRM_XE depends on INTEL_VSEC
        symbol INTEL_VSEC depends on X86_PLATFORM_DEVICES
        symbol X86_PLATFORM_DEVICES is selected by DRM_XE

The problem here is selecting a symbol from another subsystem, so change
that as well and rephrase the 'select' into the corresponding dependency.
Since X86_PLATFORM_DEVICES is 'default y', there is no change to
defconfig builds here.

Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/xe/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index beddd153c28c..c870b680431c 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -2,6 +2,8 @@
 config DRM_XE
 	tristate "Intel Xe Graphics"
 	depends on DRM && PCI && (m || (y && KUNIT=y))
+	depends on INTEL_VSEC || !INTEL_VSEC
+	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)
 	select INTERVAL_TREE
 	# we need shmfs for the swappable backing store, and in particular
 	# the shmem_readpage() which depends upon tmpfs
@@ -29,7 +31,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
-- 
2.39.5

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

Posted by Lucas De Marchi 6 months, 3 weeks ago

On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:
>From: Arnd Bergmann <arnd@arndb.de>
>
>The XE driver can be built with or without VSEC support, but fails to link as
>built-in if vsec is in a loadable module:
>
>x86_64-linux-ld: vmlinux.o: in function `xe_vsec_init':
>(.text+0x1e83e16): undefined reference to `intel_vsec_register'
>
>The normal fix for this is to add a 'depends on INTEL_VSEC || !INTEL_VSEC',
>forcing XE to be a loadable module as well, but that causes a circular
>dependency:
>
>        symbol DRM_XE depends on INTEL_VSEC
>        symbol INTEL_VSEC depends on X86_PLATFORM_DEVICES
>        symbol X86_PLATFORM_DEVICES is selected by DRM_XE
>
>The problem here is selecting a symbol from another subsystem, so change
>that as well and rephrase the 'select' into the corresponding dependency.
>Since X86_PLATFORM_DEVICES is 'default y', there is no change to
>defconfig builds here.
>
>Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
> drivers/gpu/drm/xe/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>index beddd153c28c..c870b680431c 100644
>--- a/drivers/gpu/drm/xe/Kconfig
>+++ b/drivers/gpu/drm/xe/Kconfig
>@@ -2,6 +2,8 @@
> config DRM_XE
> 	tristate "Intel Xe Graphics"
> 	depends on DRM && PCI && (m || (y && KUNIT=y))
>+	depends on INTEL_VSEC || !INTEL_VSEC
>+	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)

		   ^
Did you mean X86_PLATFORM_DEVICES here?

With that, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

I see several drivers selecting
X86_PLATFORM_DEVICES though. Maybe they should also be translated to
dependencies instead?

$ git grep "select X86_PLATFORM_DEVICES"       
drivers/gpu/drm/amd/amdgpu/Kconfig:     select X86_PLATFORM_DEVICES if ACPI && X86
drivers/gpu/drm/gma500/Kconfig: select X86_PLATFORM_DEVICES if ACPI               
drivers/gpu/drm/i915/Kconfig:   select X86_PLATFORM_DEVICES if ACPI               
drivers/gpu/drm/nouveau/Kconfig:        select X86_PLATFORM_DEVICES if ACPI && X86
drivers/gpu/drm/radeon/Kconfig: select X86_PLATFORM_DEVICES if ACPI && X86        

thanks
Lucas De Marchi

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

Posted by Andy Shevchenko 6 months, 3 weeks ago

On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
> On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:

...

> > +	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)
> 
> 		   ^
> Did you mean X86_PLATFORM_DEVICES here?

Why do we need to depend on the whole thingy (yes, it will be enabled at the
end) if we only talking about Intel?

> With that, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> I see several drivers selecting
> X86_PLATFORM_DEVICES though. Maybe they should also be translated to
> dependencies instead?

I think so, selecting that sounds wrong.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Arnd Bergmann 6 months, 3 weeks ago

On Wed, May 28, 2025, at 11:34, Andy Shevchenko wrote:
> On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
>> On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:
>
> ...
>
>> > +	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)
>> 
>> 		   ^
>> Did you mean X86_PLATFORM_DEVICES here?

Yes, my mistake.

> Why do we need to depend on the whole thingy (yes, it will be enabled at the
> end) if we only talking about Intel?

I don't understand what you mean with 'the whole thing'. My change
changed the existing 'select X86_PLATFORM_DEVICES if X86 && ACPI'
into the corresponding dependency, in order to change it the
least.

The dependency itself is needed because of

       select ACPI_WMI if X86 && ACPI

and this in turn is needed for

       select ACPI_VIDEO if X86 && ACPI

>> With that, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> 
>> I see several drivers selecting
>> X86_PLATFORM_DEVICES though. Maybe they should also be translated to
>> dependencies instead?
>
> I think so, selecting that sounds wrong.

Agreed. Overall, what I'd really like to see is to remove
all those 'select' of drivers from other subsystems. I think
ACPI_VIDEO is at the center here, and changing all the
'select ACPI_VIDEO if ACPI' instances to
'depends on ACPI_VIDEO || !ACPI_VIDEO' would solve a lot of
the recurring dependency loop problems in drivers/gpu/.

Actually doing it without regressions is going to be
nontrivial though, because any change in this area is likely
to trigger another dependency loop somewhere.

     Arnd

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

Posted by Andy Shevchenko 6 months, 3 weeks ago

On Wed, May 28, 2025 at 12:03:43PM +0200, Arnd Bergmann wrote:
> On Wed, May 28, 2025, at 11:34, Andy Shevchenko wrote:
> > On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
> >> On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:
> >
> > ...
> >
> >> > +	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)
> >> 
> >> 		   ^
> >> Did you mean X86_PLATFORM_DEVICES here?
> 
> Yes, my mistake.
> 
> > Why do we need to depend on the whole thingy (yes, it will be enabled at the
> > end) if we only talking about Intel?
> 
> I don't understand what you mean with 'the whole thing'. My change
> changed the existing 'select X86_PLATFORM_DEVICES if X86 && ACPI'
> into the corresponding dependency, in order to change it the
> least.

It used to be (for only one or two releases) X86_PLATFORM_DRIVERS_INTEL, but it
doesn't look closer to the mistake above, which I was thinking of. So, Lucas is
right.

> The dependency itself is needed because of
> 
>        select ACPI_WMI if X86 && ACPI
> 
> and this in turn is needed for
> 
>        select ACPI_VIDEO if X86 && ACPI
> 
> >> With that, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> 
> >> I see several drivers selecting
> >> X86_PLATFORM_DEVICES though. Maybe they should also be translated to
> >> dependencies instead?
> >
> > I think so, selecting that sounds wrong.
> 
> Agreed. Overall, what I'd really like to see is to remove
> all those 'select' of drivers from other subsystems.

Let's start from some low-hanging fruits?

> I think
> ACPI_VIDEO is at the center here, and changing all the
> 'select ACPI_VIDEO if ACPI' instances to
> 'depends on ACPI_VIDEO || !ACPI_VIDEO' would solve a lot of
> the recurring dependency loop problems in drivers/gpu/.
> 
> Actually doing it without regressions is going to be
> nontrivial though, because any change in this area is likely
> to trigger another dependency loop somewhere.

True and I agree this requires more comprehensive testing.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Christopher Snowhill 6 months, 3 weeks ago

On Wed May 28, 2025 at 3:03 AM PDT, Arnd Bergmann wrote:
> On Wed, May 28, 2025, at 11:34, Andy Shevchenko wrote:
>> On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
>>> On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:
>>
>> ...
>>
>>> > +	depends on INTEL_PLATFORM_DEVICES || !(X86 && ACPI)
>>> 
>>> 		   ^
>>> Did you mean X86_PLATFORM_DEVICES here?
>
> Yes, my mistake.
>
>> Why do we need to depend on the whole thingy (yes, it will be enabled at the
>> end) if we only talking about Intel?
>
> I don't understand what you mean with 'the whole thing'. My change
> changed the existing 'select X86_PLATFORM_DEVICES if X86 && ACPI'
> into the corresponding dependency, in order to change it the
> least.
>
> The dependency itself is needed because of
>
>        select ACPI_WMI if X86 && ACPI
>
> and this in turn is needed for
>
>        select ACPI_VIDEO if X86 && ACPI
>
>>> With that, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> 
>>> I see several drivers selecting
>>> X86_PLATFORM_DEVICES though. Maybe they should also be translated to
>>> dependencies instead?
>>
>> I think so, selecting that sounds wrong.
>
> Agreed. Overall, what I'd really like to see is to remove
> all those 'select' of drivers from other subsystems. I think
> ACPI_VIDEO is at the center here, and changing all the
> 'select ACPI_VIDEO if ACPI' instances to
> 'depends on ACPI_VIDEO || !ACPI_VIDEO' would solve a lot of

Maybe you meant 'depends on ACPI_VIDEO || !ACPI' ?

> the recurring dependency loop problems in drivers/gpu/.
>
> Actually doing it without regressions is going to be
> nontrivial though, because any change in this area is likely
> to trigger another dependency loop somewhere.
>
>      Arnd

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

Posted by Andy Shevchenko 6 months, 3 weeks ago

On Wed, May 28, 2025 at 03:17:03AM -0700, Christopher Snowhill wrote:
> On Wed May 28, 2025 at 3:03 AM PDT, Arnd Bergmann wrote:
> > On Wed, May 28, 2025, at 11:34, Andy Shevchenko wrote:
> >> On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
> >>> On Fri, May 23, 2025 at 02:10:46PM +0200, Arnd Bergmann wrote:

...

> > I think ACPI_VIDEO is at the center here, and changing all the
> > 'select ACPI_VIDEO if ACPI' instances to
> > 'depends on ACPI_VIDEO || !ACPI_VIDEO' would solve a lot of
> 
> Maybe you meant 'depends on ACPI_VIDEO || !ACPI' ?

I believe not. The depends on FOO || FOO=n is idiomatic in Kconfig.

> > the recurring dependency loop problems in drivers/gpu/.
> >
> > Actually doing it without regressions is going to be
> > nontrivial though, because any change in this area is likely
> > to trigger another dependency loop somewhere.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Arnd Bergmann 6 months, 3 weeks ago

On Wed, May 28, 2025, at 12:31, Andy Shevchenko wrote:
> On Wed, May 28, 2025 at 03:17:03AM -0700, Christopher Snowhill wrote:
>> On Wed May 28, 2025 at 3:03 AM PDT, Arnd Bergmann wrote:
>> > On Wed, May 28, 2025, at 11:34, Andy Shevchenko wrote:
>> >> On Tue, May 27, 2025 at 03:55:46PM -0500, Lucas De Marchi wrote:
>> > I think ACPI_VIDEO is at the center here, and changing all the
>> > 'select ACPI_VIDEO if ACPI' instances to
>> > 'depends on ACPI_VIDEO || !ACPI_VIDEO' would solve a lot of
>> 
>> Maybe you meant 'depends on ACPI_VIDEO || !ACPI' ?
>
> I believe not. The depends on FOO || FOO=n is idiomatic in Kconfig.

It depends on what we want here. 'ACPI_VIDEO || !ACPI' would
be the direct equivalent of the existing 'select ACPI_VIDEO if ACPI',
while 'ACPI_VIDEO || !ACPI_VIDEO' would allow building with
ACPI=y and ACPI_VIDEO=n, which is not possible today. I'd probably
start with the version that Christopher suggested to have a lower
regression risk.

     Arnd