[PATCH] PCI/VGA: Select SCREEN_INFO

[PATCH] PCI/VGA: Select SCREEN_INFO

[Mario Limonciello (AMD)]

commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
a screen info check") introduced an implicit dependency upon SCREEN_INFO
by removing the open coded implementation.

If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
would now return false.  Add a select for SCREEN_INFO to ensure that the
VGA arbiter works as intended.

Reported-by: Eric Biggers <ebiggers@kernel.org>
Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with a screen info check")
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
 drivers/pci/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 7065a8e5f9b14..c35fed47addd5 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -306,6 +306,7 @@ config VGA_ARB
 	bool "VGA Arbitration" if EXPERT
 	default y
 	depends on (PCI && !S390)
+	select SCREEN_INFO
 	help
 	  Some "legacy" VGA devices implemented on PCI typically have the same
 	  hard-decoded addresses as they did on ISA. When multiple PCI devices
-- 
2.43.0

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Ilpo Järvinen]

On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:

> commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> a screen info check") introduced an implicit dependency upon SCREEN_INFO
> by removing the open coded implementation.
> 
> If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
> would now return false.  Add a select for SCREEN_INFO to ensure that the
> VGA arbiter works as intended.
> 
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with a screen info check")
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>  drivers/pci/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 7065a8e5f9b14..c35fed47addd5 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -306,6 +306,7 @@ config VGA_ARB
>  	bool "VGA Arbitration" if EXPERT
>  	default y
>  	depends on (PCI && !S390)
> +	select SCREEN_INFO
>  	help
>  	  Some "legacy" VGA devices implemented on PCI typically have the same
>  	  hard-decoded addresses as they did on ISA. When multiple PCI devices

The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around the
call, but that now becomes superfluous with this select, no?

Looking into the history of the ifdefs here is quite odd pattern (only 
the last one comes with some explanation but even that is on the vague 
side and fails to remove the actual now unnecessary ifdef :-/):

#if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO

Was it intentional to allow building without CONFIG_SCREEN_INFO?

-- 
 i.

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Mario Limonciello]

On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
> On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
> 
>> commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
>> a screen info check") introduced an implicit dependency upon SCREEN_INFO
>> by removing the open coded implementation.
>>
>> If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
>> would now return false.  Add a select for SCREEN_INFO to ensure that the
>> VGA arbiter works as intended.
>>
>> Reported-by: Eric Biggers <ebiggers@kernel.org>
>> Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with a screen info check")
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>>   drivers/pci/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 7065a8e5f9b14..c35fed47addd5 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -306,6 +306,7 @@ config VGA_ARB
>>   	bool "VGA Arbitration" if EXPERT
>>   	default y
>>   	depends on (PCI && !S390)
>> +	select SCREEN_INFO
>>   	help
>>   	  Some "legacy" VGA devices implemented on PCI typically have the same
>>   	  hard-decoded addresses as they did on ISA. When multiple PCI devices
> 
> The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around the
> call, but that now becomes superfluous with this select, no?

Thanks! Will adjust.

> 
> Looking into the history of the ifdefs here is quite odd pattern (only
> the last one comes with some explanation but even that is on the vague
> side and fails to remove the actual now unnecessary ifdef :-/):
> 
> #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
> 
> Was it intentional to allow building without CONFIG_SCREEN_INFO?
> 

You mean in the VGA arbiter code?  Or just in general?  There is a lot 
of other places that conditionalize code on CONFIG_SCREEN_INFO.  You 
don't "have" to build in the the VGA arbiter and presumably those are 
correct.

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Ilpo Järvinen]

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

On Mon, 13 Oct 2025, Mario Limonciello wrote:

> On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
> > On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
> > 
> > > commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > > a screen info check") introduced an implicit dependency upon SCREEN_INFO
> > > by removing the open coded implementation.
> > > 
> > > If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
> > > would now return false.  Add a select for SCREEN_INFO to ensure that the
> > > VGA arbiter works as intended.
> > > 
> > > Reported-by: Eric Biggers <ebiggers@kernel.org>
> > > Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
> > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with a
> > > screen info check")
> > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > ---
> > >   drivers/pci/Kconfig | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > > index 7065a8e5f9b14..c35fed47addd5 100644
> > > --- a/drivers/pci/Kconfig
> > > +++ b/drivers/pci/Kconfig
> > > @@ -306,6 +306,7 @@ config VGA_ARB
> > >   	bool "VGA Arbitration" if EXPERT
> > >   	default y
> > >   	depends on (PCI && !S390)
> > > +	select SCREEN_INFO
> > >   	help
> > >   	  Some "legacy" VGA devices implemented on PCI typically have the same
> > >   	  hard-decoded addresses as they did on ISA. When multiple PCI devices
> > 
> > The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around the
> > call, but that now becomes superfluous with this select, no?
> 
> Thanks! Will adjust.
> 
> > 
> > Looking into the history of the ifdefs here is quite odd pattern (only
> > the last one comes with some explanation but even that is on the vague
> > side and fails to remove the actual now unnecessary ifdef :-/):
> > 
> > #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
> > 
> > Was it intentional to allow building without CONFIG_SCREEN_INFO?
> > 
> 
> You mean in the VGA arbiter code?  Or just in general?

