[v5,2/3] VT: Add KDFONTINFO ioctl

Commit Message

Alexey Gladkov April 17, 2024, 5:37 p.m. UTC

Each driver has its own restrictions on font size. There is currently no
way to understand what the requirements are. The new ioctl allows
userspace to get the minimum and maximum font size values.

Acked-by: Helge Deller <[e-mail]>
Signed-off-by: Alexey Gladkov <[e-mail]>
---
 drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
 drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
 include/linux/console.h   |  3 +++
 include/linux/vt_kern.h   |  1 +
 include/uapi/linux/kd.h   | 14 ++++++++++++++
 5 files changed, 55 insertions(+)

Comments

Helge Deller April 17, 2024, 7:31 p.m. UTC | #1

On 4/17/24 19:37, Alexey Gladkov wrote:
> Each driver has its own restrictions on font size. There is currently no
> way to understand what the requirements are. The new ioctl allows
> userspace to get the minimum and maximum font size values.
>
> Acked-by: Helge Deller <[e-mail]>
> Signed-off-by: Alexey Gladkov <[e-mail]>
> ---
>   drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
>   drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
>   include/linux/console.h   |  3 +++
>   include/linux/vt_kern.h   |  1 +
>   include/uapi/linux/kd.h   | 14 ++++++++++++++
>   5 files changed, 55 insertions(+)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9b5b98dfc8b4..e8db0e9ea674 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
>   	return -ENOSYS;
>   }
>
> +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> +	int rc;
> +
> +	info->min_height = 0;
> +	info->max_height = max_font_height;
> +
> +	info->min_width = 0;
> +	info->max_width = max_font_width;
> +
> +	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> +	console_lock();
> +	if (vc->vc_mode != KD_TEXT)
> +		rc = -EINVAL;
> +	else if (vc->vc_sw->con_font_info)
> +		rc = vc->vc_sw->con_font_info(vc, info);
> +	else
> +		rc = -ENOSYS;
> +	console_unlock();
> +
> +	return rc;
> +}
> +
>   /*
>    *	Interface exported to selection and vcs.
>    */
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 4b91072f3a4e..9a2f8081f650 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>   		break;
>   	}
>
> +	case KDFONTINFO: {
> +		struct console_font_info fnt_info;
> +
> +		memset(&fnt_info, 0, sizeof(fnt_info));
> +
> +		ret = con_font_info(vc, &fnt_info);
> +		if (ret)
> +			return ret;
> +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> +			return -EFAULT;
> +		break;
> +	}
> +
>   	default:
>   		return -ENOIOCTLCMD;
>   	}
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 31a8f5b85f5d..4b798322aa01 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -21,6 +21,7 @@
>   #include <linux/vesa.h>
>
>   struct vc_data;
> +struct console_font_info;
>   struct console_font_op;
>   struct console_font;
>   struct module;
> @@ -102,6 +103,8 @@ struct consw {
>   	bool	(*con_switch)(struct vc_data *vc);
>   	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
>   			     bool mode_switch);
> +	int	(*con_font_info)(struct vc_data *vc,
> +				 struct console_font_info *info);
>   	int	(*con_font_set)(struct vc_data *vc,
>   				const struct console_font *font,
>   				unsigned int vpitch, unsigned int flags);
> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index d008c3d0a9bb..383b3a4f6113 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
>   void do_unblank_screen(int leaving_gfx);
>   void poke_blanked_console(void);
>   int con_font_op(struct vc_data *vc, struct console_font_op *op);
> +int con_font_info(struct vc_data *vc, struct console_font_info *info);
>   int con_set_cmap(unsigned char __user *cmap);
>   int con_get_cmap(unsigned char __user *cmap);
>   void scrollback(struct vc_data *vc);
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index 8ddb2219a84b..68b715ad4d5c 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -185,6 +185,20 @@ struct console_font {
>
>   #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
>
> +/* font information */
> +
> +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */

Do we really need those bits?
You set a default min/max font size in con_font_info() above,
and all drivers can override those values.
So, there are always min/max sizes available.

> +struct console_font_info {
> +	__u32  flags;			/* KD_FONT_INFO_FLAG_* */

One space too much in front of "flags" ?

> +	__u32 min_width, min_height;	/* minimal font size */
> +	__u32 max_width, max_height;	/* maximum font size */
> +	__u32 reserved[5];		/* This field is reserved for future use. Must be 0. */
> +};
> +
> +#define KDFONTINFO	_IOR(KD_IOCTL_BASE, 0x73, struct console_font_info)
> +
>   /* note: 0x4B00-0x4B4E all have had a value at some time;
>      don't reuse for the time being */
>   /* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */

Greg KH April 18, 2024, 6:18 a.m. UTC | #2

On Wed, Apr 17, 2024 at 07:37:36PM +0200, Alexey Gladkov wrote:
> Each driver has its own restrictions on font size. There is currently no
> way to understand what the requirements are. The new ioctl allows
> userspace to get the minimum and maximum font size values.

Is there any userspace code that uses this yet that we can point to
here?

I know tty ioctls are woefully undocumented, but could there be some
documentation here?

> 
> Acked-by: Helge Deller <[e-mail]>
> Signed-off-by: Alexey Gladkov <[e-mail]>
> ---
>  drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
>  drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
>  include/linux/console.h   |  3 +++
>  include/linux/vt_kern.h   |  1 +
>  include/uapi/linux/kd.h   | 14 ++++++++++++++
>  5 files changed, 55 insertions(+)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9b5b98dfc8b4..e8db0e9ea674 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
>  	return -ENOSYS;
>  }
>  
> +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> +	int rc;
> +
> +	info->min_height = 0;
> +	info->max_height = max_font_height;
> +
> +	info->min_width = 0;
> +	info->max_width = max_font_width;
> +
> +	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> +	console_lock();
> +	if (vc->vc_mode != KD_TEXT)
> +		rc = -EINVAL;
> +	else if (vc->vc_sw->con_font_info)
> +		rc = vc->vc_sw->con_font_info(vc, info);
> +	else
> +		rc = -ENOSYS;
> +	console_unlock();
> +
> +	return rc;
> +}
> +
>  /*
>   *	Interface exported to selection and vcs.
>   */
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 4b91072f3a4e..9a2f8081f650 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>  		break;
>  	}
>  
> +	case KDFONTINFO: {
> +		struct console_font_info fnt_info;
> +
> +		memset(&fnt_info, 0, sizeof(fnt_info));
> +
> +		ret = con_font_info(vc, &fnt_info);

Shouldn't con_font_info() memset it first?  No need to do it in the
caller.

> +		if (ret)
> +			return ret;
> +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> +			return -EFAULT;
> +		break;
> +	}
> +
>  	default:
>  		return -ENOIOCTLCMD;
>  	}
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 31a8f5b85f5d..4b798322aa01 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -21,6 +21,7 @@
>  #include <linux/vesa.h>
>  
>  struct vc_data;
> +struct console_font_info;
>  struct console_font_op;
>  struct console_font;
>  struct module;
> @@ -102,6 +103,8 @@ struct consw {
>  	bool	(*con_switch)(struct vc_data *vc);
>  	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
>  			     bool mode_switch);
> +	int	(*con_font_info)(struct vc_data *vc,
> +				 struct console_font_info *info);

To make the names more obvious, how about:
	con_font_info_get()?

>  	int	(*con_font_set)(struct vc_data *vc,
>  				const struct console_font *font,
>  				unsigned int vpitch, unsigned int flags);
> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index d008c3d0a9bb..383b3a4f6113 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
>  void do_unblank_screen(int leaving_gfx);
>  void poke_blanked_console(void);
>  int con_font_op(struct vc_data *vc, struct console_font_op *op);
> +int con_font_info(struct vc_data *vc, struct console_font_info *info);
>  int con_set_cmap(unsigned char __user *cmap);
>  int con_get_cmap(unsigned char __user *cmap);
>  void scrollback(struct vc_data *vc);
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index 8ddb2219a84b..68b715ad4d5c 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -185,6 +185,20 @@ struct console_font {
>  
>  #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
>  
> +/* font information */
> +
> +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */

I don't understand why bit 0 and bit 1 have those comments after them.
That's confusing (i.e. bit 0 is NOT 256...)

> +
> +struct console_font_info {
> +	__u32  flags;			/* KD_FONT_INFO_FLAG_* */

Why are there flags if you are only setting these 2 values?  What are
the flags for?

If this is going to be a "multiplexed" type of structure, then make it a
union?  Or maybe we are totally over thinking this whole thing.

All you want is the min/max font size of the console, right?  So perhaps
the whole structure is just:

> +	__u32 min_width, min_height;	/* minimal font size */
> +	__u32 max_width, max_height;	/* maximum font size */

Those 4 variables?  Why have anything else here at all?  For any new
thing you wish to discover, have it be a new ioctl?

> +	__u32 reserved[5];		/* This field is reserved for future use. Must be 0. */

I understand the "must be 0" but this is a read-only structure, so
saying "it will be set to 0" might be better?"  Or something like that?

> +};
> +
> +#define KDFONTINFO	_IOR(KD_IOCTL_BASE, 0x73, struct console_font_info)

As mentioned above how about KDFONTINFOGET?

thanks,

greg k-h

Alexey Gladkov April 18, 2024, 10:27 a.m. UTC | #3

On Thu, Apr 18, 2024 at 08:18:33AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 17, 2024 at 07:37:36PM +0200, Alexey Gladkov wrote:
> > Each driver has its own restrictions on font size. There is currently no
> > way to understand what the requirements are. The new ioctl allows
> > userspace to get the minimum and maximum font size values.
> 
> Is there any userspace code that uses this yet that we can point to
> here?

Yes. I have a code that uses this. It waits for this ioctl to appear in
the kernel.

https://git.kernel.org/pub/scm/linux/kernel/git/legion/kbd.git/commit/?h=kdfontinfo-v1&id=e2ad0117ca8e46cedd8668934db7b04e9054d5d7

> I know tty ioctls are woefully undocumented, but could there be some
> documentation here?

Yes, this is a big problem for this interface. The ioctl_console(2)
describes PIO_FONT/PIO_FONTX, which is no longer supported, but does not
describe KDFONTOP at all, which is exactly used by userspace.

My TODO has a task to fix this.

But I would suggest creating documentation in the kernel because life
shows that man-page is far behind what is implemented.

> > 
> > Acked-by: Helge Deller <[e-mail]>
> > Signed-off-by: Alexey Gladkov <[e-mail]>
> > ---
> >  drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
> >  drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
> >  include/linux/console.h   |  3 +++
> >  include/linux/vt_kern.h   |  1 +
> >  include/uapi/linux/kd.h   | 14 ++++++++++++++
> >  5 files changed, 55 insertions(+)
> > 
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 9b5b98dfc8b4..e8db0e9ea674 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
> >  	return -ENOSYS;
> >  }
> >  
> > +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> > +{
> > +	int rc;
> > +
> > +	info->min_height = 0;
> > +	info->max_height = max_font_height;
> > +
> > +	info->min_width = 0;
> > +	info->max_width = max_font_width;
> > +
> > +	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> > +
> > +	console_lock();
> > +	if (vc->vc_mode != KD_TEXT)
> > +		rc = -EINVAL;
> > +	else if (vc->vc_sw->con_font_info)
> > +		rc = vc->vc_sw->con_font_info(vc, info);
> > +	else
> > +		rc = -ENOSYS;
> > +	console_unlock();
> > +
> > +	return rc;
> > +}
> > +
> >  /*
> >   *	Interface exported to selection and vcs.
> >   */
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index 4b91072f3a4e..9a2f8081f650 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> >  		break;
> >  	}
> >  
> > +	case KDFONTINFO: {
> > +		struct console_font_info fnt_info;
> > +
> > +		memset(&fnt_info, 0, sizeof(fnt_info));
> > +
> > +		ret = con_font_info(vc, &fnt_info);
> 
> Shouldn't con_font_info() memset it first?  No need to do it in the
> caller.
> 
> > +		if (ret)
> > +			return ret;
> > +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> > +			return -EFAULT;
> > +		break;
> > +	}
> > +
> >  	default:
> >  		return -ENOIOCTLCMD;
> >  	}
> > diff --git a/include/linux/console.h b/include/linux/console.h
> > index 31a8f5b85f5d..4b798322aa01 100644
> > --- a/include/linux/console.h
> > +++ b/include/linux/console.h
> > @@ -21,6 +21,7 @@
> >  #include <linux/vesa.h>
> >  
> >  struct vc_data;
> > +struct console_font_info;
> >  struct console_font_op;
> >  struct console_font;
> >  struct module;
> > @@ -102,6 +103,8 @@ struct consw {
> >  	bool	(*con_switch)(struct vc_data *vc);
> >  	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
> >  			     bool mode_switch);
> > +	int	(*con_font_info)(struct vc_data *vc,
> > +				 struct console_font_info *info);
> 
> To make the names more obvious, how about:
> 	con_font_info_get()?
> 
> >  	int	(*con_font_set)(struct vc_data *vc,
> >  				const struct console_font *font,
> >  				unsigned int vpitch, unsigned int flags);
> > diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> > index d008c3d0a9bb..383b3a4f6113 100644
> > --- a/include/linux/vt_kern.h
> > +++ b/include/linux/vt_kern.h
> > @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
> >  void do_unblank_screen(int leaving_gfx);
> >  void poke_blanked_console(void);
> >  int con_font_op(struct vc_data *vc, struct console_font_op *op);
> > +int con_font_info(struct vc_data *vc, struct console_font_info *info);
> >  int con_set_cmap(unsigned char __user *cmap);
> >  int con_get_cmap(unsigned char __user *cmap);
> >  void scrollback(struct vc_data *vc);
> > diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> > index 8ddb2219a84b..68b715ad4d5c 100644
> > --- a/include/uapi/linux/kd.h
> > +++ b/include/uapi/linux/kd.h
> > @@ -185,6 +185,20 @@ struct console_font {
> >  
> >  #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
> >  
> > +/* font information */
> > +
> > +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
> > +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
> 
> I don't understand why bit 0 and bit 1 have those comments after them.
> That's confusing (i.e. bit 0 is NOT 256...)
> 
> > +
> > +struct console_font_info {
> > +	__u32  flags;			/* KD_FONT_INFO_FLAG_* */
> 
> Why are there flags if you are only setting these 2 values?  What are
> the flags for?
> 
> If this is going to be a "multiplexed" type of structure, then make it a
> union?  Or maybe we are totally over thinking this whole thing.
> 
> All you want is the min/max font size of the console, right?  So perhaps
> the whole structure is just:
> 
> > +	__u32 min_width, min_height;	/* minimal font size */
> > +	__u32 max_width, max_height;	/* maximum font size */
> 
> Those 4 variables?  Why have anything else here at all?  For any new
> thing you wish to discover, have it be a new ioctl?
> 
> > +	__u32 reserved[5];		/* This field is reserved for future use. Must be 0. */
> 
> I understand the "must be 0" but this is a read-only structure, so
> saying "it will be set to 0" might be better?"  Or something like that?
> 
> > +};
> > +
> > +#define KDFONTINFO	_IOR(KD_IOCTL_BASE, 0x73, struct console_font_info)
> 
> As mentioned above how about KDFONTINFOGET?
> 
> thanks,
> 
> greg k-h
>

Alexey Gladkov April 18, 2024, 10:45 a.m. UTC | #4

On Wed, Apr 17, 2024 at 09:31:53PM +0200, Helge Deller wrote:
> On 4/17/24 19:37, Alexey Gladkov wrote:
> > Each driver has its own restrictions on font size. There is currently no
> > way to understand what the requirements are. The new ioctl allows
> > userspace to get the minimum and maximum font size values.
> >
> > Acked-by: Helge Deller <[e-mail]>
> > Signed-off-by: Alexey Gladkov <[e-mail]>
> > ---
> >   drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
> >   drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
> >   include/linux/console.h   |  3 +++
> >   include/linux/vt_kern.h   |  1 +
> >   include/uapi/linux/kd.h   | 14 ++++++++++++++
> >   5 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 9b5b98dfc8b4..e8db0e9ea674 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
> >   	return -ENOSYS;
> >   }
> >
> > +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> > +{
> > +	int rc;
> > +
> > +	info->min_height = 0;
> > +	info->max_height = max_font_height;
> > +
> > +	info->min_width = 0;
> > +	info->max_width = max_font_width;
> > +
> > +	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> > +
> > +	console_lock();
> > +	if (vc->vc_mode != KD_TEXT)
> > +		rc = -EINVAL;
> > +	else if (vc->vc_sw->con_font_info)
> > +		rc = vc->vc_sw->con_font_info(vc, info);
> > +	else
> > +		rc = -ENOSYS;
> > +	console_unlock();
> > +
> > +	return rc;
> > +}
> > +
> >   /*
> >    *	Interface exported to selection and vcs.
> >    */
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index 4b91072f3a4e..9a2f8081f650 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> >   		break;
> >   	}
> >
> > +	case KDFONTINFO: {
> > +		struct console_font_info fnt_info;
> > +
> > +		memset(&fnt_info, 0, sizeof(fnt_info));
> > +
> > +		ret = con_font_info(vc, &fnt_info);
> > +		if (ret)
> > +			return ret;
> > +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> > +			return -EFAULT;
> > +		break;
> > +	}
> > +
> >   	default:
> >   		return -ENOIOCTLCMD;
> >   	}
> > diff --git a/include/linux/console.h b/include/linux/console.h
> > index 31a8f5b85f5d..4b798322aa01 100644
> > --- a/include/linux/console.h
> > +++ b/include/linux/console.h
> > @@ -21,6 +21,7 @@
> >   #include <linux/vesa.h>
> >
> >   struct vc_data;
> > +struct console_font_info;
> >   struct console_font_op;
> >   struct console_font;
> >   struct module;
> > @@ -102,6 +103,8 @@ struct consw {
> >   	bool	(*con_switch)(struct vc_data *vc);
> >   	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
> >   			     bool mode_switch);
> > +	int	(*con_font_info)(struct vc_data *vc,
> > +				 struct console_font_info *info);
> >   	int	(*con_font_set)(struct vc_data *vc,
> >   				const struct console_font *font,
> >   				unsigned int vpitch, unsigned int flags);
> > diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> > index d008c3d0a9bb..383b3a4f6113 100644
> > --- a/include/linux/vt_kern.h
> > +++ b/include/linux/vt_kern.h
> > @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
> >   void do_unblank_screen(int leaving_gfx);
> >   void poke_blanked_console(void);
> >   int con_font_op(struct vc_data *vc, struct console_font_op *op);
> > +int con_font_info(struct vc_data *vc, struct console_font_info *info);
> >   int con_set_cmap(unsigned char __user *cmap);
> >   int con_get_cmap(unsigned char __user *cmap);
> >   void scrollback(struct vc_data *vc);
> > diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> > index 8ddb2219a84b..68b715ad4d5c 100644
> > --- a/include/uapi/linux/kd.h
> > +++ b/include/uapi/linux/kd.h
> > @@ -185,6 +185,20 @@ struct console_font {
> >
> >   #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
> >
> > +/* font information */
> > +
> > +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
> > +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
> 
> Do we really need those bits?
> You set a default min/max font size in con_font_info() above,
> and all drivers can override those values.
> So, there are always min/max sizes available.

These bits are not about the minimum and maximum glyph size, but about the
number of glyphs in the font.

Maybe this is an overkill, but sticon has this check:

if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
    || (op->charcount != 256 && op->charcount != 512))

