mbox series

[00/12] arch,fbdev: Move screen_info into arch/

Message ID 20230629121952.10559-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series arch,fbdev: Move screen_info into arch/ | expand

Message

Thomas Zimmermann June 29, 2023, 11:45 a.m. UTC
The variables screen_info and edid_info provide information about
the system's screen, and possibly EDID data of the connected display.
Both are defined and set by architecture code. But both variables are
declared in non-arch header files. Dependencies are at bease loosely
tracked. To resolve this, move the global state screen_info and its
companion edid_info into arch/. Only declare them on architectures
that define them. List dependencies on the variables in the Kconfig
files. Also clean up the callers.

Patch 1 to 4 resolve a number of unnecessary include statements of
<linux/screen_info.h>. The header should only be included in source
files that access struct screen_info.

Patches 5 to 7 move the declaration of screen_info and edid_info to
<asm-generic/screen_info.h>. Architectures that provide either set
a Kconfig token to enable them.

Patches 8 to 9 make users of screen_info depend on the architecture's
feature.

Finally, patches 10 to 12 rework fbdev's handling of firmware EDID
data to make use of existing helpers and the refactored edid_info.

Tested on x86-64. Built for a variety of platforms.

Future directions: with the patchset in place, it will become possible
to provide screen_info and edid_info only if there are users. Some
architectures do this by testing for CONFIG_VT, CONFIG_DUMMY_CONSOLE,
etc. A more uniform approach would be nice. We should also attempt
to minimize access to the global screen_info as much as possible. To
do so, some drivers, such as efifb and vesafb, would require an update.
The firmware's EDID data could possibly made available outside of fbdev.
For example, the simpledrm and ofdrm drivers could provide such data
to userspace compositors.

Thomas Zimmermann (12):
  efi: Do not include <linux/screen_info.h> from EFI header
  fbdev/sm712fb: Do not include <linux/screen_info.h>
  sysfb: Do not include <linux/screen_info.h> from sysfb header
  staging/sm750fb: Do not include <linux/screen_info.h>
  arch: Remove trailing whitespaces
  arch: Declare screen_info in <asm/screen_info.h>
  arch/x86: Declare edid_info in <asm/screen_info.h>
  drivers/firmware: Remove trailing whitespaces
  drivers: Add dependencies on CONFIG_ARCH_HAS_SCREEN_INFO
  fbdev/core: Use fb_is_primary_device() in fb_firmware_edid()
  fbdev/core: Protect edid_info with CONFIG_ARCH_HAS_EDID_INFO
  fbdev/core: Define empty fb_firmware_edid() in <linux/fb.h>

 arch/Kconfig                                  | 12 +++++++
 arch/alpha/Kconfig                            |  1 +
 arch/arm/Kconfig                              |  1 +
 arch/arm/kernel/efi.c                         |  2 ++
 arch/arm64/Kconfig                            |  1 +
 arch/arm64/kernel/efi.c                       |  1 +
 arch/csky/Kconfig                             |  1 +
 arch/hexagon/Kconfig                          |  1 +
 arch/ia64/Kconfig                             |  5 +--
 arch/loongarch/Kconfig                        |  1 +
 arch/mips/Kconfig                             |  1 +
 arch/nios2/Kconfig                            |  1 +
 arch/powerpc/Kconfig                          |  1 +
 arch/riscv/Kconfig                            |  1 +
 arch/sh/Kconfig                               |  7 ++--
 arch/sparc/Kconfig                            |  1 +
 arch/x86/Kconfig                              |  2 ++
 arch/xtensa/Kconfig                           |  1 +
 drivers/firmware/Kconfig                      |  3 +-
 drivers/firmware/efi/Kconfig                  |  1 +
 drivers/firmware/efi/libstub/efi-stub-entry.c |  2 ++
 drivers/firmware/efi/libstub/screen_info.c    |  2 ++
 drivers/gpu/drm/Kconfig                       |  1 +
 drivers/hv/Kconfig                            |  1 +
 drivers/staging/sm750fb/sm750.c               |  1 -
 drivers/staging/sm750fb/sm750_accel.c         |  1 -
 drivers/staging/sm750fb/sm750_cursor.c        |  1 -
 drivers/staging/sm750fb/sm750_hw.c            |  1 -
 drivers/video/console/Kconfig                 |  2 ++
 drivers/video/fbdev/Kconfig                   |  4 +++
 drivers/video/fbdev/core/fbmon.c              | 34 ++++++-------------
 drivers/video/fbdev/i810/i810-i2c.c           |  2 +-
 drivers/video/fbdev/intelfb/intelfbdrv.c      |  2 +-
 drivers/video/fbdev/nvidia/nv_i2c.c           |  2 +-
 drivers/video/fbdev/savage/savagefb-i2c.c     |  2 +-
 drivers/video/fbdev/sm712fb.c                 |  9 +++--
 include/asm-generic/Kbuild                    |  1 +
 include/asm-generic/screen_info.h             | 18 ++++++++++
 include/linux/efi.h                           |  3 +-
 include/linux/fb.h                            | 10 +++++-
 include/linux/screen_info.h                   |  2 +-
 include/linux/sysfb.h                         |  3 +-
 include/video/edid.h                          |  3 --
 43 files changed, 105 insertions(+), 47 deletions(-)
 create mode 100644 include/asm-generic/screen_info.h


