Skip to contentSkip to conversation

cleaning up asm/vga.h situation

RFC PATCH In review Al Viro 2 participants 6 messages Nov 6, 2024 – Nov 21, 2024

Subsystems (6)
lore
Share & tools
lore

Al Viro

author
#0 lore raw

RFC PATCH 1/1 cleaning up asm/vga.h situation

asm/vga.h and linux/vt_buffer.h are playing odd games that might
or might not have made sense once upon a time, but currently they are too convoluted for no good reason.

vt_buffer.h defines the primitives for access to vt contents -
scr_{read,write,memcpy,memmove,memset}w(). In all cases it's an array of
16bit values - attribute/symbol pairs. Normally it's just a kmalloc'ed
buffer; there are few exceptions:
* vgacon.c has the active console contents directly in VRAM;
for such consoles vt.c code will end up accessing that.
* vgacon.c and mdacon.c use those to access the text-mode screen
image directly in VRAM.

VRAM is little-endian; for kmalloc'ed buffers the endianness is
up to us - all accesses will be via scr_... primitives, so in absence of VGA_CONSOLE and MDA_CONSOLE we can simply keep them host-endian and turn the accessors into obvious inlines; that's the default.

The few architectures that might have VGA_CONSOLE or MDA_CONSOLE
may override that. Overrides are too convoluted - we have 4 macros (VT_BUF_HAVE_{RW,MEMCPYW,MEMSETW,MEMMOVEW}) asm/vga.h may define to control what gets overridden. Situation:
* alpha: needs separate handling for mem and iomem cases,
overrides everything.
* arm: fine with defaults.
* mips: keeps everything little-endian, nothing special needed
for VRAM access. Overrides everything, replacements for memcpy/memmove are identical to defaults.
* powerpc: more than slightly ridiculous - if VGA_CONSOLE or
MDA_CONSOLE is enabled, same as mips, defaults otherwise. The ridiculous part is that VGA_CONSOLE is actually impossible to enable there, but MDA_CONSOLE (that is to say, support of ISA Hercules cards for the second monitor) *is* possible. On the boxen with ISA slots, that is...
* sparc: overrides everything, replacements are all identical to
defaults. Neither vgacon or mdacon are possible there, so that's fine, but why bother with overrides in the first place?
* x86: fine with defaults.

Defaults themselves are also overcomplicated - there's e.g. handling of the case when we'd overridden read/write, but not memcpy (which never happens).

There are several other things in asm/vga.h, in addition
to these overrides. One is the helpers for byte VRAM accesses - vga_{read,write}b(), used only by vgacon.c and only to read/modify the screen font. Hell knows whether it really needs to be done byte-by-byte; pointer is always to VRAM. Incidentally, the loops in there are just weird - 8192 bytes of iomem read or written one by one, with cond_resched() after each. Apparently, qemu with 64 vcpu managed to be stuck in that for more than a minute... Another thing is VGA_MAP_MEM()
- finding where VRAM is mapped. Used by vgacon, mdacon and vga16fb (the last one - x86-only).

All of those are provided by asm/vga.h on anything that
might support any of the users, so there's no point keeping them in asm-generic/vga.h - as the matter of fact, that file can be empty, which would kill arch/sh/include/asm/vga.h.

Patch series cleaning that up lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.vt_buffer Individual patches in followups; please, review.

Shortlog:
Al Viro (4):
vt_buffer.h: get rid of dead code in default scr_...() instances asm/vga.h: don't bother with scr_mem{cpy,move}v() unless we need to
sparc: get rid of asm/vga.h
empty include/asm-generic/vga.h

Diffstat:

 arch/mips/include/asm/vga.h    |  4 ---
 arch/powerpc/include/asm/vga.h |  5 ----
 arch/sh/include/asm/vga.h      |  7 -----
 arch/sparc/include/asm/vga.h   | 60 ------------------------------------------
 include/asm-generic/vga.h      | 23 +---------------
 include/linux/vt_buffer.h      | 24 -----------------
 6 files changed, 1 insertion(+), 122 deletions(-)
 delete mode 100644 arch/sh/include/asm/vga.h
 delete mode 100644 arch/sparc/include/asm/vga.h
author
#1 lore raw

PATCH 1/4 vt_buffer.h: get rid of dead code in default scr_...() instances

