Following up

From: Peilin Ye <yepeilin.cs@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Following up
Date: Tue, 27 Oct 2020 12:50:21 -0400	[thread overview]
Message-ID: <20201027165021.GA1178130@PWN> (raw)
In-Reply-To: <cover.1603788511.git.yepeilin.cs@gmail.com>

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