mbox series

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

Message ID cover.1605169912.git.yepeilin.cs@gmail.com (mailing list archive)
Headers show
Series console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c | expand

Message

Peilin Ye Nov. 12, 2020, 12:02 p.m. UTC
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(-)

Comments

Daniel Vetter Nov. 13, 2020, 9:16 p.m. UTC | #1
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
>
Greg Kroah-Hartman Nov. 13, 2020, 10:47 p.m. UTC | #2
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>
Peilin Ye Nov. 14, 2020, 8:10 a.m. UTC | #3
> 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
Greg Kroah-Hartman Nov. 14, 2020, 12:18 p.m. UTC | #4
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
Greg Kroah-Hartman Nov. 14, 2020, 12:22 p.m. UTC | #5
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.
Peilin Ye Nov. 14, 2020, 12:47 p.m. UTC | #6
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
Daniel Vetter Nov. 16, 2020, 10:09 a.m. UTC | #7
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 Nov. 16, 2020, 3:33 p.m. UTC | #8
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
Peilin Ye Nov. 17, 2020, 6:21 a.m. UTC | #9
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
Peilin Ye Nov. 19, 2020, 8:32 a.m. UTC | #10
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
Daniel Vetter Nov. 19, 2020, 3:10 p.m. UTC | #11
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
Peilin Ye Nov. 20, 2020, 8:37 a.m. UTC | #12
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