Received-SPF: pass (google.com: domain of linux-kernel+bounces-68613-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 16 Feb 2024 14:03:29 +0100
From: Alexey Gladkov <legion@kernel.org>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [RFC PATCH v1 3/5] sticon: Allow to get max font width and height
Message-ID: <Zc9dIRLbna585cGM@example.org>
References: <cover.1708011391.git.legion@kernel.org>
 <0c724c30a36aefe2cb4470182cf868d256fbabcd.1708011391.git.legion@kernel.org>
 <7c91e5e0-a6eb-4d53-89ed-efde04f57c7c@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7c91e5e0-a6eb-4d53-89ed-efde04f57c7c@kernel.org>

On Fri, Feb 16, 2024 at 08:10:15AM +0100, Jiri Slaby wrote:
> On 15. 02. 24, 16:37, Alexey Gladkov wrote:
> > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> > ---
> >   drivers/video/console/sticon.c | 21 +++++++++++++++++++--
> >   1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
> > index 992a4fa431aa..63368b3ff9c5 100644
> > --- a/drivers/video/console/sticon.c
> > +++ b/drivers/video/console/sticon.c
> > @@ -56,6 +56,11 @@
> >   #define BLANK 0
> >   static int vga_is_gfx;
> >   
> > +#define STICON_MIN_FONT_WIDTH 6
> > +#define STICON_MIN_FONT_HEIGHT 6
> > +#define STICON_MAX_FONT_WIDTH 32
> > +#define STICON_MAX_FONT_HEIGHT 32
> > +
> >   #define STI_DEF_FONT	sticon_sti->font
> >   
> >   /* borrowed from fbcon.c */
> > @@ -180,8 +185,10 @@ static int sticon_set_font(struct vc_data *vc, struct console_font *op,
> >   	struct sti_cooked_font *cooked_font;
> >   	unsigned char *data = op->data, *p;
> >   
> > -	if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
> > -	    || (op->charcount != 256 && op->charcount != 512))
> > +	if ((w < STICON_MIN_FONT_WIDTH) || (h < STICON_MIN_FONT_HEIGHT) ||
> > +	    (w > STICON_MAX_FONT_WIDTH) || (h > STICON_MAX_FONT_HEIGHT) ||
> 
> Will
> !in_range(w, STICON_MIN_FONT_WIDTH, STICON_MAX_FONT_WIDTH)
> look better?

Sure! Much better. I made this patch to illustrate what I mean. I will
finish it if you find the idea itself acceptable.

> > +	    (vpitch != 32) ||
> > +	    (op->charcount != 256 && op->charcount != 512))
> >   		return -EINVAL;
> >   	pitch = ALIGN(w, 8) / 8;
> >   	bpc = pitch * h;
> > @@ -273,6 +280,15 @@ static int sticon_font_set(struct vc_data *vc, struct console_font *font,
> >   	return sticon_set_font(vc, font, vpitch);
> >   }
> >   
> > +static int sticon_font_info(struct vc_data *vc, struct console_font *font)
> > +{
> > +	font->width = STICON_MAX_FONT_WIDTH;
> > +	font->height = STICON_MAX_FONT_HEIGHT;
> > +	font->charcount = 512;
> 
> Why not to have a macro for this constant?

Because I don’t know how to correctly transfer this limit to userspace.
I mean, we have the code:

if (op->charcount != 256 && op->charcount != 512)
	return -EINVAL;

So 512 is not exactly the maximum number of glyphs. And such checks in the
code begin to look very strange.

if (op->charcount != 256 && op->charcount != STICON_MAX_FONT_GLYPHS)
	return -EINVAL;

So I decided not to give the value 512 a name.

In fact, I think this should be passed to userspace as a binary flag.
Something like this:

struct console_font_info {
	unsigned int min_width, min_height;
	unsigned int max_width, max_height;
	unsigned int fnt_flags;
}

enum console_font_flags {
	FNT_FLAG_LOW_SIZE  = 1U << 0, // 256
	FNT_FLAG_HIGH_SIZE = 1U << 1, // 512
};

> > +
> > +	return 0;
> > +}
> > +
> >   static void sticon_init(struct vc_data *c, int init)
> >   {
> >       struct sti_struct *sti = sticon_sti;
> > @@ -371,6 +387,7 @@ static const struct consw sti_con = {
> >   	.con_scroll		= sticon_scroll,
> >   	.con_switch		= sticon_switch,
> >   	.con_blank		= sticon_blank,
> > +	.con_font_info		= sticon_font_info,
> >   	.con_font_set		= sticon_font_set,
> >   	.con_font_default	= sticon_font_default,
> >   	.con_build_attr		= sticon_build_attr,
> 
> -- 
> js
> suse labs
> 

-- 
Rgrds, legion