Here in the VGA arbiter. I'm just trying to understand why the #else part 
was here post-337bf13aa9dda.

> There is a lot of
> other places that conditionalize code on CONFIG_SCREEN_INFO.  You don't "have"
> to build in the the VGA arbiter and presumably those are correct.
> 

-- 
 i.

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Thomas Zimmermann]

Hi

Am 13.10.25 um 16:35 schrieb Ilpo Järvinen:
> On Mon, 13 Oct 2025, Mario Limonciello wrote:
>
>> On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
>>> On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
>>>
>>>> commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
>>>> a screen info check") introduced an implicit dependency upon SCREEN_INFO
>>>> by removing the open coded implementation.
>>>>
>>>> If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
>>>> would now return false.  Add a select for SCREEN_INFO to ensure that the
>>>> VGA arbiter works as intended.
>>>>
>>>> Reported-by: Eric Biggers <ebiggers@kernel.org>
>>>> Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with a
>>>> screen info check")
>>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> ---
>>>>    drivers/pci/Kconfig | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>>> index 7065a8e5f9b14..c35fed47addd5 100644
>>>> --- a/drivers/pci/Kconfig
>>>> +++ b/drivers/pci/Kconfig
>>>> @@ -306,6 +306,7 @@ config VGA_ARB
>>>>    	bool "VGA Arbitration" if EXPERT
>>>>    	default y
>>>>    	depends on (PCI && !S390)
>>>> +	select SCREEN_INFO
>>>>    	help
>>>>    	  Some "legacy" VGA devices implemented on PCI typically have the same
>>>>    	  hard-decoded addresses as they did on ISA. When multiple PCI devices
>>> The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
>>> a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around the
>>> call, but that now becomes superfluous with this select, no?
>> Thanks! Will adjust.

Yes, thanks. You can no longer run into this #else.

>>
>>> Looking into the history of the ifdefs here is quite odd pattern (only
>>> the last one comes with some explanation but even that is on the vague
>>> side and fails to remove the actual now unnecessary ifdef :-/):
>>>
>>> #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
>>>
>>> Was it intentional to allow building without CONFIG_SCREEN_INFO?
>>>
>> You mean in the VGA arbiter code?  Or just in general?
> Here in the VGA arbiter. I'm just trying to understand why the #else part
> was here post-337bf13aa9dda.

The #else branch used to return false (i.e., the device is not the 
default).  But this was only used internally by fbcon for minor 
features. So it really didn't matter that much.

Now we need the default for userspace, hence we likely should have an 
#else branch at all.

Best regards
Thomas

>
>> There is a lot of
>> other places that conditionalize code on CONFIG_SCREEN_INFO.  You don't "have"
>> to build in the the VGA arbiter and presumably those are correct.
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Ilpo Järvinen]

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