Only 4 architectures define VT_BUF_HAVE_RW (alpha, mips, powerpc, sparc) and all of them define VT_BUF_HAVE_MEM{SET,CPY,MOVE}W. In other words, the code under #ifdef VT_BUF_HAVE_RW in default scr_mem...w() instances won't be compiled anyway.

---
 include/linux/vt_buffer.h | 24 ------------------------
 1 file changed, 24 deletions(-)
include/linux/vt_buffer.h +0 -24
index 919d999a8c1d..b6eeb8cb6070 100644
@@ -28,45 +28,21 @@
 #ifndef VT_BUF_HAVE_MEMSETW
 static inline void scr_memsetw(u16 *s, u16 c, unsigned int count)
 {
-#ifdef VT_BUF_HAVE_RW
-	count /= 2;
-	while (count--)
-		scr_writew(c, s++);
-#else
 	memset16(s, c, count / 2);
-#endif
 }
 #endif
 
 #ifndef VT_BUF_HAVE_MEMCPYW
 static inline void scr_memcpyw(u16 *d, const u16 *s, unsigned int count)
 {
-#ifdef VT_BUF_HAVE_RW
-	count /= 2;
-	while (count--)
-		scr_writew(scr_readw(s++), d++);
-#else
 	memcpy(d, s, count);
-#endif
 }
 #endif
 
 #ifndef VT_BUF_HAVE_MEMMOVEW
 static inline void scr_memmovew(u16 *d, const u16 *s, unsigned int count)
 {
-#ifdef VT_BUF_HAVE_RW
-	if (d < s)
-		scr_memcpyw(d, s, count);
-	else {
-		count /= 2;
-		d += count;
-		s += count;
-		while (count--)
-			scr_writew(scr_readw(--s), --d);
-	}
-#else
 	memmove(d, s, count);
-#endif
 }
 #endif
 
Signature · 1 line
2.39.5
author
#2 lore raw

PATCH 2/4 asm/vga.h: don't bother with scr_mem{cpy,move}v() unless we need to

In reply to Al Viro

... if they are identical to fallbacks, just leave them alone.

---
 arch/mips/include/asm/vga.h    |  4 ----
 arch/powerpc/include/asm/vga.h |  5 -----
 arch/sparc/include/asm/vga.h   | 16 ----------------
 3 files changed, 25 deletions(-)
arch/mips/include/asm/vga.h +0 -4
index 0136e0366698..491c2b5aeb81 100644
@@ -47,10 +47,6 @@ static inline void scr_memsetw(u16 *s, u16 v, unsigned int count)
 	memset16(s, cpu_to_le16(v), count / 2);
 }
 
-#define scr_memcpyw(d, s, c) memcpy(d, s, c)
-#define scr_memmovew(d, s, c) memmove(d, s, c)
-#define VT_BUF_HAVE_MEMCPYW
-#define VT_BUF_HAVE_MEMMOVEW
 #define VT_BUF_HAVE_MEMSETW
 
 #endif /* _ASM_VGA_H */
arch/powerpc/include/asm/vga.h +0 -5
index fcf721682a71..f2dc40e1c52a 100644
@@ -40,11 +40,6 @@ static inline void scr_memsetw(u16 *s, u16 v, unsigned int n)
 	memset16(s, cpu_to_le16(v), n / 2);
 }
 
-#define VT_BUF_HAVE_MEMCPYW
-#define VT_BUF_HAVE_MEMMOVEW
-#define scr_memcpyw	memcpy
-#define scr_memmovew	memmove
-
 #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */
 
 #ifdef __powerpc64__
arch/sparc/include/asm/vga.h +0 -16
index 2952d667d936..cc2bc60b4c8b 100644
@@ -14,8 +14,6 @@
 
 #define VT_BUF_HAVE_RW
 #define VT_BUF_HAVE_MEMSETW
-#define VT_BUF_HAVE_MEMCPYW
-#define VT_BUF_HAVE_MEMMOVEW
 
 #undef scr_writew
 #undef scr_readw
@@ -41,20 +39,6 @@ static inline void scr_memsetw(u16 *p, u16 v, unsigned int n)
 	memset16(p, cpu_to_le16(v), n / 2);
 }
 
-static inline void scr_memcpyw(u16 *d, u16 *s, unsigned int n)
-{
-	BUG_ON((long) d >= 0);
-
-	memcpy(d, s, n);
-}
-
-static inline void scr_memmovew(u16 *d, u16 *s, unsigned int n)
-{
-	BUG_ON((long) d >= 0);
-
-	memmove(d, s, n);
-}
-
 #define VGA_MAP_MEM(x,s) (x)
 
 #endif
Signature · 1 line
2.39.5
author
#3 lore raw

PATCH 3/4 sparc: get rid of asm/vga.h

In reply to Al Viro

The only thing we are using it for on sparc is telling vt_buffer.h to pick what it would pick by default anyway - we are not accessing any VRAM here...

---
 arch/sparc/include/asm/vga.h | 44 ------------------------------------
 1 file changed, 44 deletions(-)
 delete mode 100644 arch/sparc/include/asm/vga.h
arch/sparc/include/asm/vga.h deleted +0 -44
deleted file mode 100644
index cc2bc60b4c8b..000000000000
@@ -1,44 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- *	Access to VGA videoram
- *
- *	(c) 1998 Martin Mares <[email protected]>
- */
-
-#ifndef _LINUX_ASM_VGA_H_
-#define _LINUX_ASM_VGA_H_
-
-#include <linux/bug.h>
-#include <linux/string.h>
-#include <asm/types.h>
-
-#define VT_BUF_HAVE_RW
-#define VT_BUF_HAVE_MEMSETW
-
-#undef scr_writew
-#undef scr_readw
-
-static inline void scr_writew(u16 val, u16 *addr)
-{
-	BUG_ON((long) addr >= 0);
-
-	*addr = val;
-}
-
-static inline u16 scr_readw(const u16 *addr)
-{
-	BUG_ON((long) addr >= 0);
-
-	return *addr;
-}
-
-static inline void scr_memsetw(u16 *p, u16 v, unsigned int n)
-{
-	BUG_ON((long) p >= 0);
-
-	memset16(p, cpu_to_le16(v), n / 2);
-}
-
-#define VGA_MAP_MEM(x,s) (x)
-
-#endif
Signature · 1 line
2.39.5
author
#4 lore raw

empty include/asm-generic/vga.h

In reply to Al Viro

all places that use anything defined in it (vgacon, mdacon and vga16fb) are built only on architectures that have all that stuff in their native asm/vga.h

allows to kill stub asm/vga.h on sh, while we are at it...

---
 arch/sh/include/asm/vga.h |  7 -------
 include/asm-generic/vga.h | 23 +----------------------
 2 files changed, 1 insertion(+), 29 deletions(-)
 delete mode 100644 arch/sh/include/asm/vga.h
arch/sh/include/asm/vga.h deleted +0 -7
deleted file mode 100644
index 089fbdc6c0b1..000000000000
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __ASM_SH_VGA_H
-#define __ASM_SH_VGA_H
-
-/* Stupid drivers. */
-
-#endif /* __ASM_SH_VGA_H */
include/asm-generic/vga.h +1 -22
index adf91a783b5c..5dcaf4ae904a 100644
@@ -1,25 +1,4 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-/*
- *	Access to VGA videoram
- *
- *	(c) 1998 Martin Mares <[email protected]>
- */
 #ifndef __ASM_GENERIC_VGA_H
 #define __ASM_GENERIC_VGA_H
-
-/*
- *	On most architectures that support VGA, we can just
- *	recalculate addresses and then access the videoram
- *	directly without any black magic.
- *
- *	Everyone else needs to ioremap the address and use
- *	proper I/O accesses.
- */
-#ifndef VGA_MAP_MEM
-#define VGA_MAP_MEM(x, s) (unsigned long)phys_to_virt(x)
-#endif
-
-#define vga_readb(x) (*(x))
-#define vga_writeb(x, y) (*(y) = (x))
-
-#endif /* _ASM_GENERIC_VGA_H */
+#endif /* __ASM_GENERIC_VGA_H */
Signature · 1 line
2.39.5
#5 lore raw
In reply to Al Viro

Hi Al,

Quoted 5 lines · Al Viro

* powerpc: more than slightly ridiculous - if VGA_CONSOLE or
MDA_CONSOLE is enabled, same as mips, defaults otherwise. The ridiculous part is that VGA_CONSOLE is actually impossible to enable there, but MDA_CONSOLE (that is to say, support of ISA Hercules cards for the second monitor) *is* possible. On the boxen with ISA slots, that is...

Once upon a time, in the previous century, you could enable VGA_CONSOLE, run an x86 BIOS emulator, and get a VGA text console on a (second) "PC style" (aka normal) graphics card in a CHRP PPC box... With CHRP support being removed, there is no point in resurrecting that.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds