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-validatio...
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