[PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

Hi all,

This is a collection of some miscellaneous clean-ups for fbcon and some
console drivers. Since v2, I rebased them on linux-next, added some
Reviewed-by: tags from Daniel and Greg, and rewrote the commit messages as
suggested by Jiri. See [1] for v2 links.

It does the following:

  - Garbage collect KD_FONT_OP_COPY callbacks since we disabled it
    recently. Mark it as obsolete.
  - Delete dummy con_font_op() callbacks. (Reviewed by Greg)

  - Add a charcount field to our new font descriptor, `struct font_desc`.
    (Reviewed by Daniel)
  - Do not use a hard-coded 256 for built-in font charcount in
    console/sticore.c, use the new charcount field of `struct font_desc`
    instead. (Reviewed by Daniel)
  - Similarly, in fbcon.c, avoid using the magic negative-indexing macro,
    FNTCHARCNT(). Set `vc->vc_font.charcount` properly and always use that
    instead.

Daniel, hopefully [5/5] removes FNTCHARCNT() for ever, but I have not
tested it sufficiently yet. I remember you mentioned elsewhere that
"fbtest.c" is insufficient for framebuffer testing, then how should we
test it? The first 4 patches should be fine.

Please reference commit messages for more information. Thank you!

[1] v2 links:

2/5: https://lore.kernel.org/lkml/c5563eeea36aae7bd72ea2e985bc610d585ece40.1604306433.git.yepeilin.cs@gmail.com/
3/5: https://lore.kernel.org/lkml/20201028060533.1206307-1-yepeilin.cs@gmail.com/
4/5: https://lore.kernel.org/lkml/c38042bbf5c9777c84900d56c09f3c156b32af48.1603788512.git.yepeilin.cs@gmail.com/
5/5: https://lore.kernel.org/lkml/20201028155139.1220549-1-yepeilin.cs@gmail.com/

Peilin Ye (5):
  console: Delete unused con_font_copy() callback implementations
  console: Delete dummy con_font_set() and con_font_default() callback implementations
  Fonts: Add charcount field to font_desc
  parisc/sticore: Avoid hard-coding built-in font charcount
  fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount

 drivers/usb/misc/sisusbvga/sisusb_con.c | 21 --------
 drivers/video/console/dummycon.c        | 20 --------
 drivers/video/console/sticore.c         |  8 +--
 drivers/video/fbdev/core/fbcon.c        | 68 ++++++++-----------------
 drivers/video/fbdev/core/fbcon_rotate.c |  3 +-
 drivers/video/fbdev/core/tileblit.c     |  4 +-
 include/linux/console.h                 |  1 -
 include/linux/font.h                    |  1 +
 include/uapi/linux/kd.h                 |  2 +-
 lib/fonts/font_10x18.c                  |  1 +
 lib/fonts/font_6x10.c                   |  1 +
 lib/fonts/font_6x11.c                   |  1 +
 lib/fonts/font_6x8.c                    |  1 +
 lib/fonts/font_7x14.c                   |  1 +
 lib/fonts/font_8x16.c                   |  1 +
 lib/fonts/font_8x8.c                    |  1 +
 lib/fonts/font_acorn_8x8.c              |  1 +
 lib/fonts/font_mini_4x6.c               |  1 +
 lib/fonts/font_pearl_8x8.c              |  1 +
 lib/fonts/font_sun12x22.c               |  1 +
 lib/fonts/font_sun8x16.c                |  1 +
 lib/fonts/font_ter16x32.c               |  1 +
 22 files changed, 42 insertions(+), 99 deletions(-)

-- 
2.25.1

[PATCH 1/5] console: Delete unused con_font_copy() callback implementations

[Peilin Ye]

Recently in commit 3c4e0dff2095 ("vt: Disable KD_FONT_OP_COPY") we
disabled the KD_FONT_OP_COPY ioctl() option. Delete all the
con_font_copy() callbacks, since we no longer use them.

Mark KD_FONT_OP_COPY as "obsolete" in include/uapi/linux/kd.h, just like
what we have done for PPPIOCDETACH in commit af8d3c7c001a ("ppp: remove
the PPPIOCDETACH ioctl").

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 drivers/usb/misc/sisusbvga/sisusb_con.c |  6 ------
 drivers/video/console/dummycon.c        |  6 ------
 drivers/video/fbdev/core/fbcon.c        | 11 -----------
 include/linux/console.h                 |  1 -
 include/uapi/linux/kd.h                 |  2 +-
 5 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index c63e545fb105..fd9954381fbf 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -1358,11 +1358,6 @@ static int sisusbdummycon_font_default(struct vc_data *vc,
 	return 0;
 }
 
-static int sisusbdummycon_font_copy(struct vc_data *vc, int con)
-{
-	return 0;
-}
-
 static const struct consw sisusb_dummy_con = {
 	.owner =		THIS_MODULE,
 	.con_startup =		sisusbdummycon_startup,
@@ -1377,7 +1372,6 @@ static const struct consw sisusb_dummy_con = {
 	.con_blank =		sisusbdummycon_blank,
 	.con_font_set =		sisusbdummycon_font_set,
 	.con_font_default =	sisusbdummycon_font_default,
-	.con_font_copy =	sisusbdummycon_font_copy,
 };
 
 int
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index 2a0d0bda7faa..ab3df752fb57 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -136,11 +136,6 @@ static int dummycon_font_default(struct vc_data *vc,
 	return 0;
 }
 
-static int dummycon_font_copy(struct vc_data *vc, int con)
-{
-	return 0;
-}
-
 /*
  *  The console `switch' structure for the dummy console
  *
@@ -161,6 +156,5 @@ const struct consw dummy_con = {
 	.con_blank =	dummycon_blank,
 	.con_font_set =	dummycon_font_set,
 	.con_font_default =	dummycon_font_default,
-	.con_font_copy =	dummycon_font_copy,
 };
 EXPORT_SYMBOL_GPL(dummy_con);
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index cef437817b0d..26d1b0916692 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2451,16 +2451,6 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h,
 	return 0;
 }
 
-static int fbcon_copy_font(struct vc_data *vc, int con)
-{
-	struct fbcon_display *od = &fb_display[con];
-	struct console_font *f = &vc->vc_font;
-
-	if (od->fontdata == f->data)
-		return 0;	/* already the same font... */
-	return fbcon_do_set_font(vc, f->width, f->height, od->fontdata, od->userfont);
-}
-
 /*
  *  User asked to set font; we are guaranteed that
  *	a) width and height are in range 1..32
@@ -3111,7 +3101,6 @@ static const struct consw fb_con = {
 	.con_font_set 		= fbcon_set_font,
 	.con_font_get 		= fbcon_get_font,
 	.con_font_default	= fbcon_set_def_font,
-	.con_font_copy 		= fbcon_copy_font,
 	.con_set_palette 	= fbcon_set_palette,
 	.con_invert_region 	= fbcon_invert_region,
 	.con_screen_pos 	= fbcon_screen_pos,
diff --git a/include/linux/console.h b/include/linux/console.h
index 4b1e26c4cb42..20874db50bc8 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -62,7 +62,6 @@ struct consw {
 	int	(*con_font_get)(struct vc_data *vc, struct console_font *font);
 	int	(*con_font_default)(struct vc_data *vc,
 			struct console_font *font, char *name);
-	int	(*con_font_copy)(struct vc_data *vc, int con);
 	int     (*con_resize)(struct vc_data *vc, unsigned int width,
 			unsigned int height, unsigned int user);
 	void	(*con_set_palette)(struct vc_data *vc,
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 4616b31f84da..ee929ece4112 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -173,7 +173,7 @@ struct console_font {
 #define KD_FONT_OP_SET		0	/* Set font */
 #define KD_FONT_OP_GET		1	/* Get font */
 #define KD_FONT_OP_SET_DEFAULT	2	/* Set font to default, data points to name / NULL */
-#define KD_FONT_OP_COPY		3	/* Copy from another console */
+#define KD_FONT_OP_COPY		3	/* Obsolete, do not use */
 
 #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
 
-- 
2.25.1

[PATCH v3 2/5] console: Delete dummy con_font_set() and con_font_default() callback implementations

[Peilin Ye]

.con_font_set and .con_font_default callbacks should not pass `struct
console_font *` as a parameter, since `struct console_font` is a UAPI
structure.

We are trying to let them use our new kernel font descriptor, `struct
font_desc` instead. To make that work slightly easier, first delete all of
their no-op implementations used by dummy consoles.

This will make KD_FONT_OP_SET and KD_FONT_OP_SET_DEFAULT ioctl() requests
on dummy consoles start to fail and return `-ENOSYS`, which is intended,
since no user should ever expect such operations to succeed on dummy
consoles.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: https://lore.kernel.org/lkml/c5563eeea36aae7bd72ea2e985bc610d585ece40.1604306433.git.yepeilin.cs@gmail.com/

Strictly speaking this is different from v2 (see changelog), but it really
shouldn't matter, so I'm keeping Greg's "Reviewed-by:".

Changes in v3:
  - Improve commit message. (Jiri)
  - Do not delete con_font_copy() callbacks, since they have been deleted
    in patch "console: Delete unused con_font_copy() callback
    implementations".

Change in v2:
  - Do not Cc: stable.

 drivers/usb/misc/sisusbvga/sisusb_con.c | 15 ---------------
 drivers/video/console/dummycon.c        | 14 --------------
 2 files changed, 29 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index fd9954381fbf..dfa0d5ce6012 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -1345,19 +1345,6 @@ static int sisusbdummycon_blank(struct vc_data *vc, int blank, int mode_switch)
 	return 0;
 }
 
-static int sisusbdummycon_font_set(struct vc_data *vc,
-				   struct console_font *font,
-				   unsigned int flags)
-{
-	return 0;
-}
-
-static int sisusbdummycon_font_default(struct vc_data *vc,
-				       struct console_font *font, char *name)
-{
-	return 0;
-}
-
 static const struct consw sisusb_dummy_con = {
 	.owner =		THIS_MODULE,
 	.con_startup =		sisusbdummycon_startup,
@@ -1370,8 +1357,6 @@ static const struct consw sisusb_dummy_con = {
 	.con_scroll =		sisusbdummycon_scroll,
 	.con_switch =		sisusbdummycon_switch,
 	.con_blank =		sisusbdummycon_blank,
-	.con_font_set =		sisusbdummycon_font_set,
-	.con_font_default =	sisusbdummycon_font_default,
 };
 
 int
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index ab3df752fb57..f1711b2f9ff0 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -124,18 +124,6 @@ static int dummycon_switch(struct vc_data *vc)
 	return 0;
 }
 
-static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
-			     unsigned int flags)
-{
-	return 0;
-}
-
-static int dummycon_font_default(struct vc_data *vc,
-				 struct console_font *font, char *name)
-{
-	return 0;
-}
-
 /*
  *  The console `switch' structure for the dummy console
  *
@@ -154,7 +142,5 @@ const struct consw dummy_con = {
 	.con_scroll =	dummycon_scroll,
 	.con_switch =	dummycon_switch,
 	.con_blank =	dummycon_blank,
-	.con_font_set =	dummycon_font_set,
-	.con_font_default =	dummycon_font_default,
 };
 EXPORT_SYMBOL_GPL(dummy_con);
-- 
2.25.1

[PATCH v3 3/5] Fonts: Add charcount field to font_desc

[Peilin Ye]

Subsystems are hard-coding the number of characters of our built-in fonts
as 256. Include that information in our kernel font descriptor, `struct
font_desc`.

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
v2: https://lore.kernel.org/lkml/20201028060533.1206307-1-yepeilin.cs@gmail.com/

Change in v3:
  - Rebase onto linux-next.

Change in v2:
  - Rebase onto 5.10-rc1.

 include/linux/font.h       | 1 +
 lib/fonts/font_10x18.c     | 1 +
 lib/fonts/font_6x10.c      | 1 +
 lib/fonts/font_6x11.c      | 1 +
 lib/fonts/font_6x8.c       | 1 +
 lib/fonts/font_7x14.c      | 1 +
 lib/fonts/font_8x16.c      | 1 +
 lib/fonts/font_8x8.c       | 1 +
 lib/fonts/font_acorn_8x8.c | 1 +
 lib/fonts/font_mini_4x6.c  | 1 +
 lib/fonts/font_pearl_8x8.c | 1 +
 lib/fonts/font_sun12x22.c  | 1 +
 lib/fonts/font_sun8x16.c   | 1 +
 lib/fonts/font_ter16x32.c  | 1 +
 14 files changed, 14 insertions(+)

diff --git a/include/linux/font.h b/include/linux/font.h
index 4f50d736ea72..abf1442ce719 100644
--- a/include/linux/font.h
+++ b/include/linux/font.h
@@ -17,6 +17,7 @@ struct font_desc {
     int idx;
     const char *name;
     unsigned int width, height;
+    unsigned int charcount;
     const void *data;
     int pref;
 };
diff --git a/lib/fonts/font_10x18.c b/lib/fonts/font_10x18.c
index e02f9df24d1e..5d940db626e7 100644
--- a/lib/fonts/font_10x18.c
+++ b/lib/fonts/font_10x18.c
@@ -5137,6 +5137,7 @@ const struct font_desc font_10x18 = {
 	.name	= "10x18",
 	.width	= 10,
 	.height	= 18,
+	.charcount = 256,
 	.data	= fontdata_10x18.data,
 #ifdef __sparc__
 	.pref	= 5,
diff --git a/lib/fonts/font_6x10.c b/lib/fonts/font_6x10.c
index 6e3c4b7691c8..e65df019e0d2 100644
--- a/lib/fonts/font_6x10.c
+++ b/lib/fonts/font_6x10.c
@@ -3083,6 +3083,7 @@ const struct font_desc font_6x10 = {
 	.name	= "6x10",
 	.width	= 6,
 	.height	= 10,
+	.charcount = 256,
 	.data	= fontdata_6x10.data,
 	.pref	= 0,
 };
diff --git a/lib/fonts/font_6x11.c b/lib/fonts/font_6x11.c
index 2d22a24e816f..bd76b3f6b635 100644
--- a/lib/fonts/font_6x11.c
+++ b/lib/fonts/font_6x11.c
@@ -3346,6 +3346,7 @@ const struct font_desc font_vga_6x11 = {
 	.name	= "ProFont6x11",
 	.width	= 6,
 	.height	= 11,
+	.charcount = 256,
 	.data	= fontdata_6x11.data,
 	/* Try avoiding this font if possible unless on MAC */
 	.pref	= -2000,
