Received-SPF: pass (google.com: domain of linux-kernel+bounces-68639-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 16 Feb 2024 14:26:38 +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>,
	Samuel Thibault <samuel.thibault@ens-lyon.org>
Subject: Re: [RFC PATCH v1 0/5] VT: Add ability to get font requirements
Message-ID: <Zc9ijvUofv4PCLw_@example.org>
References: <cover.1708011391.git.legion@kernel.org>
 <d7743747-1ec2-4557-9f2f-4cffd77284b3@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: <d7743747-1ec2-4557-9f2f-4cffd77284b3@kernel.org>

On Fri, Feb 16, 2024 at 08:21:38AM +0100, Jiri Slaby wrote:
> On 15. 02. 24, 16:37, Alexey Gladkov wrote:
> > We now have KD_FONT_OP_SET_TALL, but in fact such large fonts cannot be
> > loaded. No console driver supports tall fonts.
> 
> I thought fbcon can, no? If not, we should likely remove all the 
> KD_FONT_OP_SET_TALL checks here and there.

I thought so too until kbd users started trying to use such fonts. A month
after adding KD_FONT_OP_SET_TALL, support for large fonts was turned off
in fbcon:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=2b09d5d364986f724f17001ccfe4126b9b43a0be

But I don't think we need to remove KD_FONT_OP_SET_TALL completely. Maybe
support for large fonts can be fixed.

I suggested GET_INFO to solve the problem in general. Even if there is no
SET_TALL, the problem still remains. For example newport only supports
8x16 fonts.

> > Unfortunately, userspace
> > cannot distinguish the lack of support in the driver from errors in the
> > font itself. In all cases, EINVAL will be returned.
> 
> Yeah, AFAIR userspace just tries many possibilities and sees what trial 
> worked.

Yep. In case of big font, I don’t know how to improve the error message in
setfont. The EINVAL is very confusing for users.

> > How about adding another operator to get the supported font size so that
> > userspace can handle this situation correctly?
> 
> The whole font interface is horrid (and we got rid of v1 interface only 
> recently). Like (ab)using height = vpitch in KD_FONT_OP_SET_TALL :(.

Yep.

> So perhaps, as a band-aid, this might be fine (note you give no 
> opportunity to find out supported vpitch for example).

The vpitch can be 32 or if the height is greater, then vpitch = fnt_height

> Eventually, we need to invent a v3 interface with some better font_op
> struct with reserved fields for future use and so on.

Yes, yes, yes! Can we discuss this, pleeeeese? :)

> 
> > I mean something like this proof-of-concept.
> > 
> > ---
> > 
> > Alexey Gladkov (5):
> >    VT: Add KD_FONT_OP_GET_INFO operation
> >    newport_con: Allow to get max font width and height
> >    sticon: Allow to get max font width and height
> >    vgacon: Allow to get max font width and height
> >    fbcon: Allow to get max font width and height
> > 
> >   drivers/tty/vt/vt.c                 | 27 +++++++++++++++++++++++++++
> >   drivers/tty/vt/vt_ioctl.c           |  2 +-
> >   drivers/video/console/newport_con.c | 21 +++++++++++++++++----
> >   drivers/video/console/sticon.c      | 21 +++++++++++++++++++--
> >   drivers/video/console/vgacon.c      | 17 ++++++++++++++++-
> >   drivers/video/fbdev/core/fbcon.c    | 18 +++++++++++++++++-
> >   include/linux/console.h             |  1 +
> >   include/uapi/linux/kd.h             |  1 +
> >   8 files changed, 99 insertions(+), 9 deletions(-)
> > 
> 
> -- 
> js
> suse labs
> 

-- 
Rgrds, legion