On Mon, 13 Oct 2025, Thomas Zimmermann wrote:
> Am 13.10.25 um 16:35 schrieb Ilpo Järvinen:
> > On Mon, 13 Oct 2025, Mario Limonciello wrote:
> > 
> > > On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
> > > > On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
> > > > 
> > > > > commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > > > > a screen info check") introduced an implicit dependency upon
> > > > > SCREEN_INFO
> > > > > by removing the open coded implementation.
> > > > > 
> > > > > If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
> > > > > would now return false.  Add a select for SCREEN_INFO to ensure that
> > > > > the
> > > > > VGA arbiter works as intended.
> > > > > 
> > > > > Reported-by: Eric Biggers <ebiggers@kernel.org>
> > > > > Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
> > > > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > > > > a
> > > > > screen info check")
> > > > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > > > ---
> > > > >    drivers/pci/Kconfig | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > > > > index 7065a8e5f9b14..c35fed47addd5 100644
> > > > > --- a/drivers/pci/Kconfig
> > > > > +++ b/drivers/pci/Kconfig
> > > > > @@ -306,6 +306,7 @@ config VGA_ARB
> > > > >    	bool "VGA Arbitration" if EXPERT
> > > > >    	default y
> > > > >    	depends on (PCI && !S390)
> > > > > +	select SCREEN_INFO
> > > > >    	help
> > > > >    	  Some "legacy" VGA devices implemented on PCI typically have
> > > > > the same
> > > > >    	  hard-decoded addresses as they did on ISA. When multiple PCI
> > > > > devices
> > > > The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default()
> > > > with
> > > > a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around
> > > > the
> > > > call, but that now becomes superfluous with this select, no?
> > > Thanks! Will adjust.
> 
> Yes, thanks. You can no longer run into this #else.
> 
> > > 
> > > > Looking into the history of the ifdefs here is quite odd pattern (only
> > > > the last one comes with some explanation but even that is on the vague
> > > > side and fails to remove the actual now unnecessary ifdef :-/):
> > > > 
> > > > #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
> > > > 
> > > > Was it intentional to allow building without CONFIG_SCREEN_INFO?
> > > > 
> > > You mean in the VGA arbiter code?  Or just in general?
> > Here in the VGA arbiter. I'm just trying to understand why the #else part
> > was here post-337bf13aa9dda.
> 
> The #else branch used to return false (i.e., the device is not the default). 
> But this was only used internally by fbcon for minor features. So it really
> didn't matter that much.
> 
> Now we need the default for userspace, hence we likely should have an #else
> branch at all.

Thanks for the explanation.

-- 
 i.

Re: [PATCH] PCI/VGA: Select SCREEN_INFO

[Thomas Zimmermann]

Am 13.10.25 um 17:36 schrieb Ilpo Järvinen:
> On Mon, 13 Oct 2025, Thomas Zimmermann wrote:
>> Am 13.10.25 um 16:35 schrieb Ilpo Järvinen:
>>> On Mon, 13 Oct 2025, Mario Limonciello wrote:
>>>
>>>> On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
>>>>> On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
>>>>>
>>>>>> commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
>>>>>> a screen info check") introduced an implicit dependency upon
>>>>>> SCREEN_INFO
>>>>>> by removing the open coded implementation.
>>>>>>
>>>>>> If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
>>>>>> would now return false.  Add a select for SCREEN_INFO to ensure that
>>>>>> the
>>>>>> VGA arbiter works as intended.
>>>>>>
>>>>>> Reported-by: Eric Biggers <ebiggers@kernel.org>
>>>>>> Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
>>>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
>>>>>> a
>>>>>> screen info check")
>>>>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>>>> ---
>>>>>>     drivers/pci/Kconfig | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>>>>> index 7065a8e5f9b14..c35fed47addd5 100644
>>>>>> --- a/drivers/pci/Kconfig
>>>>>> +++ b/drivers/pci/Kconfig
>>>>>> @@ -306,6 +306,7 @@ config VGA_ARB
>>>>>>     	bool "VGA Arbitration" if EXPERT
>>>>>>     	default y
>>>>>>     	depends on (PCI && !S390)
>>>>>> +	select SCREEN_INFO
>>>>>>     	help
>>>>>>     	  Some "legacy" VGA devices implemented on PCI typically have
>>>>>> the same
>>>>>>     	  hard-decoded addresses as they did on ISA. When multiple PCI
>>>>>> devices
>>>>> The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default()
>>>>> with
>>>>> a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around
>>>>> the
>>>>> call, but that now becomes superfluous with this select, no?
>>>> Thanks! Will adjust.
>> Yes, thanks. You can no longer run into this #else.
>>
>>>>> Looking into the history of the ifdefs here is quite odd pattern (only
>>>>> the last one comes with some explanation but even that is on the vague
>>>>> side and fails to remove the actual now unnecessary ifdef :-/):
>>>>>
>>>>> #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
>>>>>
>>>>> Was it intentional to allow building without CONFIG_SCREEN_INFO?
>>>>>
>>>> You mean in the VGA arbiter code?  Or just in general?
>>> Here in the VGA arbiter. I'm just trying to understand why the #else part
>>> was here post-337bf13aa9dda.
>> The #else branch used to return false (i.e., the device is not the default).
>> But this was only used internally by fbcon for minor features. So it really
>> didn't matter that much.
>>
>> Now we need the default for userspace, hence we likely should have an #else

'should not have'. Oops

>> branch at all.
> Thanks for the explanation.
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)