diff --git a/lib/fonts/font_6x8.c b/lib/fonts/font_6x8.c
index e7442a0d183d..06ace7792521 100644
--- a/lib/fonts/font_6x8.c
+++ b/lib/fonts/font_6x8.c
@@ -2571,6 +2571,7 @@ const struct font_desc font_6x8 = {
 	.name	= "6x8",
 	.width	= 6,
 	.height	= 8,
+	.charcount = 256,
 	.data	= fontdata_6x8.data,
 	.pref	= 0,
 };
diff --git a/lib/fonts/font_7x14.c b/lib/fonts/font_7x14.c
index 9cc7ae2e03f7..a2f561c9fa04 100644
--- a/lib/fonts/font_7x14.c
+++ b/lib/fonts/font_7x14.c
@@ -4113,6 +4113,7 @@ const struct font_desc font_7x14 = {
 	.name	= "7x14",
 	.width	= 7,
 	.height	= 14,
+	.charcount = 256,
 	.data	= fontdata_7x14.data,
 	.pref	= 0,
 };
diff --git a/lib/fonts/font_8x16.c b/lib/fonts/font_8x16.c
index bab25dc59e8d..06ae14088514 100644
--- a/lib/fonts/font_8x16.c
+++ b/lib/fonts/font_8x16.c
@@ -4627,6 +4627,7 @@ const struct font_desc font_vga_8x16 = {
 	.name	= "VGA8x16",
 	.width	= 8,
 	.height	= 16,
+	.charcount = 256,
 	.data	= fontdata_8x16.data,
 	.pref	= 0,
 };
diff --git a/lib/fonts/font_8x8.c b/lib/fonts/font_8x8.c
index 109d0572368f..69570b8c31af 100644
--- a/lib/fonts/font_8x8.c
+++ b/lib/fonts/font_8x8.c
@@ -2578,6 +2578,7 @@ const struct font_desc font_vga_8x8 = {
 	.name	= "VGA8x8",
 	.width	= 8,
 	.height	= 8,
+	.charcount = 256,
 	.data	= fontdata_8x8.data,
 	.pref	= 0,
 };
diff --git a/lib/fonts/font_acorn_8x8.c b/lib/fonts/font_acorn_8x8.c
index fb395f0d4031..18755c33d249 100644
--- a/lib/fonts/font_acorn_8x8.c
+++ b/lib/fonts/font_acorn_8x8.c
@@ -270,6 +270,7 @@ const struct font_desc font_acorn_8x8 = {
 	.name	= "Acorn8x8",
 	.width	= 8,
 	.height	= 8,
+	.charcount = 256,
 	.data	= acorndata_8x8.data,
 #ifdef CONFIG_ARCH_ACORN
 	.pref	= 20,
diff --git a/lib/fonts/font_mini_4x6.c b/lib/fonts/font_mini_4x6.c
index 592774a90917..8d39fd447952 100644
--- a/lib/fonts/font_mini_4x6.c
+++ b/lib/fonts/font_mini_4x6.c
@@ -2152,6 +2152,7 @@ const struct font_desc font_mini_4x6 = {
 	.name	= "MINI4x6",
 	.width	= 4,
 	.height	= 6,
+	.charcount = 256,
 	.data	= fontdata_mini_4x6.data,
 	.pref	= 3,
 };
diff --git a/lib/fonts/font_pearl_8x8.c b/lib/fonts/font_pearl_8x8.c
index a6f95ebce950..b1678ed0017c 100644
--- a/lib/fonts/font_pearl_8x8.c
+++ b/lib/fonts/font_pearl_8x8.c
@@ -2582,6 +2582,7 @@ const struct font_desc font_pearl_8x8 = {
 	.name	= "PEARL8x8",
 	.width	= 8,
 	.height	= 8,
+	.charcount = 256,
 	.data	= fontdata_pearl8x8.data,
 	.pref	= 2,
 };
diff --git a/lib/fonts/font_sun12x22.c b/lib/fonts/font_sun12x22.c
index a5b65bd49604..91daf5ab8b6b 100644
--- a/lib/fonts/font_sun12x22.c
+++ b/lib/fonts/font_sun12x22.c
@@ -6156,6 +6156,7 @@ const struct font_desc font_sun_12x22 = {
 	.name	= "SUN12x22",
 	.width	= 12,
 	.height	= 22,
+	.charcount = 256,
 	.data	= fontdata_sun12x22.data,
 #ifdef __sparc__
 	.pref	= 5,
diff --git a/lib/fonts/font_sun8x16.c b/lib/fonts/font_sun8x16.c
index e577e76a6a7c..81bb4eeae04e 100644
--- a/lib/fonts/font_sun8x16.c
+++ b/lib/fonts/font_sun8x16.c
@@ -268,6 +268,7 @@ const struct font_desc font_sun_8x16 = {
 	.name	= "SUN8x16",
 	.width	= 8,
 	.height	= 16,
+	.charcount = 256,
 	.data	= fontdata_sun8x16.data,
 #ifdef __sparc__
 	.pref	= 10,
diff --git a/lib/fonts/font_ter16x32.c b/lib/fonts/font_ter16x32.c
index f7c3abb6b99e..1955d624177c 100644
--- a/lib/fonts/font_ter16x32.c
+++ b/lib/fonts/font_ter16x32.c
@@ -2062,6 +2062,7 @@ const struct font_desc font_ter_16x32 = {
 	.name	= "TER16x32",
 	.width	= 16,
 	.height = 32,
+	.charcount = 256,
 	.data	= fontdata_ter16x32.data,
 #ifdef __sparc__
 	.pref	= 5,
-- 
2.25.1

[PATCH v3 4/5] parisc/sticore: Avoid hard-coding built-in font charcount

[Peilin Ye]

sti_select_fbfont() and sti_cook_fonts() are hard-coding the number of
characters of our built-in fonts as 256. Recently, we included that
information in our kernel font descriptor `struct font_desc`, so use
`fbfont->charcount` instead of hard-coded values.

Depends on patch "Fonts: Add charcount field to font_desc".

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
v2: https://lore.kernel.org/lkml/c38042bbf5c9777c84900d56c09f3c156b32af48.1603788512.git.yepeilin.cs@gmail.com/

Changes since v1:
  - Slightly improved commit message.
  - Rebased onto linux-next.

 drivers/video/console/sticore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
index d1bb5915082b..f869b723494f 100644
--- a/drivers/video/console/sticore.c
+++ b/drivers/video/console/sticore.c
@@ -506,7 +506,7 @@ sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
 			fbfont->width, fbfont->height, fbfont->name);
 			
 	bpc = ((fbfont->width+7)/8) * fbfont->height; 
-	size = bpc * 256;
+	size = bpc * fbfont->charcount;
 	size += sizeof(struct sti_rom_font);
 
 	nf = kzalloc(size, STI_LOWMEM);
@@ -514,7 +514,7 @@ sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
 		return NULL;
 
 	nf->first_char = 0;
-	nf->last_char = 255;
+	nf->last_char = fbfont->charcount - 1;
 	nf->width = fbfont->width;
 	nf->height = fbfont->height;
 	nf->font_type = STI_FONT_HPROMAN8;
@@ -525,7 +525,7 @@ sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
 
 	dest = nf;
 	dest += sizeof(struct sti_rom_font);
-	memcpy(dest, fbfont->data, bpc*256);
+	memcpy(dest, fbfont->data, bpc * fbfont->charcount);
 
 	cooked_font = kzalloc(sizeof(*cooked_font), GFP_KERNEL);
 	if (!cooked_font) {
@@ -660,7 +660,7 @@ static int sti_cook_fonts(struct sti_cooked_rom *cooked_rom,
 void sti_font_convert_bytemode(struct sti_struct *sti, struct sti_cooked_font *f)
 {
 	unsigned char *n, *p, *q;
-	int size = f->raw->bytes_per_char * 256 + sizeof(struct sti_rom_font);
+	int size = f->raw->bytes_per_char * (f->raw->last_char + 1) + sizeof(struct sti_rom_font);
 	struct sti_rom_font *old_font;
 
 	if (sti->wordmode)
-- 
2.25.1

[PATCH v3 RFC 5/5] fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount

[Peilin Ye]

For user-provided fonts, the framebuffer layer is using a magic
negative-indexing macro, FNTCHARCNT(), to keep track of their number of
characters:

	#define FNTCHARCNT(fd)	(((int *)(fd))[-3])

For built-in fonts, it is using hard-coded values (256). This results in
something like the following:

		map.length = (ops->p->userfont) ?
			FNTCHARCNT(ops->p->fontdata) : 256;

This is unsatisfactory. In fact, there is already a `charcount` field in
our virtual console descriptor (see `struct console_font` inside `struct
vc_data`), let us use it:

		map.length = vc->vc_font.charcount;

Recently we added a `charcount` field to `struct font_desc`. Use it to set
`vc->vc_font.charcount` properly. The idea is:

  - We only use FNTCHARCNT() on `vc->vc_font.data` and `p->fontdata`.
    Assume FNTCHARCNT() is working as intended;
  - Whenever `vc->vc_font.data` is set, also set `vc->vc_font.charcount`
    properly;
  - We can now replace `FNTCHARCNT(vc->vc_font.data)` with
    `vc->vc_font.charcount`;
  - Since `p->fontdata` always point to the same font data buffer with
    `vc->vc_font.data`, we can also replace `FNTCHARCNT(p->fontdata)` with
    `vc->vc_font.charcount`.

In conclusion, set `vc->vc_font.charcount` properly in fbcon_startup(),
fbcon_init(), fbcon_set_disp() and fbcon_do_set_font(), then replace
FNTCHARCNT() with `vc->vc_font.charcount`. No more if-else between
negative-indexing macros and hard-coded values.

Do not include <linux/font.h> in fbcon_rotate.c and tileblit.c, since they
no longer need it.

Depends on patch "Fonts: Add charcount field to font_desc".

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
v2: https://lore.kernel.org/lkml/20201028155139.1220549-1-yepeilin.cs@gmail.com/

Boot-tested only, thus the "RFC". Looking forward to suggestions about how
to test this properly. Thank you!

I am not very sure about the statement "`p->fontdata` always point to the
same font data buffer with `vc->vc_font.data`". It is concluded from code
reading.

Change in v3:
  - Do not touch fbcon_copy_font() since we removed it in patch "console:
    Delete unused con_font_copy() callback implementations"

Changes in v2:
  - Try avoid using FNTCHARCNT() altogether, instead of only changing a
    little bit (Daniel)
  - Set `vc->vc_font.charcount` properly, in fbcon_startup(),
    fbcon_init(), fbcon_set_disp() and fbcon_do_set_font()
  - Replace hard-coded 256 whenever possible

 drivers/video/fbdev/core/fbcon.c        | 57 +++++++++----------------
 drivers/video/fbdev/core/fbcon_rotate.c |  3 +-
 drivers/video/fbdev/core/tileblit.c     |  4 +-
 3 files changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 26d1b0916692..06520667fc07 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -1004,7 +1004,7 @@ static const char *fbcon_startup(void)
 		vc->vc_font.width = font->width;
 		vc->vc_font.height = font->height;
 		vc->vc_font.data = (void *)(p->fontdata = font->data);
-		vc->vc_font.charcount = 256; /* FIXME  Need to support more fonts */
+		vc->vc_font.charcount = font->charcount;
 	} else {
 		p->fontdata = vc->vc_font.data;
 	}
@@ -1032,7 +1032,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 	struct vc_data **default_mode = vc->vc_display_fg;
 	struct vc_data *svc = *default_mode;
 	struct fbcon_display *t, *p = &fb_display[vc->vc_num];
-	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
+	int logo = 1, new_rows, new_cols, rows, cols;
 	int cap, ret;
 
 	if (WARN_ON(info_idx == -1))
@@ -1068,6 +1068,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 						    fvc->vc_font.data);
 			vc->vc_font.width = fvc->vc_font.width;
 			vc->vc_font.height = fvc->vc_font.height;
+			vc->vc_font.charcount = fvc->vc_font.charcount;
 			p->userfont = t->userfont;
 
 			if (p->userfont)
@@ -1083,17 +1084,13 @@ static void fbcon_init(struct vc_data *vc, int init)
 			vc->vc_font.width = font->width;
 			vc->vc_font.height = font->height;
 			vc->vc_font.data = (void *)(p->fontdata = font->data);
-			vc->vc_font.charcount = 256; /* FIXME  Need to
-							support more fonts */
+			vc->vc_font.charcount = font->charcount;
 		}
 	}
 
-	if (p->userfont)
-		charcnt = FNTCHARCNT(p->fontdata);
-
 	vc->vc_can_do_color = (fb_get_color_depth(&info->var, &info->fix)!=1);
 	vc->vc_complement_mask = vc->vc_can_do_color ? 0x7700 : 0x0800;
-	if (charcnt == 256) {
+	if (vc->vc_font.charcount == 256) {
 		vc->vc_hi_font_mask = 0;
 	} else {
 		vc->vc_hi_font_mask = 0x100;
@@ -1358,7 +1355,7 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 	struct vc_data **default_mode, *vc;
 	struct vc_data *svc;
 	struct fbcon_ops *ops = info->fbcon_par;
-	int rows, cols, charcnt = 256;
+	int rows, cols;
 
 	p = &fb_display[unit];
 
@@ -1378,12 +1375,11 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 		vc->vc_font.data = (void *)(p->fontdata = t->fontdata);
 		vc->vc_font.width = (*default_mode)->vc_font.width;
 		vc->vc_font.height = (*default_mode)->vc_font.height;
+		vc->vc_font.charcount = (*default_mode)->vc_font.charcount;
 		p->userfont = t->userfont;
 		if (p->userfont)
 			REFCOUNT(p->fontdata)++;
 	}
-	if (p->userfont)
-		charcnt = FNTCHARCNT(p->fontdata);
 
 	var->activate = FB_ACTIVATE_NOW;
 	info->var.activate = var->activate;
@@ -1393,7 +1389,7 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 	ops->var = info->var;
 	vc->vc_can_do_color = (fb_get_color_depth(&info->var, &info->fix)!=1);
 	vc->vc_complement_mask = vc->vc_can_do_color ? 0x7700 : 0x0800;
-	if (charcnt == 256) {
+	if (vc->vc_font.charcount == 256) {
 		vc->vc_hi_font_mask = 0;
 	} else {
 		vc->vc_hi_font_mask = 0x100;
@@ -2027,7 +2023,7 @@ static int fbcon_resize(struct vc_data *vc, unsigned int width,
 		 */
 		if (pitch <= 0)
 			return -EINVAL;
-		size = CALC_FONTSZ(vc->vc_font.height, pitch, FNTCHARCNT(vc->vc_font.data));
+		size = CALC_FONTSZ(vc->vc_font.height, pitch, vc->vc_font.charcount);
 		if (size > FNTSIZE(vc->vc_font.data))
 			return -EINVAL;
 	}
@@ -2075,7 +2071,7 @@ static int fbcon_switch(struct vc_data *vc)
 	struct fbcon_ops *ops;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fb_var_screeninfo var;
-	int i, ret, prev_console, charcnt = 256;
+	int i, ret, prev_console;
 
 	info = registered_fb[con2fb_map[vc->vc_num]];
 	ops = info->fbcon_par;
@@ -2152,10 +2148,7 @@ static int fbcon_switch(struct vc_data *vc)
 	vc->vc_can_do_color = (fb_get_color_depth(&info->var, &info->fix)!=1);
 	vc->vc_complement_mask = vc->vc_can_do_color ? 0x7700 : 0x0800;
 
-	if (p->userfont)
-		charcnt = FNTCHARCNT(vc->vc_font.data);
-
-	if (charcnt > 256)
+	if (vc->vc_font.charcount > 256)
 		vc->vc_complement_mask <<= 1;
 
 	updatescrollmode(p, info, vc);
@@ -2405,31 +2398,27 @@ static void set_vc_hi_font(struct vc_data *vc, bool set)
 	}
 }
 
-static int fbcon_do_set_font(struct vc_data *vc, int w, int h,
+static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
 			     const u8 * data, int userfont)
 {
 	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	int resize;
-	int cnt;
 	char *old_data = NULL;
 
 	resize = (w != vc->vc_font.width) || (h != vc->vc_font.height);
 	if (p->userfont)
 		old_data = vc->vc_font.data;
-	if (userfont)
-		cnt = FNTCHARCNT(data);
-	else
-		cnt = 256;
 	vc->vc_font.data = (void *)(p->fontdata = data);
 	if ((p->userfont = userfont))
 		REFCOUNT(data)++;
 	vc->vc_font.width = w;
 	vc->vc_font.height = h;
-	if (vc->vc_hi_font_mask && cnt == 256)
+	vc->vc_font.charcount = charcount;
+	if (vc->vc_hi_font_mask && charcount == 256)
 		set_vc_hi_font(vc, false);
-	else if (!vc->vc_hi_font_mask && cnt == 512)
+	else if (!vc->vc_hi_font_mask && charcount == 512)
 		set_vc_hi_font(vc, true);
 
 	if (resize) {
@@ -2496,9 +2485,10 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
 	if (!new_data)
 		return -ENOMEM;
 
+	memset(new_data, 0, FONT_EXTRA_WORDS * sizeof(int));
+
 	new_data += FONT_EXTRA_WORDS * sizeof(int);
 	FNTSIZE(new_data) = size;
-	FNTCHARCNT(new_data) = charcount;
 	REFCOUNT(new_data) = 0;	/* usage counter */
 	for (i=0; i< charcount; i++) {
 		memcpy(new_data + i*h*pitch, data +  i*32*pitch, h*pitch);
@@ -2524,7 +2514,7 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
 			break;
 		}
 	}
-	return fbcon_do_set_font(vc, font->width, font->height, new_data, 1);
+	return fbcon_do_set_font(vc, font->width, font->height, charcount, new_data, 1);
 }
 
 static int fbcon_set_def_font(struct vc_data *vc, struct console_font *font, char *name)
@@ -2540,7 +2530,7 @@ static int fbcon_set_def_font(struct vc_data *vc, struct console_font *font, cha
 
 	font->width = f->width;
 	font->height = f->height;
-	return fbcon_do_set_font(vc, f->width, f->height, f->data, 0);
+	return fbcon_do_set_font(vc, f->width, f->height, f->charcount, f->data, 0);
 }
 
 static u16 palette_red[16];
@@ -3009,7 +2999,6 @@ void fbcon_get_requirement(struct fb_info *info,
 			   struct fb_blit_caps *caps)
 {
 	struct vc_data *vc;
-	struct fbcon_display *p;
 
 	if (caps->flags) {
 		int i, charcnt;
@@ -3018,11 +3007,9 @@ void fbcon_get_requirement(struct fb_info *info,
 			vc = vc_cons[i].d;
 			if (vc && vc->vc_mode == KD_TEXT &&
 			    info->node == con2fb_map[i]) {
-				p = &fb_display[i];
 				caps->x |= 1 << (vc->vc_font.width - 1);
 				caps->y |= 1 << (vc->vc_font.height - 1);
-				charcnt = (p->userfont) ?
-					FNTCHARCNT(p->fontdata) : 256;
+				charcnt = vc->vc_font.charcount;
 				if (caps->len < charcnt)
 					caps->len = charcnt;
 			}
@@ -3032,11 +3019,9 @@ void fbcon_get_requirement(struct fb_info *info,
 
 		if (vc && vc->vc_mode == KD_TEXT &&
 		    info->node == con2fb_map[fg_console]) {
-			p = &fb_display[fg_console];
 			caps->x = 1 << (vc->vc_font.width - 1);
 			caps->y = 1 << (vc->vc_font.height - 1);
-			caps->len = (p->userfont) ?
-				FNTCHARCNT(p->fontdata) : 256;
+			caps->len = vc->vc_font.charcount;
 		}
 	}
 }
diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c
index ac72d4f85f7d..ef1d421c261a 100644
--- a/drivers/video/fbdev/core/fbcon_rotate.c
+++ b/drivers/video/fbdev/core/fbcon_rotate.c
@@ -14,7 +14,6 @@
 #include <linux/fb.h>
 #include <linux/vt_kern.h>
 #include <linux/console.h>
-#include <linux/font.h>
 #include <asm/types.h>
 #include "fbcon.h"
 #include "fbcon_rotate.h"
@@ -33,7 +32,7 @@ static int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
 
 	src = ops->fontdata = vc->vc_font.data;
 	ops->cur_rotate = ops->p->con_rotate;
-	len = (!ops->p->userfont) ? 256 : FNTCHARCNT(src);
+	len = vc->vc_font.charcount;
 	s_cellsize = ((vc->vc_font.width + 7)/8) *
 		vc->vc_font.height;
 	d_cellsize = s_cellsize;
diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c
index 628fe5e010c0..e2cc0cb6d50e 100644
--- a/drivers/video/fbdev/core/tileblit.c
+++ b/drivers/video/fbdev/core/tileblit.c
@@ -13,7 +13,6 @@
 #include <linux/fb.h>
 #include <linux/vt_kern.h>
 #include <linux/console.h>
-#include <linux/font.h>
 #include <asm/types.h>
 #include "fbcon.h"
 
@@ -145,8 +144,7 @@ void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info)
 		map.width = vc->vc_font.width;
 		map.height = vc->vc_font.height;
 		map.depth = 1;
-		map.length = (ops->p->userfont) ?
-			FNTCHARCNT(ops->p->fontdata) : 256;
+		map.length = vc->vc_font.charcount;
 		map.data = ops->p->fontdata;
 		info->tileops->fb_settile(info, &map);
 	}
-- 
2.25.1

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Daniel Vetter]

On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> Hi all,
> 
> This is a collection of some miscellaneous clean-ups for fbcon and some
> console drivers. Since v2, I rebased them on linux-next, added some
> Reviewed-by: tags from Daniel and Greg, and rewrote the commit messages as
> suggested by Jiri. See [1] for v2 links.
> 
> It does the following:
> 
>   - Garbage collect KD_FONT_OP_COPY callbacks since we disabled it
>     recently. Mark it as obsolete.
>   - Delete dummy con_font_op() callbacks. (Reviewed by Greg)
> 
>   - Add a charcount field to our new font descriptor, `struct font_desc`.
>     (Reviewed by Daniel)
>   - Do not use a hard-coded 256 for built-in font charcount in
>     console/sticore.c, use the new charcount field of `struct font_desc`
>     instead. (Reviewed by Daniel)
>   - Similarly, in fbcon.c, avoid using the magic negative-indexing macro,
>     FNTCHARCNT(). Set `vc->vc_font.charcount` properly and always use that
>     instead.
> 
> Daniel, hopefully [5/5] removes FNTCHARCNT() for ever, but I have not
> tested it sufficiently yet. I remember you mentioned elsewhere that
> "fbtest.c" is insufficient for framebuffer testing, then how should we
> test it? The first 4 patches should be fine.
> 
> Please reference commit messages for more information. Thank you!
> 
> [1] v2 links:
> 
> 2/5: https://lore.kernel.org/lkml/c5563eeea36aae7bd72ea2e985bc610d585ece40.1604306433.git.yepeilin.cs@gmail.com/
> 3/5: https://lore.kernel.org/lkml/20201028060533.1206307-1-yepeilin.cs@gmail.com/
> 4/5: https://lore.kernel.org/lkml/c38042bbf5c9777c84900d56c09f3c156b32af48.1603788512.git.yepeilin.cs@gmail.com/
> 5/5: https://lore.kernel.org/lkml/20201028155139.1220549-1-yepeilin.cs@gmail.com/
> 
> Peilin Ye (5):
>   console: Delete unused con_font_copy() callback implementations
>   console: Delete dummy con_font_set() and con_font_default() callback implementations
>   Fonts: Add charcount field to font_desc
>   parisc/sticore: Avoid hard-coding built-in font charcount
>   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount

Patches all look good to me, if Greg is ok with me applying the entire
pile to drm-misc-next I'll do that next week.

Thanks, Daniel

> 
>  drivers/usb/misc/sisusbvga/sisusb_con.c | 21 --------
>  drivers/video/console/dummycon.c        | 20 --------
>  drivers/video/console/sticore.c         |  8 +--
>  drivers/video/fbdev/core/fbcon.c        | 68 ++++++++-----------------
>  drivers/video/fbdev/core/fbcon_rotate.c |  3 +-
>  drivers/video/fbdev/core/tileblit.c     |  4 +-
>  include/linux/console.h                 |  1 -
>  include/linux/font.h                    |  1 +
>  include/uapi/linux/kd.h                 |  2 +-
>  lib/fonts/font_10x18.c                  |  1 +
>  lib/fonts/font_6x10.c                   |  1 +
>  lib/fonts/font_6x11.c                   |  1 +
>  lib/fonts/font_6x8.c                    |  1 +
>  lib/fonts/font_7x14.c                   |  1 +
>  lib/fonts/font_8x16.c                   |  1 +
>  lib/fonts/font_8x8.c                    |  1 +
>  lib/fonts/font_acorn_8x8.c              |  1 +
>  lib/fonts/font_mini_4x6.c               |  1 +
>  lib/fonts/font_pearl_8x8.c              |  1 +
>  lib/fonts/font_sun12x22.c               |  1 +
>  lib/fonts/font_sun8x16.c                |  1 +
>  lib/fonts/font_ter16x32.c               |  1 +
>  22 files changed, 42 insertions(+), 99 deletions(-)
> 
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Greg Kroah-Hartman]

On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote:
> On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> > Hi all,
> > 
> > This is a collection of some miscellaneous clean-ups for fbcon and some
> > console drivers. Since v2, I rebased them on linux-next, added some
> > Reviewed-by: tags from Daniel and Greg, and rewrote the commit messages as
> > suggested by Jiri. See [1] for v2 links.
> > 
> > It does the following:
> > 
> >   - Garbage collect KD_FONT_OP_COPY callbacks since we disabled it
> >     recently. Mark it as obsolete.
> >   - Delete dummy con_font_op() callbacks. (Reviewed by Greg)
> > 
> >   - Add a charcount field to our new font descriptor, `struct font_desc`.
> >     (Reviewed by Daniel)
> >   - Do not use a hard-coded 256 for built-in font charcount in
> >     console/sticore.c, use the new charcount field of `struct font_desc`
> >     instead. (Reviewed by Daniel)
> >   - Similarly, in fbcon.c, avoid using the magic negative-indexing macro,
> >     FNTCHARCNT(). Set `vc->vc_font.charcount` properly and always use that
> >     instead.
> > 
> > Daniel, hopefully [5/5] removes FNTCHARCNT() for ever, but I have not
> > tested it sufficiently yet. I remember you mentioned elsewhere that
> > "fbtest.c" is insufficient for framebuffer testing, then how should we
> > test it? The first 4 patches should be fine.
> > 
> > Please reference commit messages for more information. Thank you!
> > 
> > [1] v2 links:
> > 
> > 2/5: https://lore.kernel.org/lkml/c5563eeea36aae7bd72ea2e985bc610d585ece40.1604306433.git.yepeilin.cs@gmail.com/
> > 3/5: https://lore.kernel.org/lkml/20201028060533.1206307-1-yepeilin.cs@gmail.com/
> > 4/5: https://lore.kernel.org/lkml/c38042bbf5c9777c84900d56c09f3c156b32af48.1603788512.git.yepeilin.cs@gmail.com/
> > 5/5: https://lore.kernel.org/lkml/20201028155139.1220549-1-yepeilin.cs@gmail.com/
> > 
> > Peilin Ye (5):
> >   console: Delete unused con_font_copy() callback implementations
> >   console: Delete dummy con_font_set() and con_font_default() callback implementations
> >   Fonts: Add charcount field to font_desc
> >   parisc/sticore: Avoid hard-coding built-in font charcount
> >   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount
> 
> Patches all look good to me, if Greg is ok with me applying the entire
> pile to drm-misc-next I'll do that next week.

Yes, please do!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

> On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> > > Peilin Ye (5):
> > >   console: Delete unused con_font_copy() callback implementations
> > >   console: Delete dummy con_font_set() and con_font_default() callback implementations
> > >   Fonts: Add charcount field to font_desc
> > >   parisc/sticore: Avoid hard-coding built-in font charcount
> > >   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount
> > 
> > Patches all look good to me, if Greg is ok with me applying the entire
> > pile to drm-misc-next I'll do that next week.

On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote:
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing!  Questions about the last patch [5/5] though, it
depends on the following assumption:

"""
For each console `idx`, `vc_cons[idx].d->vc_font.data` and
`fb_display[idx].fontdata` always point to the same buffer.
"""

Is this true?  I think it is true by grepping for `fontdata`.  I also
noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
interchangeably, see fbcon_get_requirement():

		vc = vc_cons[fg_console].d;
		[...]
			p = &fb_display[fg_console];
			caps->x = 1 << (vc->vc_font.width - 1);
					^^^^^^^^^^^
			caps->y = 1 << (vc->vc_font.height - 1);
					^^^^^^^^^^^
			caps->len = (p->userfont) ?
				FNTCHARCNT(p->fontdata) : 256;
					   ^^^^^^^^^^^

If it is true, then what is the point of using `fontdata` in `struct
fbcon_display`?  Just for the `userfont` flag?  Should we delete
`fontdata`, when we no longer need the `userfont` flag?

In this sense I think [5/5] needs more testing.  Do we have test files
for fbcon, or should I try to write some tests from scratch?

Thank you,
Peilin Ye

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Greg Kroah-Hartman]

On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote:
> > On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> > > > Peilin Ye (5):
> > > >   console: Delete unused con_font_copy() callback implementations
> > > >   console: Delete dummy con_font_set() and con_font_default() callback implementations
> > > >   Fonts: Add charcount field to font_desc
> > > >   parisc/sticore: Avoid hard-coding built-in font charcount
> > > >   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount
> > > 
> > > Patches all look good to me, if Greg is ok with me applying the entire
> > > pile to drm-misc-next I'll do that next week.
> 
> On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote:
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks for reviewing!  Questions about the last patch [5/5] though, it
> depends on the following assumption:
> 
> """
> For each console `idx`, `vc_cons[idx].d->vc_font.data` and
> `fb_display[idx].fontdata` always point to the same buffer.
> """
> 
> Is this true?  I think it is true by grepping for `fontdata`.  I also
> noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
> interchangeably, see fbcon_get_requirement():
> 
> 		vc = vc_cons[fg_console].d;
> 		[...]
> 			p = &fb_display[fg_console];
> 			caps->x = 1 << (vc->vc_font.width - 1);
> 					^^^^^^^^^^^
> 			caps->y = 1 << (vc->vc_font.height - 1);
> 					^^^^^^^^^^^
> 			caps->len = (p->userfont) ?
> 				FNTCHARCNT(p->fontdata) : 256;
> 					   ^^^^^^^^^^^
> 
> If it is true, then what is the point of using `fontdata` in `struct
> fbcon_display`?  Just for the `userfont` flag?  Should we delete
> `fontdata`, when we no longer need the `userfont` flag?

Yes, after a quick look, I think your analysis here is correct.  So if
you can delete that, it would be nice if possible.

> In this sense I think [5/5] needs more testing.  Do we have test files
> for fbcon, or should I try to write some tests from scratch?

I don't know of any direct tests, I usually just booted into that mode
and saw if everything looked like it did before.  There must be some
tool that you can use to change the current font, as it seems to happen
at boot time on some distros.  I can't remember what it's called at the
moment...

thanks,

greg k-h

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Greg Kroah-Hartman]

On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote:
> On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote:
> > > On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> > > > > Peilin Ye (5):
> > > > >   console: Delete unused con_font_copy() callback implementations
> > > > >   console: Delete dummy con_font_set() and con_font_default() callback implementations
> > > > >   Fonts: Add charcount field to font_desc
> > > > >   parisc/sticore: Avoid hard-coding built-in font charcount
> > > > >   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount
> > > > 
> > > > Patches all look good to me, if Greg is ok with me applying the entire
> > > > pile to drm-misc-next I'll do that next week.
> > 
> > On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote:
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > Thanks for reviewing!  Questions about the last patch [5/5] though, it
> > depends on the following assumption:
> > 
> > """
> > For each console `idx`, `vc_cons[idx].d->vc_font.data` and
> > `fb_display[idx].fontdata` always point to the same buffer.
> > """
> > 
> > Is this true?  I think it is true by grepping for `fontdata`.  I also
> > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
> > interchangeably, see fbcon_get_requirement():
> > 
> > 		vc = vc_cons[fg_console].d;
> > 		[...]
> > 			p = &fb_display[fg_console];
> > 			caps->x = 1 << (vc->vc_font.width - 1);
> > 					^^^^^^^^^^^
> > 			caps->y = 1 << (vc->vc_font.height - 1);
> > 					^^^^^^^^^^^
> > 			caps->len = (p->userfont) ?
> > 				FNTCHARCNT(p->fontdata) : 256;
> > 					   ^^^^^^^^^^^
> > 
> > If it is true, then what is the point of using `fontdata` in `struct
> > fbcon_display`?  Just for the `userfont` flag?  Should we delete
> > `fontdata`, when we no longer need the `userfont` flag?
> 
> Yes, after a quick look, I think your analysis here is correct.  So if
> you can delete that, it would be nice if possible.
> 
> > In this sense I think [5/5] needs more testing.  Do we have test files
> > for fbcon, or should I try to write some tests from scratch?
> 
> I don't know of any direct tests, I usually just booted into that mode
> and saw if everything looked like it did before.  There must be some
> tool that you can use to change the current font, as it seems to happen
> at boot time on some distros.  I can't remember what it's called at the
> moment...

Ah, here's a hint:
	https://wiki.archlinux.org/index.php/Linux_console#Fonts

The setfont tool should help you out here.

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote:
> On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote:
> > On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote:
> > > Thanks for reviewing!  Questions about the last patch [5/5] though, it
> > > depends on the following assumption:
> > > 
> > > """
> > > For each console `idx`, `vc_cons[idx].d->vc_font.data` and
> > > `fb_display[idx].fontdata` always point to the same buffer.
> > > """
> > > 
> > > Is this true?  I think it is true by grepping for `fontdata`.  I also
> > > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
> > > interchangeably, see fbcon_get_requirement():
> > > 
> > > 		vc = vc_cons[fg_console].d;
> > > 		[...]
> > > 			p = &fb_display[fg_console];
> > > 			caps->x = 1 << (vc->vc_font.width - 1);
> > > 					^^^^^^^^^^^
> > > 			caps->y = 1 << (vc->vc_font.height - 1);
> > > 					^^^^^^^^^^^
> > > 			caps->len = (p->userfont) ?
> > > 				FNTCHARCNT(p->fontdata) : 256;
> > > 					   ^^^^^^^^^^^
> > > 
> > > If it is true, then what is the point of using `fontdata` in `struct
> > > fbcon_display`?  Just for the `userfont` flag?  Should we delete
> > > `fontdata`, when we no longer need the `userfont` flag?
> > 
> > Yes, after a quick look, I think your analysis here is correct.  So if
> > you can delete that, it would be nice if possible.

I see, at the moment we still need `userfont` for refcount handling, I
will try to delete both `fontdata` and `userfont` after refcount is
taken care of.

> > > In this sense I think [5/5] needs more testing.  Do we have test files
> > > for fbcon, or should I try to write some tests from scratch?
> > 
> > I don't know of any direct tests, I usually just booted into that mode
> > and saw if everything looked like it did before.  There must be some
> > tool that you can use to change the current font, as it seems to happen
> > at boot time on some distros.  I can't remember what it's called at the
> > moment...
> 
> Ah, here's a hint:
> 	https://wiki.archlinux.org/index.php/Linux_console#Fonts
> 
> The setfont tool should help you out here.

Oh, I didn't know about this one.  I'll go experimenting with it,
thank you!

Peilin Ye

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Daniel Vetter]

On Sat, Nov 14, 2020 at 07:47:16AM -0500, Peilin Ye wrote:
> On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote:
> > On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote:
> > > On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote:
> > > > Thanks for reviewing!  Questions about the last patch [5/5] though, it
> > > > depends on the following assumption:
> > > > 
> > > > """
> > > > For each console `idx`, `vc_cons[idx].d->vc_font.data` and
> > > > `fb_display[idx].fontdata` always point to the same buffer.
> > > > """
> > > > 
> > > > Is this true?  I think it is true by grepping for `fontdata`.  I also
> > > > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
> > > > interchangeably, see fbcon_get_requirement():
> > > > 
> > > > 		vc = vc_cons[fg_console].d;
> > > > 		[...]
> > > > 			p = &fb_display[fg_console];
> > > > 			caps->x = 1 << (vc->vc_font.width - 1);
> > > > 					^^^^^^^^^^^
> > > > 			caps->y = 1 << (vc->vc_font.height - 1);
> > > > 					^^^^^^^^^^^
> > > > 			caps->len = (p->userfont) ?
> > > > 				FNTCHARCNT(p->fontdata) : 256;
> > > > 					   ^^^^^^^^^^^
> > > > 
> > > > If it is true, then what is the point of using `fontdata` in `struct
> > > > fbcon_display`?  Just for the `userfont` flag?  Should we delete
> > > > `fontdata`, when we no longer need the `userfont` flag?
> > > 
> > > Yes, after a quick look, I think your analysis here is correct.  So if
> > > you can delete that, it would be nice if possible.
> 
> I see, at the moment we still need `userfont` for refcount handling, I
> will try to delete both `fontdata` and `userfont` after refcount is
> taken care of.

+1 on sunsetting fondata. I think there's a bunch of these in fbcon code,
because the console subsystem is older than the standard "allow drivers to
embed the subsystem struct into their own private struct" subclassing
model. So lots of global arrays indexed by the console id :-/

> > > > In this sense I think [5/5] needs more testing.  Do we have test files
> > > > for fbcon, or should I try to write some tests from scratch?
> > > 
> > > I don't know of any direct tests, I usually just booted into that mode
> > > and saw if everything looked like it did before.  There must be some
> > > tool that you can use to change the current font, as it seems to happen
> > > at boot time on some distros.  I can't remember what it's called at the
> > > moment...
> > 
> > Ah, here's a hint:
> > 	https://wiki.archlinux.org/index.php/Linux_console#Fonts
> > 
> > The setfont tool should help you out here.
> 
> Oh, I didn't know about this one.  I'll go experimenting with it,
> thank you!

We're also trying to start some kind of test suite for fbdev chardev ioctl
interface in the gpu test suite. fbcon tests are maybe more related to
tty/vt, and I have no idea whether anything exists there already.

But if you want to do some automated testcases for fbcon and there's
absolutely no other home for them, the gpu test suite might be an option
of last resort too:

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

On Mon, Nov 16, 2020 at 11:09:49AM +0100, Daniel Vetter wrote:
> On Sat, Nov 14, 2020 at 07:47:16AM -0500, Peilin Ye wrote:
> > On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote:
> > > On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote:
> > > > On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote:
> > > > > Thanks for reviewing!  Questions about the last patch [5/5] though, it
> > > > > depends on the following assumption:
> > > > > 
> > > > > """
> > > > > For each console `idx`, `vc_cons[idx].d->vc_font.data` and
> > > > > `fb_display[idx].fontdata` always point to the same buffer.
> > > > > """
> > > > > 
> > > > > Is this true?  I think it is true by grepping for `fontdata`.  I also
> > > > > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata`
> > > > > interchangeably, see fbcon_get_requirement():
> > > > > 
> > > > > 		vc = vc_cons[fg_console].d;
> > > > > 		[...]
> > > > > 			p = &fb_display[fg_console];
> > > > > 			caps->x = 1 << (vc->vc_font.width - 1);
> > > > > 					^^^^^^^^^^^
> > > > > 			caps->y = 1 << (vc->vc_font.height - 1);
> > > > > 					^^^^^^^^^^^
> > > > > 			caps->len = (p->userfont) ?
> > > > > 				FNTCHARCNT(p->fontdata) : 256;
> > > > > 					   ^^^^^^^^^^^
> > > > > 
> > > > > If it is true, then what is the point of using `fontdata` in `struct
> > > > > fbcon_display`?  Just for the `userfont` flag?  Should we delete
> > > > > `fontdata`, when we no longer need the `userfont` flag?
> > > > 
> > > > Yes, after a quick look, I think your analysis here is correct.  So if
> > > > you can delete that, it would be nice if possible.
> > 
> > I see, at the moment we still need `userfont` for refcount handling, I
> > will try to delete both `fontdata` and `userfont` after refcount is
> > taken care of.
> 
> +1 on sunsetting fondata. I think there's a bunch of these in fbcon code,
> because the console subsystem is older than the standard "allow drivers to
> embed the subsystem struct into their own private struct" subclassing
> model. So lots of global arrays indexed by the console id :-/

Yeah, I saw your comments about registered_fb[] :(

> We're also trying to start some kind of test suite for fbdev chardev ioctl
> interface in the gpu test suite. fbcon tests are maybe more related to
> tty/vt, and I have no idea whether anything exists there already.
> 
> But if you want to do some automated testcases for fbcon and there's
> absolutely no other home for them, the gpu test suite might be an option
> of last resort too:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validation

Oh, I didn't know about igt-gpu-tools, thanks for the pointer!  Now,
since charcount is taken care of, and font_desc now contains all fields
of console_font, I think it is a good time to replace console_font with
font_desc in vc_data.  Then we can get rid of FNTSUM(), FNTSIZE(), then
(finally) REFCOUNT().

I will start working on vc_data, after done enough testing on [5/5],
ofc. Thanks,

Peilin Ye

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote:
> Ah, here's a hint:
> 	https://wiki.archlinux.org/index.php/Linux_console#Fonts
> 
> The setfont tool should help you out here.

setfont seems to work fine, I tried Georgian-Fixed16 (256 chars) and
Uni2-VGA16 (512 chars) under /usr/share/consolefonts/ in my Ubuntu box,
including setting all consoles to the same font:

for i in {1..6}; do
        sudo setfont -C /dev/tty${i} /usr/share/consolefonts/Georgian-Fixed16.psf.gz
done

Font rotation also seems to work fine:

for i in {1..4}; do
        echo $i | sudo tee /sys/class/graphics/fbcon/rotate
        sleep 1
done

One last thing I can think of is tile blitting, but I don't have the
hardware (e.g. a Matrox G400 card, see FB_TILEBLITTING in
drivers/video/fbdev/Kconfig) at hand, nor did I figure out how to
simulate it after searching for a while.  However based on the other
tests above I believe vc->vc_font.charcount is set properly.

Thanks,
Peilin Ye

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Daniel Vetter]

On Thu, Nov 19, 2020 at 9:33 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote:
> > Ah, here's a hint:
> >       https://wiki.archlinux.org/index.php/Linux_console#Fonts
> >
> > The setfont tool should help you out here.
>
> setfont seems to work fine, I tried Georgian-Fixed16 (256 chars) and
> Uni2-VGA16 (512 chars) under /usr/share/consolefonts/ in my Ubuntu box,
> including setting all consoles to the same font:
>
> for i in {1..6}; do
>         sudo setfont -C /dev/tty${i} /usr/share/consolefonts/Georgian-Fixed16.psf.gz
> done
>
> Font rotation also seems to work fine:
>
> for i in {1..4}; do
>         echo $i | sudo tee /sys/class/graphics/fbcon/rotate
>         sleep 1
> done

Thanks a lot for checking all this.

> One last thing I can think of is tile blitting, but I don't have the
> hardware (e.g. a Matrox G400 card, see FB_TILEBLITTING in
> drivers/video/fbdev/Kconfig) at hand, nor did I figure out how to
> simulate it after searching for a while.  However based on the other
> tests above I believe vc->vc_font.charcount is set properly.

tbh I'll just go ahead and delete it if it's broken :-)

Userspace we have to keep working (and there's actually people
creating new products on top of drm display drivers using fbdev
emulation and /dev/fb/0 interface!), but kernel internal stuff like
fbcon acceleration we can trim pretty aggressively I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Peilin Ye]

On Thu, Nov 19, 2020 at 04:10:57PM +0100, Daniel Vetter wrote:
> On Thu, Nov 19, 2020 at 9:33 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> > setfont seems to work fine, I tried Georgian-Fixed16 (256 chars) and
> > Uni2-VGA16 (512 chars) under /usr/share/consolefonts/ in my Ubuntu box,
> > including setting all consoles to the same font:
> >
> > for i in {1..6}; do
> >         sudo setfont -C /dev/tty${i} /usr/share/consolefonts/Georgian-Fixed16.psf.gz
> > done
> >
> > Font rotation also seems to work fine:
> >
> > for i in {1..4}; do
> >         echo $i | sudo tee /sys/class/graphics/fbcon/rotate
> >         sleep 1
> > done
> 
> Thanks a lot for checking all this.

Not a problem, watching my console rotating was fun :)

> > One last thing I can think of is tile blitting, but I don't have the
> > hardware (e.g. a Matrox G400 card, see FB_TILEBLITTING in
> > drivers/video/fbdev/Kconfig) at hand, nor did I figure out how to
> > simulate it after searching for a while.  However based on the other
> > tests above I believe vc->vc_font.charcount is set properly.
> 
> tbh I'll just go ahead and delete it if it's broken :-)
> 
> Userspace we have to keep working (and there's actually people
> creating new products on top of drm display drivers using fbdev
> emulation and /dev/fb/0 interface!), but kernel internal stuff like
> fbcon acceleration we can trim pretty aggressively I think.

Ah, I see, I'll leave it be, then.

Thanks,
Peilin Ye

Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c

[Daniel Vetter]

On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote:
> > > Hi all,
> > > 
> > > This is a collection of some miscellaneous clean-ups for fbcon and some
> > > console drivers. Since v2, I rebased them on linux-next, added some
> > > Reviewed-by: tags from Daniel and Greg, and rewrote the commit messages as
> > > suggested by Jiri. See [1] for v2 links.
> > > 
> > > It does the following:
> > > 
> > >   - Garbage collect KD_FONT_OP_COPY callbacks since we disabled it
> > >     recently. Mark it as obsolete.
> > >   - Delete dummy con_font_op() callbacks. (Reviewed by Greg)
> > > 
> > >   - Add a charcount field to our new font descriptor, `struct font_desc`.
> > >     (Reviewed by Daniel)
> > >   - Do not use a hard-coded 256 for built-in font charcount in
> > >     console/sticore.c, use the new charcount field of `struct font_desc`
> > >     instead. (Reviewed by Daniel)
> > >   - Similarly, in fbcon.c, avoid using the magic negative-indexing macro,
> > >     FNTCHARCNT(). Set `vc->vc_font.charcount` properly and always use that
> > >     instead.
> > > 
> > > Daniel, hopefully [5/5] removes FNTCHARCNT() for ever, but I have not
> > > tested it sufficiently yet. I remember you mentioned elsewhere that
> > > "fbtest.c" is insufficient for framebuffer testing, then how should we
> > > test it? The first 4 patches should be fine.
> > > 
> > > Please reference commit messages for more information. Thank you!
> > > 
> > > [1] v2 links:
> > > 
> > > 2/5: https://lore.kernel.org/lkml/c5563eeea36aae7bd72ea2e985bc610d585ece40.1604306433.git.yepeilin.cs@gmail.com/
> > > 3/5: https://lore.kernel.org/lkml/20201028060533.1206307-1-yepeilin.cs@gmail.com/
> > > 4/5: https://lore.kernel.org/lkml/c38042bbf5c9777c84900d56c09f3c156b32af48.1603788512.git.yepeilin.cs@gmail.com/
> > > 5/5: https://lore.kernel.org/lkml/20201028155139.1220549-1-yepeilin.cs@gmail.com/
> > > 
> > > Peilin Ye (5):
> > >   console: Delete unused con_font_copy() callback implementations
> > >   console: Delete dummy con_font_set() and con_font_default() callback implementations
> > >   Fonts: Add charcount field to font_desc
> > >   parisc/sticore: Avoid hard-coding built-in font charcount
> > >   fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount
> > 
> > Patches all look good to me, if Greg is ok with me applying the entire
> > pile to drm-misc-next I'll do that next week.
> 
> Yes, please do!
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Pulled entire series into drm-misc-next.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch