| Message ID | cover.1603788511.git.yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Preparation work for using font_desc in vc_data | expand |
Hi Daniel,
More about the 3 things we've discussed before:
1. Cleaning up con_font_op():
(drivers/tty/vt/vt.c)
int con_font_op(struct vc_data *vc, struct console_font_op *op)
{
switch (op->op) {
case KD_FONT_OP_SET:
return con_font_set(vc, op);
case KD_FONT_OP_GET:
return con_font_get(vc, op);
case KD_FONT_OP_SET_DEFAULT:
return con_font_default(vc, op);
case KD_FONT_OP_COPY:
return con_font_copy(vc, op);
}
return -ENOSYS;
}
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
> I think if we change the conf_font_get/set/default/copy functions to not
> take the *op struct (which is take pretty arbitrarily from one of the
> ioctl), but the parameters each needs directly, that would clean up the
> code a _lot_.
This is on my TODO list! One day I came up with some idea about
fbcon.c, so I postponed this a bit...
2. Removing dummy functions, like sisusbdummycon_font_set():
Turns out, before c396a5bf457f ("console: Expand dummy functions for
CFI"), they were just some macros:
-#define SISUSBCONDUMMY (void *)sisusbdummycon_dummy
+static int sisusbdummycon_font_set(struct vc_data *vc,
+ struct console_font *font,
+ unsigned int flags)
+{
+ return 0;
+}
...and they had been there for a very long (10+ years) time. Removing
code like this makes me a bit nervous, and...
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
> This actually does something. tbh I would not be surprises if the
> fb_set utility is the only thing that uses this - with a bit of code
> search we could perhaps confirm this, and delete all the other
> implementations.
...you mentioned code search, where & what should we look at, in order
to confirm it's safe to remove them?
3. Using `font_desc` in `vc_data`:
Our plan for the gradual conversion was to use a helper function to
set font for a vc, but after reviewing the 300-ish occurrence of
`vc_font`, it seems like code doesn't usually set it as a whole:
(drivers/usb/misc/sisusbvga/sisusb_con.c)
[...]
c->vc_font.height = sisusb->current_font_height;
[...]
...that's it! It only cares about the height. There are only 4 or 5
places in fbcon.c that actually set all fields of `vc_font`, like:
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 */
To make it even more complicated, `p` is a `struct fbcon_display *`,
containing yet another font data pointer (`fontdata`) that I think
should be replaced by a `font_desc *`...
In conclusion, I think it's all about a few hard problems in fbcon.c.
I'll keep trying and see how it goes.
Thank you,
Peilin Ye
On Tue, Oct 27, 2020 at 5:50 PM Peilin Ye <yepeilin.cs@gmail.com> wrote: > > Hi Daniel, > > More about the 3 things we've discussed before: > > 1. Cleaning up con_font_op(): > > (drivers/tty/vt/vt.c) > int con_font_op(struct vc_data *vc, struct console_font_op *op) > { > switch (op->op) { > case KD_FONT_OP_SET: > return con_font_set(vc, op); > case KD_FONT_OP_GET: > return con_font_get(vc, op); > case KD_FONT_OP_SET_DEFAULT: > return con_font_default(vc, op); > case KD_FONT_OP_COPY: > return con_font_copy(vc, op); > } > return -ENOSYS; > } > > On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote: > > I think if we change the conf_font_get/set/default/copy functions to not > > take the *op struct (which is take pretty arbitrarily from one of the > > ioctl), but the parameters each needs directly, that would clean up the > > code a _lot_. > > This is on my TODO list! One day I came up with some idea about > fbcon.c, so I postponed this a bit... > > 2. Removing dummy functions, like sisusbdummycon_font_set(): > > Turns out, before c396a5bf457f ("console: Expand dummy functions for > CFI"), they were just some macros: > > -#define SISUSBCONDUMMY (void *)sisusbdummycon_dummy > +static int sisusbdummycon_font_set(struct vc_data *vc, > + struct console_font *font, > + unsigned int flags) > +{ > + return 0; > +} > > ...and they had been there for a very long (10+ years) time. Removing > code like this makes me a bit nervous, and... > > On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote: > > This actually does something. tbh I would not be surprises if the > > fb_set utility is the only thing that uses this - with a bit of code > > search we could perhaps confirm this, and delete all the other > > implementations. > > ...you mentioned code search, where & what should we look at, in order > to confirm it's safe to remove them? Way back there was google's code search, which was awesome. Now I just put the structure name/ioctl #define/number into google/bing/duckduckgo and see if anything turns up. Plus check how it's used in fb tools (although I just recently learned that fb-test pretty much disappeared from the internet, very hard to find the original). If you're unsure, we can merge a patch, then wait about 1 year for any users to show up with problems. If that's not the case, assume they're all gone, or it was never used and just implemented because it was copied from somewhere else, or "just in case". There's lots of dead uapi around. > 3. Using `font_desc` in `vc_data`: > > Our plan for the gradual conversion was to use a helper function to > set font for a vc, but after reviewing the 300-ish occurrence of > `vc_font`, it seems like code doesn't usually set it as a whole: > > (drivers/usb/misc/sisusbvga/sisusb_con.c) > [...] > c->vc_font.height = sisusb->current_font_height; > [...] > > ...that's it! It only cares about the height. There are only 4 or 5 > places in fbcon.c that actually set all fields of `vc_font`, like: > > 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 */ > > To make it even more complicated, `p` is a `struct fbcon_display *`, > containing yet another font data pointer (`fontdata`) that I think > should be replaced by a `font_desc *`... > > In conclusion, I think it's all about a few hard problems in fbcon.c. > I'll keep trying and see how it goes. Yeah fbcon.c is pretty good horrors unfortunately :-/ -Daniel
On Tue, Oct 27, 2020 at 07:36:54PM +0100, Daniel Vetter wrote: > On Tue, Oct 27, 2020 at 5:50 PM Peilin Ye <yepeilin.cs@gmail.com> wrote: > > ...you mentioned code search, where & what should we look at, in order > > to confirm it's safe to remove them? > > Way back there was google's code search, which was awesome. Now I just > put the structure name/ioctl #define/number into > google/bing/duckduckgo and see if anything turns up. Plus check how > it's used in fb tools (although I just recently learned that fb-test > pretty much disappeared from the internet, very hard to find the > original). > > If you're unsure, we can merge a patch, then wait about 1 year for any > users to show up with problems. If that's not the case, assume they're > all gone, or it was never used and just implemented because it was > copied from somewhere else, or "just in case". There's lots of dead > uapi around. I see, it will be my next thing to do. Hopefully this will remove a lot of console_font occurrences. Peilin
On Tue, Oct 27, 2020 at 12:27:35PM -0400, Peilin Ye wrote: > Hi Daniel, Hi Greg, Hi all, > > We are planning to use `font_desc` instead of `console_font` in `vc_data`, > and this is just some prep work for it. It doesn't do much, but at least > it removes two "FIXME"s in fbcon.c :) Btw nitpeek on how you submit patches: The threading you're using here is "deep" i.e. every patch is reply to the previous patch. The usual thing is flat, i.e. all patches are replies to the cover letter. I think only very old versions of git had the former as a default, so not sure what's going on. But the deep threading makes piecing the conversation together quite a bit harder, at least here in mutt. -Daniel > > Peilin Ye (5): > [1/5] fbdev/atafb: Remove unused extern variables > > Searching for `fontdata` gave me this in fbdev/atafb.c: > > extern unsigned char fontdata_8x8[]; > extern unsigned char fontdata_8x16[]; > > ...which freaked me out, since in 6735b4632def ("Fonts: Support > FONT_EXTRA_WORDS macros for built-in fonts") I changed them from char > arrays to structures, in lib/fonts/. Fortunately it turns out these > extern variables have nothing to do with lib/fonts/, and are not being > used anywhere, so remove them for less confusion. > > m68k cross-compiled. > > [2/5] Fonts: Make font size unsigned in font_desc > > Our goal is to use `font_desc` "everywhere" in the kernel, and signed > `width` and `height` is inappropriate. > > Also, change some printk() format identifiers in console/sticore.c from > `%d` to `%u`. parisc cross-compiled. > > [3/5] Fonts: Add charcount field to font_desc > > Add `unsigned int charcount` to `font_desc`, and update each of our 13 > built-in fonts. > > [4/5] fbcon: Avoid hard-coding built-in font charcount > [5/5] parisc/sticore: Avoid hard-coding built-in font charcount > > Everyone (tty, fbcon, sticore, etc.) is assuming that all built-in fonts > have 256 characters, and is using hard-coded 256 or 255 everywhere. > These two patches removes some of them. [5/5] is parisc cross-compiled. > > Now is a good time to review all find_font() and get_default_font() > callers: > > drivers/media/pci/solo6x10/solo6x10-enc.c 133 const struct font_desc *vga = find_font("VGA8x16"); > drivers/media/test-drivers/vimc/vimc-core.c 268 const struct font_desc *font = find_font("VGA8x16"); > drivers/media/test-drivers/vivid/vivid-core.c 1928 const struct font_desc *font = find_font("VGA8x16"); > drivers/usb/misc/sisusbvga/sisusb.c 2285 myfont = find_font("VGA8x16"); > * These 4 only care about font VGA8x16, so let them be for now; > > drivers/video/console/sticore.c 499 fbfont = find_font(fbfont_name); > drivers/video/console/sticore.c 501 fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0); > * Uses 255 and 256, (hopefully) cleaned up by [5/5]; > > drivers/video/fbdev/core/fbcon.c 999 if (!fontname[0] || !(font = find_font(fontname))) > drivers/video/fbdev/core/fbcon.c 1000 font = get_default_font(info->var.xres, > drivers/video/fbdev/core/fbcon.c 1078 if (!fontname[0] || !(font = find_font(fontname))) > drivers/video/fbdev/core/fbcon.c 1079 font = get_default_font(info->var.xres, > * Use 256, cleaned up by [4/5]; > > drivers/video/fbdev/core/fbcon.c 2548 else if (!(f = find_font(name))) > drivers/video/fbdev/core/fbcon.c 2546 f = get_default_font(info->var.xres, info->var.yres, > * Uses 256 but no easy fix. I'll clean this up after making > fbcon_do_set_font() pass a `font_desc` as parameter; > > drivers/firmware/efi/earlycon.c 234 font = get_default_font(xres, yres, -1, -1); > * Does not care about charcount. > > Thank you! > Peilin Ye > > drivers/video/console/sticore.c | 10 +++++----- > drivers/video/fbdev/atafb.c | 8 -------- > drivers/video/fbdev/core/fbcon.c | 5 ++--- > include/linux/font.h | 3 ++- > 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 + > 17 files changed, 22 insertions(+), 17 deletions(-) > > -- > 2.25.1 >
Hi Daniel, Hi Greg, Hi all, We are planning to use `font_desc` instead of `console_font` in `vc_data`, and this is just some prep work for it. It doesn't do much, but at least it removes two "FIXME"s in fbcon.c :) Peilin Ye (5): [1/5] fbdev/atafb: Remove unused extern variables Searching for `fontdata` gave me this in fbdev/atafb.c: extern unsigned char fontdata_8x8[]; extern unsigned char fontdata_8x16[]; ...which freaked me out, since in 6735b4632def ("Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts") I changed them from char arrays to structures, in lib/fonts/. Fortunately it turns out these extern variables have nothing to do with lib/fonts/, and are not being used anywhere, so remove them for less confusion. m68k cross-compiled. [2/5] Fonts: Make font size unsigned in font_desc Our goal is to use `font_desc` "everywhere" in the kernel, and signed `width` and `height` is inappropriate. Also, change some printk() format identifiers in console/sticore.c from `%d` to `%u`. parisc cross-compiled. [3/5] Fonts: Add charcount field to font_desc Add `unsigned int charcount` to `font_desc`, and update each of our 13 built-in fonts. [4/5] fbcon: Avoid hard-coding built-in font charcount [5/5] parisc/sticore: Avoid hard-coding built-in font charcount Everyone (tty, fbcon, sticore, etc.) is assuming that all built-in fonts have 256 characters, and is using hard-coded 256 or 255 everywhere. These two patches removes some of them. [5/5] is parisc cross-compiled. Now is a good time to review all find_font() and get_default_font() callers: drivers/media/pci/solo6x10/solo6x10-enc.c 133 const struct font_desc *vga = find_font("VGA8x16"); drivers/media/test-drivers/vimc/vimc-core.c 268 const struct font_desc *font = find_font("VGA8x16"); drivers/media/test-drivers/vivid/vivid-core.c 1928 const struct font_desc *font = find_font("VGA8x16"); drivers/usb/misc/sisusbvga/sisusb.c 2285 myfont = find_font("VGA8x16"); * These 4 only care about font VGA8x16, so let them be for now; drivers/video/console/sticore.c 499 fbfont = find_font(fbfont_name); drivers/video/console/sticore.c 501 fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0); * Uses 255 and 256, (hopefully) cleaned up by [5/5]; drivers/video/fbdev/core/fbcon.c 999 if (!fontname[0] || !(font = find_font(fontname))) drivers/video/fbdev/core/fbcon.c 1000 font = get_default_font(info->var.xres, drivers/video/fbdev/core/fbcon.c 1078 if (!fontname[0] || !(font = find_font(fontname))) drivers/video/fbdev/core/fbcon.c 1079 font = get_default_font(info->var.xres, * Use 256, cleaned up by [4/5]; drivers/video/fbdev/core/fbcon.c 2548 else if (!(f = find_font(name))) drivers/video/fbdev/core/fbcon.c 2546 f = get_default_font(info->var.xres, info->var.yres, * Uses 256 but no easy fix. I'll clean this up after making fbcon_do_set_font() pass a `font_desc` as parameter; drivers/firmware/efi/earlycon.c 234 font = get_default_font(xres, yres, -1, -1); * Does not care about charcount. Thank you! Peilin Ye drivers/video/console/sticore.c | 10 +++++----- drivers/video/fbdev/atafb.c | 8 -------- drivers/video/fbdev/core/fbcon.c | 5 ++--- include/linux/font.h | 3 ++- 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 + 17 files changed, 22 insertions(+), 17 deletions(-)