base-commit: d2f0af8472494398a42153684b790b723a79f143

Comments

Arnd Bergmann June 29, 2023, 1:31 p.m. UTC | #1
On Thu, Jun 29, 2023, at 13:45, Thomas Zimmermann wrote:
> The variables screen_info and edid_info provide information about
> the system's screen, and possibly EDID data of the connected display.
> Both are defined and set by architecture code. But both variables are
> declared in non-arch header files. Dependencies are at bease loosely
> tracked. To resolve this, move the global state screen_info and its
> companion edid_info into arch/. Only declare them on architectures
> that define them. List dependencies on the variables in the Kconfig
> files. Also clean up the callers.
>
> Patch 1 to 4 resolve a number of unnecessary include statements of
> <linux/screen_info.h>. The header should only be included in source
> files that access struct screen_info.
>
> Patches 5 to 7 move the declaration of screen_info and edid_info to
> <asm-generic/screen_info.h>. Architectures that provide either set
> a Kconfig token to enable them.
>
> Patches 8 to 9 make users of screen_info depend on the architecture's
> feature.
>
> Finally, patches 10 to 12 rework fbdev's handling of firmware EDID
> data to make use of existing helpers and the refactored edid_info.
>
> Tested on x86-64. Built for a variety of platforms.

This all looks like a nice cleanup!

> Future directions: with the patchset in place, it will become possible
> to provide screen_info and edid_info only if there are users. Some
> architectures do this by testing for CONFIG_VT, CONFIG_DUMMY_CONSOLE,
> etc. A more uniform approach would be nice. We should also attempt
> to minimize access to the global screen_info as much as possible. To
> do so, some drivers, such as efifb and vesafb, would require an update.
> The firmware's EDID data could possibly made available outside of fbdev.
> For example, the simpledrm and ofdrm drivers could provide such data
> to userspace compositors.

I suspect that most architectures that provide a screen_info only
have this in order to compile the framebuffer drivers, and provide
hardcoded data that does not even reflect any real hardware.

We can probably reduce the number of architectures that do this
a lot, especially if we get EFI out of the picture.

      Arnd
Thomas Zimmermann June 29, 2023, 2:15 p.m. UTC | #2
Hi

Am 29.06.23 um 15:31 schrieb Arnd Bergmann:
> On Thu, Jun 29, 2023, at 13:45, Thomas Zimmermann wrote:
>> The variables screen_info and edid_info provide information about
>> the system's screen, and possibly EDID data of the connected display.
>> Both are defined and set by architecture code. But both variables are
>> declared in non-arch header files. Dependencies are at bease loosely
>> tracked. To resolve this, move the global state screen_info and its
>> companion edid_info into arch/. Only declare them on architectures
>> that define them. List dependencies on the variables in the Kconfig
>> files. Also clean up the callers.
>>
>> Patch 1 to 4 resolve a number of unnecessary include statements of
>> <linux/screen_info.h>. The header should only be included in source
>> files that access struct screen_info.
>>
>> Patches 5 to 7 move the declaration of screen_info and edid_info to
>> <asm-generic/screen_info.h>. Architectures that provide either set
>> a Kconfig token to enable them.
>>
>> Patches 8 to 9 make users of screen_info depend on the architecture's
>> feature.
>>
>> Finally, patches 10 to 12 rework fbdev's handling of firmware EDID
>> data to make use of existing helpers and the refactored edid_info.
>>
>> Tested on x86-64. Built for a variety of platforms.
> 
> This all looks like a nice cleanup!

I guess that patches 1 to 4 are uncontroversial and could be landed 
quickly. Patches 10 to 12 are probably as well.

> 
>> Future directions: with the patchset in place, it will become possible
>> to provide screen_info and edid_info only if there are users. Some
>> architectures do this by testing for CONFIG_VT, CONFIG_DUMMY_CONSOLE,
>> etc. A more uniform approach would be nice. We should also attempt
>> to minimize access to the global screen_info as much as possible. To
>> do so, some drivers, such as efifb and vesafb, would require an update.
>> The firmware's EDID data could possibly made available outside of fbdev.
>> For example, the simpledrm and ofdrm drivers could provide such data
>> to userspace compositors.
> 
> I suspect that most architectures that provide a screen_info only
> have this in order to compile the framebuffer drivers, and provide
> hardcoded data that does not even reflect any real hardware.

That's quite possible. Only x86's bootparam and EFI code sets 
screen_info from external data. The rest is hardcoded. A number of 
architectures protect screen_info with CONFIG_VT, CONFIG_DUMMY_CONSOLE, 
etc. In a later patchset, I wanted to change this such that these users 
of screen_info would enable the feature via select in their Kconfig.

Do you know the reason for this branch in dummycon:

https://elixir.bootlin.com/linux/v6.4/source/drivers/video/console/dummycon.c#L21

What is special about arm that dummycon uses the screeninfo?

> 
> We can probably reduce the number of architectures that do this
> a lot, especially if we get EFI out of the picture.

Can you elaborate?

Best regards
Thomas

> 
>        Arnd
Arnd Bergmann June 29, 2023, 2:29 p.m. UTC | #3
On Thu, Jun 29, 2023, at 15:31, Arnd Bergmann wrote:
> On Thu, Jun 29, 2023, at 13:45, Thomas Zimmermann wrote:
>>
>> Future directions: with the patchset in place, it will become possible
>> to provide screen_info and edid_info only if there are users. Some
>> architectures do this by testing for CONFIG_VT, CONFIG_DUMMY_CONSOLE,
>> etc. A more uniform approach would be nice. We should also attempt
>> to minimize access to the global screen_info as much as possible. To
>> do so, some drivers, such as efifb and vesafb, would require an update.
>> The firmware's EDID data could possibly made available outside of fbdev.
>> For example, the simpledrm and ofdrm drivers could provide such data
>> to userspace compositors.
>
> I suspect that most architectures that provide a screen_info only
> have this in order to compile the framebuffer drivers, and provide
> hardcoded data that does not even reflect any real hardware.
>
> We can probably reduce the number of architectures that do this
> a lot, especially if we get EFI out of the picture.

I tried to have another look at who uses what, and here are
some observations:

- EFIFB and hyperv are the only ones that are relevant on modern
  systmes, and they are only used on systems using EFI, so they
  could use a separate data structure that is defined as part of
  drivers/firmware/efi. This would likely mean we don't have to
  define a separate screen_info for arm64, loongarch, ia64 and
  riscv, and could separate the legacy vgacon/vesafb stuff on
  arm32 from the efi side.

- FB_SIS can likely be marked 'depends on X86' like FB_INTEL,
  it seems to depend on x86 BOOT_VESA_SUPPORT.

- FB_VGA16 is currently support on powerpc and enabled on
  one defconfig (pasemi), which I'm fairly sure is a mistake,
  so this could be made x86 specific as well.

- VGA_CONSOLE has a complicated Kconfig dependency list that
  lists platforms without VGA support but I think this is better
  expressed with a positive list. It looks like csky, hexagon,
  nios2 and xtensa should be excluded here and not provide
  screen_info.

- arm and mips only need to provide screen_info on machines
  with VGA_CONSOLE. On Arm we have a dependency on
  footbridge/integrator/netwinder, while on mips the
  dependency can be added to the platforms that fill
  the info (mips, malta, sibyte, sni).

- DUMMY_CONSOLE only uses screen_info on arm, and this should
  likely be limited to the three obsolete machines that
  support VGACON. Almost all Arm machines use DT these days
  and won't ever fill the screen info dynamically.

      Arnd
Arnd Bergmann June 29, 2023, 2:42 p.m. UTC | #4
On Thu, Jun 29, 2023, at 16:15, Thomas Zimmermann wrote:
> Am 29.06.23 um 15:31 schrieb Arnd Bergmann:
>> On Thu, Jun 29, 2023, at 13:45, Thomas Zimmermann wrote:
>>> Future directions: with the patchset in place, it will become possible
>>> to provide screen_info and edid_info only if there are users. Some
>>> architectures do this by testing for CONFIG_VT, CONFIG_DUMMY_CONSOLE,
>>> etc. A more uniform approach would be nice. We should also attempt
>>> to minimize access to the global screen_info as much as possible. To
>>> do so, some drivers, such as efifb and vesafb, would require an update.
>>> The firmware's EDID data could possibly made available outside of fbdev.
>>> For example, the simpledrm and ofdrm drivers could provide such data
>>> to userspace compositors.
>> 
>> I suspect that most architectures that provide a screen_info only
>> have this in order to compile the framebuffer drivers, and provide
>> hardcoded data that does not even reflect any real hardware.
>
> That's quite possible. Only x86's bootparam and EFI code sets 
> screen_info from external data. The rest is hardcoded. A number of 
> architectures protect screen_info with CONFIG_VT, CONFIG_DUMMY_CONSOLE, 
> etc. In a later patchset, I wanted to change this such that these users 
> of screen_info would enable the feature via select in their Kconfig.
>
> Do you know the reason for this branch in dummycon:
>
> https://elixir.bootlin.com/linux/v6.4/source/drivers/video/console/dummycon.c#L21
>
> What is special about arm that dummycon uses the screeninfo?

I can only guess myself, but I see that the values are only ever
set from the old ATAGS data, and not from DT on any of the
modern ones, and my interpretation is that this is meant to
match whatever the vga console was set to on the three
platforms that support vgacon.
 
I see this was added in linux-2.1.111, just before the
corresponding sparc specific hack was removed, but I don't have
patch descriptions from that era. Russell might remember, or know
if that is actually still needed.

   Arnd