Re: [PATCH 32/45] tty: vt: use enum for VESA blanking modes

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 10003 bytes --]

Hi

Am 18.01.24 um 08:57 schrieb Jiri Slaby (SUSE):
> Switch VESA macros to an enum and add and use VESA_BLANK_MAX. This
> improves type checking in consw::con_blank().
> 
> There is a downside of this. The macros were defined twice: in
> linux/console.h and uapi/linux/fb.h. We cannot remove the latter (uapi
> header), but nor we want to expand them in the kernel too. So protect
> them using __KERNEL__. In the kernel case, include linux/console.h
> instead. This header dependency is preexisting.
> 
> Alternatively, we could create a vesa.h header with that sole enum and
> include it. If it turns out linux/console.h is too much for fb.h.

Personally I'd prefer something like include/uapi/video/vesa.h that 
contains the current defines. Fbdev is deprecated and the more we can 
avoid building upon it, the better. If you want an enum type in the 
kernel, maybe create it from the constants like this:

enum vesa_blank_mode {
	VESA_BLANK_MODE_NO_BLANKING = VESA_NO_BLANKING,
	VESA_BLANK_MODE_VSYNC = VESA_VSYNC_SYSPEND,
	[...]
	VESA_MAX_BLANK_MODE = ...,
};

Best regards
Thomas

> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Helge Deller <deller@gmx.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: linux-fbdev@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-parisc@vger.kernel.org
> ---
>   drivers/tty/vt/vt.c                 |  4 ++--
>   drivers/video/console/dummycon.c    |  6 ++++--
>   drivers/video/console/mdacon.c      |  3 ++-
>   drivers/video/console/newport_con.c |  3 ++-
>   drivers/video/console/sticon.c      |  3 ++-
>   drivers/video/console/vgacon.c      |  7 ++++---
>   drivers/video/fbdev/core/fbcon.c    |  3 ++-
>   include/linux/console.h             | 18 +++++++++++-------
>   include/uapi/linux/fb.h             |  5 ++++-
>   9 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 6f46fefedcfb..756291f37d47 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -175,7 +175,7 @@ int do_poke_blanked_console;
>   int console_blanked;
>   EXPORT_SYMBOL(console_blanked);
>   
> -static int vesa_blank_mode; /* 0:none 1:suspendV 2:suspendH 3:powerdown */
> +static enum vesa_blank_mode vesa_blank_mode;
>   static int vesa_off_interval;
>   static int blankinterval;
>   core_param(consoleblank, blankinterval, int, 0444);
> @@ -4334,7 +4334,7 @@ static int set_vesa_blanking(u8 __user *mode_user)
>   		return -EFAULT;
>   
>   	console_lock();
> -	vesa_blank_mode = (mode < 4) ? mode : VESA_NO_BLANKING;
> +	vesa_blank_mode = (mode <= VESA_BLANK_MAX) ? mode : VESA_NO_BLANKING;
>   	console_unlock();
>   
>   	return 0;
> diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
> index c8d5aa0e3ed0..d86c1d798690 100644
> --- a/drivers/video/console/dummycon.c
> +++ b/drivers/video/console/dummycon.c
> @@ -79,7 +79,8 @@ static void dummycon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>   	raw_notifier_call_chain(&dummycon_output_nh, 0, NULL);
>   }
>   
> -static int dummycon_blank(struct vc_data *vc, int blank, int mode_switch)
> +static int dummycon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
> +			  int mode_switch)
>   {
>   	/* Redraw, so that we get putc(s) for output done while blanked */
>   	return 1;
> @@ -89,7 +90,8 @@ static void dummycon_putc(struct vc_data *vc, u16 c, unsigned int y,
>   			  unsigned int x) { }
>   static void dummycon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>   			   unsigned int ypos, unsigned int xpos) { }
> -static int dummycon_blank(struct vc_data *vc, int blank, int mode_switch)
> +static int dummycon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
> +			  int mode_switch)
>   {
>   	return 0;
>   }
> diff --git a/drivers/video/console/mdacon.c b/drivers/video/console/mdacon.c
> index 4485ef923bb3..63e3ce678aab 100644
> --- a/drivers/video/console/mdacon.c
> +++ b/drivers/video/console/mdacon.c
> @@ -451,7 +451,8 @@ static bool mdacon_switch(struct vc_data *c)
>   	return true;	/* redrawing needed */
>   }
>   
> -static int mdacon_blank(struct vc_data *c, int blank, int mode_switch)
> +static int mdacon_blank(struct vc_data *c, enum vesa_blank_mode blank,
> +			int mode_switch)
>   {
>   	if (mda_type == TYPE_MDA) {
>   		if (blank)
> diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
> index ad3a09142770..38437a53b7f1 100644
> --- a/drivers/video/console/newport_con.c
> +++ b/drivers/video/console/newport_con.c
> @@ -476,7 +476,8 @@ static bool newport_switch(struct vc_data *vc)
>   	return true;
>   }
>   
> -static int newport_blank(struct vc_data *c, int blank, int mode_switch)
> +static int newport_blank(struct vc_data *c, enum vesa_blank_mode blank,
> +			 int mode_switch)
>   {
>   	unsigned short treg;
>   
> diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
> index 817b89c45e81..e9d5d1f92883 100644
> --- a/drivers/video/console/sticon.c
> +++ b/drivers/video/console/sticon.c
> @@ -298,7 +298,8 @@ static bool sticon_switch(struct vc_data *conp)
>       return true;	/* needs refreshing */
>   }
>   
> -static int sticon_blank(struct vc_data *c, int blank, int mode_switch)
> +static int sticon_blank(struct vc_data *c, enum vesa_blank_mode blank,
> +			int mode_switch)
>   {
>       if (blank == VESA_NO_BLANKING) {
>   	if (mode_switch)
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index 910dc73874b7..a4bd97ab502d 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -81,7 +81,7 @@ static unsigned int	vga_video_num_lines;			/* Number of text lines */
>   static bool		vga_can_do_color;			/* Do we support colors? */
>   static unsigned int	vga_default_font_height __read_mostly;	/* Height of default screen font */
>   static unsigned char	vga_video_type		__read_mostly;	/* Card type */
> -static int		vga_vesa_blanked;
> +static enum vesa_blank_mode vga_vesa_blanked;
>   static bool 		vga_palette_blanked;
>   static bool 		vga_is_gfx;
>   static bool 		vga_512_chars;
> @@ -683,7 +683,7 @@ static struct {
>   	unsigned char ClockingMode;	/* Seq-Controller:01h */
>   } vga_state;
>   
> -static void vga_vesa_blank(struct vgastate *state, int mode)
> +static void vga_vesa_blank(struct vgastate *state, enum vesa_blank_mode mode)
>   {
>   	/* save original values of VGA controller registers */
>   	if (!vga_vesa_blanked) {
> @@ -797,7 +797,8 @@ static void vga_pal_blank(struct vgastate *state)
>   	}
>   }
>   
> -static int vgacon_blank(struct vc_data *c, int blank, int mode_switch)
> +static int vgacon_blank(struct vc_data *c, enum vesa_blank_mode blank,
> +			int mode_switch)
>   {
>   	switch (blank) {
>   	case VESA_NO_BLANKING:		/* Unblank */
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index d5d924225209..69be5f2106bc 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2198,7 +2198,8 @@ static void fbcon_generic_blank(struct vc_data *vc, struct fb_info *info,
>   	}
>   }
>   
> -static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
> +static int fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
> +		       int mode_switch)
>   {
>   	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
> diff --git a/include/linux/console.h b/include/linux/console.h
> index f7c6b5fc3a36..5ea984b8c5e4 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -31,6 +31,15 @@ enum con_scroll {
>   	SM_DOWN,
>   };
>   
> +/* Note: fbcon defines the below as macros for userspace (in fb.h). */
> +enum vesa_blank_mode {
> +	VESA_NO_BLANKING	= 0,
> +	VESA_VSYNC_SUSPEND	= 1,
> +	VESA_HSYNC_SUSPEND	= 2,
> +	VESA_POWERDOWN		= VESA_VSYNC_SUSPEND | VESA_HSYNC_SUSPEND,
> +	VESA_BLANK_MAX		= VESA_POWERDOWN,
> +};
> +
>   enum vc_intensity;
>   
>   /**
> @@ -69,7 +78,8 @@ struct consw {
>   			unsigned int bottom, enum con_scroll dir,
>   			unsigned int lines);
>   	bool	(*con_switch)(struct vc_data *vc);
> -	int	(*con_blank)(struct vc_data *vc, int blank, int mode_switch);
> +	int	(*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
> +			     int mode_switch);
>   	int	(*con_font_set)(struct vc_data *vc, struct console_font *font,
>   			unsigned int vpitch, unsigned int flags);
>   	int	(*con_font_get)(struct vc_data *vc, struct console_font *font,
> @@ -520,12 +530,6 @@ void vcs_remove_sysfs(int index);
>    */
>   extern atomic_t ignore_console_lock_warning;
>   
> -/* VESA Blanking Levels */
> -#define VESA_NO_BLANKING        0
> -#define VESA_VSYNC_SUSPEND      1
> -#define VESA_HSYNC_SUSPEND      2
> -#define VESA_POWERDOWN          3
> -
>   extern void console_init(void);
>   
>   /* For deferred console takeover */
> diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
> index 3a49913d006c..562bdbb76ad9 100644
> --- a/include/uapi/linux/fb.h
> +++ b/include/uapi/linux/fb.h
> @@ -294,11 +294,14 @@ struct fb_con2fbmap {
>   };
>   
>   /* VESA Blanking Levels */
> +#ifdef __KERNEL__
> +#include <linux/console.h>
> +#else
>   #define VESA_NO_BLANKING        0
>   #define VESA_VSYNC_SUSPEND      1
>   #define VESA_HSYNC_SUSPEND      2
>   #define VESA_POWERDOWN          3
> -
> +#endif
>   
>   enum {
>   	/* screen: unblanked, hsync: on,  vsync: on */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]