Linux-fbdev Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Peilin Ye <yepeilin.cs@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Daniel Vetter <daniel@ffwll.ch>,
	Jiri Slaby <jirislaby@kernel.org>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-fbdev@vger.kernel.org
Subject: Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c
Date: Mon, 16 Nov 2020 11:09:49 +0100
Message-ID: <20201116100949.GA401619@phenom.ffwll.local> (raw)
In-Reply-To: <20201114124716.GA12895@PWN>

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

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-12 12:02 Peilin Ye
2020-11-12 12:04 ` [PATCH 1/5] console: Delete unused con_font_copy() callback implementations Peilin Ye
2020-11-12 12:11 ` [PATCH v3 2/5] console: Delete dummy con_font_set() and con_font_default() " Peilin Ye
2020-11-12 12:13 ` [PATCH v3 3/5] Fonts: Add charcount field to font_desc Peilin Ye
2020-11-12 12:14 ` [PATCH v3 4/5] parisc/sticore: Avoid hard-coding built-in font charcount Peilin Ye
2020-11-12 12:15 ` [PATCH v3 RFC 5/5] fbcon: Avoid using FNTCHARCNT() and hard-coded " Peilin Ye
2020-11-13 21:16 ` [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c Daniel Vetter
2020-11-13 22:47   ` Greg Kroah-Hartman
2020-11-14  8:10     ` Peilin Ye
2020-11-14 12:18       ` Greg Kroah-Hartman
2020-11-14 12:22         ` Greg Kroah-Hartman
2020-11-14 12:47           ` Peilin Ye
2020-11-16 10:09             ` Daniel Vetter [this message]
2020-11-17  6:21               ` Peilin Ye
2020-11-19  8:32           ` Peilin Ye
2020-11-19 15:10             ` Daniel Vetter
2020-11-20  8:37               ` Peilin Ye
2020-11-16 15:33     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201116100949.GA401619@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=yepeilin.cs@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-fbdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fbdev/0 linux-fbdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fbdev linux-fbdev/ https://lore.kernel.org/linux-fbdev \
		linux-fbdev@vger.kernel.org
	public-inbox-index linux-fbdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fbdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git