[ to be honest, I don’t know why this driver doesn’t accept a glyph of
width 4 ]

I thought it would be worth fixing the maximum number of requirements in
the drivers since I started adding a new ioctl.

> > +struct console_font_info {
> > +	__u32  flags;			/* KD_FONT_INFO_FLAG_* */
> 
> One space too much in front of "flags" ?

No problem. I will fix.

> 
> > +	__u32 min_width, min_height;	/* minimal font size */
> > +	__u32 max_width, max_height;	/* maximum font size */
> > +	__u32 reserved[5];		/* This field is reserved for future use. Must be 0. */
> > +};
> > +
> > +#define KDFONTINFO	_IOR(KD_IOCTL_BASE, 0x73, struct console_font_info)
> > +
> >   /* note: 0x4B00-0x4B4E all have had a value at some time;
> >      don't reuse for the time being */
> >   /* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
>

Helge Deller April 25, 2024, 10:33 a.m. UTC | #5

On 4/18/24 12:45, Alexey Gladkov wrote:
> On Wed, Apr 17, 2024 at 09:31:53PM +0200, Helge Deller wrote:
>> On 4/17/24 19:37, Alexey Gladkov wrote:
>>> Each driver has its own restrictions on font size. There is currently no
>>> way to understand what the requirements are. The new ioctl allows
>>> userspace to get the minimum and maximum font size values.
>>>
>>> Acked-by: Helge Deller <[e-mail]>
>>> Signed-off-by: Alexey Gladkov <[e-mail]>
>>> ---
>>>    drivers/tty/vt/vt.c       | 24 ++++++++++++++++++++++++
>>>    drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
>>>    include/linux/console.h   |  3 +++
>>>    include/linux/vt_kern.h   |  1 +
>>>    include/uapi/linux/kd.h   | 14 ++++++++++++++
>>>    5 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>>> index 9b5b98dfc8b4..e8db0e9ea674 100644
>>> --- a/drivers/tty/vt/vt.c
>>> +++ b/drivers/tty/vt/vt.c
>>> @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
>>>    	return -ENOSYS;
>>>    }
>>>
>>> +int con_font_info(struct vc_data *vc, struct console_font_info *info)
>>> +{
>>> +	int rc;
>>> +
>>> +	info->min_height = 0;
>>> +	info->max_height = max_font_height;
>>> +
>>> +	info->min_width = 0;
>>> +	info->max_width = max_font_width;
>>> +
>>> +	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
>>> +
>>> +	console_lock();
>>> +	if (vc->vc_mode != KD_TEXT)
>>> +		rc = -EINVAL;
>>> +	else if (vc->vc_sw->con_font_info)
>>> +		rc = vc->vc_sw->con_font_info(vc, info);
>>> +	else
>>> +		rc = -ENOSYS;
>>> +	console_unlock();
>>> +
>>> +	return rc;
>>> +}
>>> +
>>>    /*
>>>     *	Interface exported to selection and vcs.
>>>     */
>>> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
>>> index 4b91072f3a4e..9a2f8081f650 100644
>>> --- a/drivers/tty/vt/vt_ioctl.c
>>> +++ b/drivers/tty/vt/vt_ioctl.c
>>> @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>>>    		break;
>>>    	}
>>>
>>> +	case KDFONTINFO: {
>>> +		struct console_font_info fnt_info;
>>> +
>>> +		memset(&fnt_info, 0, sizeof(fnt_info));
>>> +
>>> +		ret = con_font_info(vc, &fnt_info);
>>> +		if (ret)
>>> +			return ret;
>>> +		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>>> +			return -EFAULT;
>>> +		break;
>>> +	}
>>> +
>>>    	default:
>>>    		return -ENOIOCTLCMD;
>>>    	}
>>> diff --git a/include/linux/console.h b/include/linux/console.h
>>> index 31a8f5b85f5d..4b798322aa01 100644
>>> --- a/include/linux/console.h
>>> +++ b/include/linux/console.h
>>> @@ -21,6 +21,7 @@
>>>    #include <linux/vesa.h>
>>>
>>>    struct vc_data;
>>> +struct console_font_info;
>>>    struct console_font_op;
>>>    struct console_font;
>>>    struct module;
>>> @@ -102,6 +103,8 @@ struct consw {
>>>    	bool	(*con_switch)(struct vc_data *vc);
>>>    	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
>>>    			     bool mode_switch);
>>> +	int	(*con_font_info)(struct vc_data *vc,
>>> +				 struct console_font_info *info);
>>>    	int	(*con_font_set)(struct vc_data *vc,
>>>    				const struct console_font *font,
>>>    				unsigned int vpitch, unsigned int flags);
>>> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
>>> index d008c3d0a9bb..383b3a4f6113 100644
>>> --- a/include/linux/vt_kern.h
>>> +++ b/include/linux/vt_kern.h
>>> @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
>>>    void do_unblank_screen(int leaving_gfx);
>>>    void poke_blanked_console(void);
>>>    int con_font_op(struct vc_data *vc, struct console_font_op *op);
>>> +int con_font_info(struct vc_data *vc, struct console_font_info *info);
>>>    int con_set_cmap(unsigned char __user *cmap);
>>>    int con_get_cmap(unsigned char __user *cmap);
>>>    void scrollback(struct vc_data *vc);
>>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
>>> index 8ddb2219a84b..68b715ad4d5c 100644
>>> --- a/include/uapi/linux/kd.h
>>> +++ b/include/uapi/linux/kd.h
>>> @@ -185,6 +185,20 @@ struct console_font {
>>>
>>>    #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
>>>
>>> +/* font information */
>>> +
>>> +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
>>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
>>
>> Do we really need those bits?
>> You set a default min/max font size in con_font_info() above,
>> and all drivers can override those values.
>> So, there are always min/max sizes available.
>
> These bits are not about the minimum and maximum glyph size, but about the
> number of glyphs in the font.
>
> Maybe this is an overkill, but sticon has this check:
>
> if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
>      || (op->charcount != 256 && op->charcount != 512))
>
> [ to be honest, I don’t know why this driver doesn’t accept a glyph of
> width 4 ]

I think there was no technical limitation when I added that.
It's just that the font would be so small...

> I thought it would be worth fixing the maximum number of requirements in
> the drivers since I started adding a new ioctl.

Ok.

Helge

Alexey Gladkov April 25, 2024, 11:06 a.m. UTC | #6

On Thu, Apr 25, 2024 at 12:33:28PM +0200, Helge Deller wrote:
> >>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> >>> index 8ddb2219a84b..68b715ad4d5c 100644
> >>> --- a/include/uapi/linux/kd.h
> >>> +++ b/include/uapi/linux/kd.h
> >>> @@ -185,6 +185,20 @@ struct console_font {
> >>>
> >>>    #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
> >>>
> >>> +/* font information */
> >>> +
> >>> +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
> >>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
> >>
> >> Do we really need those bits?
> >> You set a default min/max font size in con_font_info() above,
> >> and all drivers can override those values.
> >> So, there are always min/max sizes available.
> >
> > These bits are not about the minimum and maximum glyph size, but about the
> > number of glyphs in the font.
> >
> > Maybe this is an overkill, but sticon has this check:
> >
> > if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
> >      || (op->charcount != 256 && op->charcount != 512))
> >
> > [ to be honest, I don’t know why this driver doesn’t accept a glyph of
> > width 4 ]
> 
> I think there was no technical limitation when I added that.
> It's just that the font would be so small...

If so, then I can remove min_height/min_width from the ioctl structure.
And most likely the flags can also be left empty since at the moment all
drivers support 512.

> > I thought it would be worth fixing the maximum number of requirements in
> > the drivers since I started adding a new ioctl.
> 
> Ok.
> 
> Helge
>

Helge Deller April 25, 2024, 11:35 a.m. UTC | #7

On 4/25/24 13:06, Alexey Gladkov wrote:
> On Thu, Apr 25, 2024 at 12:33:28PM +0200, Helge Deller wrote:
>>>>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
>>>>> index 8ddb2219a84b..68b715ad4d5c 100644
>>>>> --- a/include/uapi/linux/kd.h
>>>>> +++ b/include/uapi/linux/kd.h
>>>>> @@ -185,6 +185,20 @@ struct console_font {
>>>>>
>>>>>     #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
>>>>>
>>>>> +/* font information */
>>>>> +
>>>>> +#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
>>>>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
>>>>
>>>> Do we really need those bits?
>>>> You set a default min/max font size in con_font_info() above,
>>>> and all drivers can override those values.
>>>> So, there are always min/max sizes available.
>>>
>>> These bits are not about the minimum and maximum glyph size, but about the
>>> number of glyphs in the font.
>>>
>>> Maybe this is an overkill, but sticon has this check:
>>>
>>> if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
>>>       || (op->charcount != 256 && op->charcount != 512))
>>>
>>> [ to be honest, I don’t know why this driver doesn’t accept a glyph of
>>> width 4 ]
>>
>> I think there was no technical limitation when I added that.
>> It's just that the font would be so small...
>
> If so, then I can remove min_height/min_width from the ioctl structure.
> And most likely the flags can also be left empty since at the moment all
> drivers support 512.

Yes, I think that's ok.

Helge

Patch

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9b5b98dfc8b4..e8db0e9ea674 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4851,6 +4851,30 @@  int con_font_op(struct vc_data *vc, struct console_font_op *op)
 	return -ENOSYS;
 }
 
+int con_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+	int rc;
+
+	info->min_height = 0;
+	info->max_height = max_font_height;
+
+	info->min_width = 0;
+	info->max_width = max_font_width;
+
+	info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+	console_lock();
+	if (vc->vc_mode != KD_TEXT)
+		rc = -EINVAL;
+	else if (vc->vc_sw->con_font_info)
+		rc = vc->vc_sw->con_font_info(vc, info);
+	else
+		rc = -ENOSYS;
+	console_unlock();
+
+	return rc;
+}
+
 /*
  *	Interface exported to selection and vcs.
  */
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 4b91072f3a4e..9a2f8081f650 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -479,6 +479,19 @@  static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
 		break;
 	}
 
+	case KDFONTINFO: {
+		struct console_font_info fnt_info;
+
+		memset(&fnt_info, 0, sizeof(fnt_info));
+
+		ret = con_font_info(vc, &fnt_info);
+		if (ret)
+			return ret;
+		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
+			return -EFAULT;
+		break;
+	}
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/linux/console.h b/include/linux/console.h
index 31a8f5b85f5d..4b798322aa01 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -21,6 +21,7 @@ 
 #include <linux/vesa.h>
 
 struct vc_data;
+struct console_font_info;
 struct console_font_op;
 struct console_font;
 struct module;
@@ -102,6 +103,8 @@  struct consw {
 	bool	(*con_switch)(struct vc_data *vc);
 	bool	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
 			     bool mode_switch);
+	int	(*con_font_info)(struct vc_data *vc,
+				 struct console_font_info *info);
 	int	(*con_font_set)(struct vc_data *vc,
 				const struct console_font *font,
 				unsigned int vpitch, unsigned int flags);
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index d008c3d0a9bb..383b3a4f6113 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -33,6 +33,7 @@  void do_blank_screen(int entering_gfx);
 void do_unblank_screen(int leaving_gfx);
 void poke_blanked_console(void);
 int con_font_op(struct vc_data *vc, struct console_font_op *op);
+int con_font_info(struct vc_data *vc, struct console_font_info *info);
 int con_set_cmap(unsigned char __user *cmap);
 int con_get_cmap(unsigned char __user *cmap);
 void scrollback(struct vc_data *vc);
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 8ddb2219a84b..68b715ad4d5c 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -185,6 +185,20 @@  struct console_font {
 
 #define KD_FONT_FLAG_DONT_RECALC 	1	/* Don't recalculate hw charcell size [compat] */
 
+/* font information */
+
+#define KD_FONT_INFO_FLAG_LOW_SIZE	_BITUL(0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE	_BITUL(1) /* 512 */
+
+struct console_font_info {
+	__u32  flags;			/* KD_FONT_INFO_FLAG_* */
+	__u32 min_width, min_height;	/* minimal font size */
+	__u32 max_width, max_height;	/* maximum font size */
+	__u32 reserved[5];		/* This field is reserved for future use. Must be 0. */
+};
+
+#define KDFONTINFO	_IOR(KD_IOCTL_BASE, 0x73, struct console_font_info)
+
 /* note: 0x4B00-0x4B4E all have had a value at some time;
    don't reuse for the time being */
 /* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */