[PATCH 0/6] fbcon: Fix 'cursor_blink' sysfs attribute

[PATCH 0/6] fbcon: Fix 'cursor_blink' sysfs attribute

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make the 'cursor_blink' sysfs attribute actually usable from
udev rules. I also took the opportinity to some cleanups t
the code after getting annoyed by it.

Ville Syrjälä (6):
  fbcon: Make cursor_blink=0 work when configured before fb devices
    appear
  fbcon: Add sysfs attributes before registering the device
  fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink
  fbcon: fbcon_is_inactive() -> fbcon_is_active()
  fbcon: Introduce get_{fg,bg}_color()
  fbcon: Use 'bool' where appopriate

 drivers/video/fbdev/core/fbcon.c | 180 ++++++++++++-------------------
 1 file changed, 69 insertions(+), 111 deletions(-)

-- 
2.44.2

[PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently setting cursor_blink attribute to 0 before any fb
devices are around does absolutely nothing. When fb devices appear
and fbcon becomes active the cursor starts blinking. Fix the problem
by recoding the desired state of the attribute even if no fb devices
are present at the time.

Also adjust the show() method such that it shows the current
state of the attribute even when no fb devices are in use.

Note that store_cursor_blink() is still a bit dodgy:
- seems to be missing some of the other checks that we do
  elsewhere when deciding whether the cursor should be
  blinking or not
- when set to 0 when the cursor is currently invisible due
  to blinking, the cursor will remains invisible. We should
  either explicitly make it visible here, or wait until the
  full blink cycle has finished.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 34 +++++++-------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 2e093535884b..8936fa79b9e0 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -3217,26 +3217,7 @@ static ssize_t show_rotate(struct device *device,
 static ssize_t show_cursor_blink(struct device *device,
 				 struct device_attribute *attr, char *buf)
 {
-	struct fb_info *info;
-	struct fbcon_ops *ops;
-	int idx, blink = -1;
-
-	console_lock();
-	idx = con2fb_map[fg_console];
-
-	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
-		goto err;
-
-	info = fbcon_registered_fb[idx];
-	ops = info->fbcon_par;
-
-	if (!ops)
-		goto err;
-
-	blink = delayed_work_pending(&ops->cursor_work);
-err:
-	console_unlock();
-	return sysfs_emit(buf, "%d\n", blink);
+	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
 }
 
 static ssize_t store_cursor_blink(struct device *device,
@@ -3247,9 +3228,13 @@ static ssize_t store_cursor_blink(struct device *device,
 	int blink, idx;
 	char **last = NULL;
 
+	blink = simple_strtoul(buf, last, 0);
+
 	console_lock();
 	idx = con2fb_map[fg_console];
 
+	fbcon_cursor_noblink = !blink;
+
 	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
@@ -3258,15 +3243,10 @@ static ssize_t store_cursor_blink(struct device *device,
 	if (!info->fbcon_par)
 		goto err;
 
-	blink = simple_strtoul(buf, last, 0);
-
-	if (blink) {
-		fbcon_cursor_noblink = 0;
+	if (blink)
 		fbcon_add_cursor_work(info);
-	} else {
-		fbcon_cursor_noblink = 1;
+	else
 		fbcon_del_cursor_work(info);
-	}
 
 err:
 	console_unlock();
-- 
2.44.2

[PATCH 2/6] fbcon: Add sysfs attributes before registering the device

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently fbcon adds the attributes after registering the device,
which means the attributes may not be there by the time udev
gets the ADD uevent. Fix the race by switching over to
device_create_with_groups().

With this one can reliably turn off the power wasting cursor
blink with a udev rule such as:
ACTION=="add", SUBSYSTEM=="graphics", TEST=="cursor_blink", ATTR{cursor_blink}="0"

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 73 +++++++++-----------------------
 1 file changed, 19 insertions(+), 54 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 8936fa79b9e0..bbe332572ca7 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -160,7 +160,6 @@ static int info_idx = -1;
 
 /* console rotation */
 static int initial_rotation = -1;
-static int fbcon_has_sysfs;
 static int margin_color;
 
 static const struct consw fb_con;
@@ -188,8 +187,6 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
 static void fbcon_modechanged(struct fb_info *info);
 static void fbcon_set_all_vcs(struct fb_info *info);
 
-static struct device *fbcon_device;
-
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION
 static inline void fbcon_set_rotation(struct fb_info *info)
 {
@@ -3151,7 +3148,7 @@ static const struct consw fb_con = {
 	.con_debug_leave	= fbcon_debug_leave,
 };
 
-static ssize_t store_rotate(struct device *device,
+static ssize_t rotate_store(struct device *device,
 			    struct device_attribute *attr, const char *buf,
 			    size_t count)
 {
@@ -3173,7 +3170,7 @@ static ssize_t store_rotate(struct device *device,
 	return count;
 }
 
-static ssize_t store_rotate_all(struct device *device,
+static ssize_t rotate_all_store(struct device *device,
 				struct device_attribute *attr,const char *buf,
 				size_t count)
 {
@@ -3195,7 +3192,7 @@ static ssize_t store_rotate_all(struct device *device,
 	return count;
 }
 
-static ssize_t show_rotate(struct device *device,
+static ssize_t rotate_show(struct device *device,
 			   struct device_attribute *attr,char *buf)
 {
 	struct fb_info *info;
@@ -3214,13 +3211,13 @@ static ssize_t show_rotate(struct device *device,
 	return sysfs_emit(buf, "%d\n", rotate);
 }
 
-static ssize_t show_cursor_blink(struct device *device,
+static ssize_t cursor_blink_show(struct device *device,
 				 struct device_attribute *attr, char *buf)
 {
 	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
 }
 
-static ssize_t store_cursor_blink(struct device *device,
+static ssize_t cursor_blink_store(struct device *device,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
 {
@@ -3253,35 +3250,17 @@ static ssize_t store_cursor_blink(struct device *device,
 	return count;
 }
 
-static struct device_attribute device_attrs[] = {
-	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
-	__ATTR(rotate_all, S_IWUSR, NULL, store_rotate_all),
-	__ATTR(cursor_blink, S_IRUGO|S_IWUSR, show_cursor_blink,
-	       store_cursor_blink),
-};
-
-static int fbcon_init_device(void)
-{
-	int i, error = 0;
-
-	fbcon_has_sysfs = 1;
-
-	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
-		error = device_create_file(fbcon_device, &device_attrs[i]);
-
-		if (error)
-			break;
-	}
-
-	if (error) {
-		while (--i >= 0)
-			device_remove_file(fbcon_device, &device_attrs[i]);
-
-		fbcon_has_sysfs = 0;
-	}
+static DEVICE_ATTR_RW(rotate);
+static DEVICE_ATTR_WO(rotate_all);
+static DEVICE_ATTR_RW(cursor_blink);
 
-	return 0;
-}
+static struct attribute *device_attrs[] = {
+	&dev_attr_rotate.attr,
+	&dev_attr_rotate_all.attr,
+	&dev_attr_cursor_blink.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(device);
 
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
 static void fbcon_register_existing_fbs(struct work_struct *work)
@@ -3336,19 +3315,18 @@ static void fbcon_start(void)
 
 void __init fb_console_init(void)
 {
+	struct device *fbcon_device;
 	int i;
 
 	console_lock();
-	fbcon_device = device_create(fb_class, NULL, MKDEV(0, 0), NULL,
-				     "fbcon");
 
+	fbcon_device = device_create_with_groups(fb_class, NULL, MKDEV(0, 0),
+						 NULL, device_groups, "fbcon");
 	if (IS_ERR(fbcon_device)) {
 		printk(KERN_WARNING "Unable to create device "
 		       "for fbcon; errno = %ld\n",
 		       PTR_ERR(fbcon_device));
-		fbcon_device = NULL;
-	} else
-		fbcon_init_device();
+	}
 
 	for (i = 0; i < MAX_NR_CONSOLES; i++)
 		con2fb_map[i] = -1;
@@ -3359,18 +3337,6 @@ void __init fb_console_init(void)
 
 #ifdef MODULE
 
-static void __exit fbcon_deinit_device(void)
-{
-	int i;
-
-	if (fbcon_has_sysfs) {
-		for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
-			device_remove_file(fbcon_device, &device_attrs[i]);
-
-		fbcon_has_sysfs = 0;
-	}
-}
-
 void __exit fb_console_exit(void)
 {
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
@@ -3383,7 +3349,6 @@ void __exit fb_console_exit(void)
 #endif
 
 	console_lock();
-	fbcon_deinit_device();
 	device_destroy(fb_class, MKDEV(0, 0));
 
 	do_unregister_con_driver(&fb_con);
-- 
2.44.2

[PATCH 3/6] fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Invert fbcon_cursor_noblink into fbcon_cursor_blink so that:
- it matches the sysfs attribute exactly
- avoids having to do these NOT operations all over the place

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bbe332572ca7..eb30aa872371 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -166,7 +166,7 @@ static const struct consw fb_con;
 
 #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
 
-static int fbcon_cursor_noblink;
+static int fbcon_cursor_blink;
 
 #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
 
@@ -399,7 +399,7 @@ static void fbcon_add_cursor_work(struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!fbcon_cursor_noblink)
+	if (fbcon_cursor_blink)
 		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
 				   ops->cur_blink_jiffies);
 }
@@ -3214,7 +3214,7 @@ static ssize_t rotate_show(struct device *device,
 static ssize_t cursor_blink_show(struct device *device,
 				 struct device_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
+	return sysfs_emit(buf, "%d\n", fbcon_cursor_blink);
 }
 
 static ssize_t cursor_blink_store(struct device *device,
@@ -3230,7 +3230,7 @@ static ssize_t cursor_blink_store(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	fbcon_cursor_noblink = !blink;
+	fbcon_cursor_blink = blink;
 
 	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
-- 
2.44.2

[PATCH 4/6] fbcon: fbcon_is_inactive() -> fbcon_is_active()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Invert fbcon_is_inactive() into fbcon_is_active(). Much easier
on the poor brain when you don't have to do dobule negations
all over the place.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index eb30aa872371..2a78cca3e9de 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -281,12 +281,12 @@ static bool fbcon_skip_panic(struct fb_info *info)
 #endif
 }
 
-static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
+static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	return (info->state != FBINFO_STATE_RUNNING ||
-		vc->vc_mode != KD_TEXT || ops->graphics || fbcon_skip_panic(info));
+	return info->state == FBINFO_STATE_RUNNING &&
+		vc->vc_mode == KD_TEXT && !ops->graphics && !fbcon_skip_panic(info);
 }
 
 static int get_color(struct vc_data *vc, struct fb_info *info,
@@ -1253,7 +1253,7 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	u_int y_break;
 
-	if (fbcon_is_inactive(vc, info))
+	if (!fbcon_is_active(vc, info))
 		return;
 
 	if (!height || !width)
@@ -1295,7 +1295,7 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!fbcon_is_inactive(vc, info))
+	if (fbcon_is_active(vc, info))
 		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
 			   get_color(vc, info, scr_readw(s), 1),
 			   get_color(vc, info, scr_readw(s), 0));
@@ -1306,7 +1306,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
 	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!fbcon_is_inactive(vc, info))
+	if (fbcon_is_active(vc, info))
 		ops->clear_margins(vc, info, margin_color, bottom_only);
 }
 
@@ -1318,7 +1318,7 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
 
 	ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
 
-	if (fbcon_is_inactive(vc, info) || vc->vc_deccm != 1)
+	if (!fbcon_is_active(vc, info) || vc->vc_deccm != 1)
 		return;
 
 	if (vc->vc_cursor_type & CUR_SW)
@@ -1724,7 +1724,7 @@ static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
 	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
-	if (fbcon_is_inactive(vc, info))
+	if (!fbcon_is_active(vc, info))
 		return;
 
 	if (!width || !height)
@@ -1748,7 +1748,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
 
-	if (fbcon_is_inactive(vc, info))
+	if (!fbcon_is_active(vc, info))
 		return true;
 
 	fbcon_cursor(vc, false);
@@ -2132,7 +2132,7 @@ static bool fbcon_switch(struct vc_data *vc)
 			fbcon_del_cursor_work(old_info);
 	}
 
-	if (fbcon_is_inactive(vc, info) ||
+	if (!fbcon_is_active(vc, info) ||
 	    ops->blank_state != FB_BLANK_UNBLANK)
 		fbcon_del_cursor_work(info);
 	else
@@ -2172,7 +2172,7 @@ static bool fbcon_switch(struct vc_data *vc)
 	scrollback_max = 0;
 	scrollback_current = 0;
 
-	if (!fbcon_is_inactive(vc, info)) {
+	if (fbcon_is_active(vc, info)) {
 	    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
 	    ops->update_start(info);
 	}
@@ -2228,7 +2228,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
 		}
 	}
 
- 	if (!fbcon_is_inactive(vc, info)) {
+	if (fbcon_is_active(vc, info)) {
 		if (ops->blank_state != blank) {
 			ops->blank_state = blank;
 			fbcon_cursor(vc, !blank);
@@ -2242,7 +2242,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
 			update_screen(vc);
 	}
 
-	if (mode_switch || fbcon_is_inactive(vc, info) ||
+	if (mode_switch || !fbcon_is_active(vc, info) ||
 	    ops->blank_state != FB_BLANK_UNBLANK)
 		fbcon_del_cursor_work(info);
 	else
@@ -2572,7 +2572,7 @@ static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table)
 	int i, j, k, depth;
 	u8 val;
 
-	if (fbcon_is_inactive(vc, info))
+	if (!fbcon_is_active(vc, info))
 		return;
 
 	if (!con_is_visible(vc))
@@ -2672,7 +2672,7 @@ static void fbcon_modechanged(struct fb_info *info)
 		scrollback_max = 0;
 		scrollback_current = 0;
 
-		if (!fbcon_is_inactive(vc, info)) {
+		if (fbcon_is_active(vc, info)) {
 		    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
 		    ops->update_start(info);
 		}
-- 
2.44.2

[PATCH 5/6] fbcon: Introduce get_{fg,bg}_color()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make the code more legible by adding get_{fg,bg}_color()
which hide the obscure 'is_fg' parameter of get_color()
from the caller.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 2a78cca3e9de..17540cdf1edf 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -356,6 +356,16 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
 	return color;
 }
 
+static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
+{
+	return get_color(vc, info, c, 1);
+}
+
+static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
+{
+	return get_color(vc, info, c, 0);
+}
+
 static void fb_flashcursor(struct work_struct *work)
 {
 	struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work);
@@ -387,8 +397,9 @@ static void fb_flashcursor(struct work_struct *work)
 
 	c = scr_readw((u16 *) vc->vc_pos);
 	enable = ops->cursor_flash && !ops->cursor_state.enable;
-	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
-		    get_color(vc, info, c, 0));
+	ops->cursor(vc, info, enable,
+		    get_fg_color(vc, info, c),
+		    get_bg_color(vc, info, c));
 	console_unlock();
 
 	queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
@@ -1297,8 +1308,8 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
 
 	if (fbcon_is_active(vc, info))
 		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
-			   get_color(vc, info, scr_readw(s), 1),
-			   get_color(vc, info, scr_readw(s), 0));
+			   get_fg_color(vc, info, scr_readw(s)),
+			   get_bg_color(vc, info, scr_readw(s)));
 }
 
 static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
@@ -1331,8 +1342,9 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
 	if (!ops->cursor)
 		return;
 
-	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
-		    get_color(vc, info, c, 0));
+	ops->cursor(vc, info, enable,
+		    get_fg_color(vc, info, c),
+		    get_bg_color(vc, info, c));
 }
 
 static int scrollback_phys_max = 0;
-- 
2.44.2

[PATCH 6/6] fbcon: Use 'bool' where appopriate

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use 'bool' type where it makes more sense than 'int'.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 17540cdf1edf..03d48e665bba 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -129,9 +129,9 @@ static int logo_shown = FBCON_LOGO_CANSHOW;
 /* console mappings */
 static unsigned int first_fb_vc;
 static unsigned int last_fb_vc = MAX_NR_CONSOLES - 1;
-static int fbcon_is_default = 1;
+static bool fbcon_is_default = true;
 static int primary_device = -1;
-static int fbcon_has_console_bind;
+static bool fbcon_has_console_bind;
 
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
 static int map_override;
@@ -166,7 +166,7 @@ static const struct consw fb_con;
 
 #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
 
-static int fbcon_cursor_blink;
+static bool fbcon_cursor_blink;
 
 #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
 
@@ -281,7 +281,7 @@ static bool fbcon_skip_panic(struct fb_info *info)
 #endif
 }
 
-static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
+static inline bool fbcon_is_active(struct vc_data *vc, struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
@@ -290,7 +290,7 @@ static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
 }
 
 static int get_color(struct vc_data *vc, struct fb_info *info,
-	      u16 c, int is_fg)
+		     u16 c, bool is_fg)
 {
 	int depth = fb_get_color_depth(&info->var, &info->fix);
 	int color = 0;
@@ -358,12 +358,12 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
 
 static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
 {
-	return get_color(vc, info, c, 1);
+	return get_color(vc, info, c, true);
 }
 
 static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
 {
-	return get_color(vc, info, c, 0);
+	return get_color(vc, info, c, false);
 }
 
 static void fb_flashcursor(struct work_struct *work)
@@ -467,7 +467,7 @@ static int __init fb_console_setup(char *this_opt)
 				last_fb_vc = simple_strtoul(options, &options, 10) - 1;
 			if (last_fb_vc < first_fb_vc || last_fb_vc >= MAX_NR_CONSOLES)
 				last_fb_vc = MAX_NR_CONSOLES - 1;
-			fbcon_is_default = 0;
+			fbcon_is_default = false;
 			continue;
 		}
 
@@ -558,7 +558,7 @@ static int do_fbcon_takeover(int show_logo)
 			con2fb_map[i] = -1;
 		info_idx = -1;
 	} else {
-		fbcon_has_console_bind = 1;
+		fbcon_has_console_bind = true;
 	}
 
 	return err;
@@ -2802,7 +2802,7 @@ static void fbcon_unbind(void)
 				fbcon_is_default);
 
 	if (!ret)
-		fbcon_has_console_bind = 0;
+		fbcon_has_console_bind = false;
 }
 #else
 static inline void fbcon_unbind(void) {}
@@ -3234,8 +3234,9 @@ static ssize_t cursor_blink_store(struct device *device,
 				  const char *buf, size_t count)
 {
 	struct fb_info *info;
-	int blink, idx;
 	char **last = NULL;
+	bool blink;
+	int idx;
 
 	blink = simple_strtoul(buf, last, 0);
 
-- 
2.44.2

Re: [PATCH 5/6] fbcon: Introduce get_{fg,bg}_color()

[Helge Deller]

On 9/23/24 17:57, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the code more legible by adding get_{fg,bg}_color()
> which hide the obscure 'is_fg' parameter of get_color()
> from the caller.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Nice cleanup.
Acked-by: Helge Deller <deller@gmx.de>


> ---
>   drivers/video/fbdev/core/fbcon.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2a78cca3e9de..17540cdf1edf 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -356,6 +356,16 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
>   	return color;
>   }
>
> +static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
> +{
> +	return get_color(vc, info, c, 1);
> +}
> +
> +static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
> +{
> +	return get_color(vc, info, c, 0);
> +}
> +
>   static void fb_flashcursor(struct work_struct *work)
>   {
>   	struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work);
> @@ -387,8 +397,9 @@ static void fb_flashcursor(struct work_struct *work)
>
>   	c = scr_readw((u16 *) vc->vc_pos);
>   	enable = ops->cursor_flash && !ops->cursor_state.enable;
> -	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
> -		    get_color(vc, info, c, 0));
> +	ops->cursor(vc, info, enable,
> +		    get_fg_color(vc, info, c),
> +		    get_bg_color(vc, info, c));
>   	console_unlock();
>
>   	queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
> @@ -1297,8 +1308,8 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>
>   	if (fbcon_is_active(vc, info))
>   		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
> -			   get_color(vc, info, scr_readw(s), 1),
> -			   get_color(vc, info, scr_readw(s), 0));
> +			   get_fg_color(vc, info, scr_readw(s)),
> +			   get_bg_color(vc, info, scr_readw(s)));
>   }
>
>   static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
> @@ -1331,8 +1342,9 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
>   	if (!ops->cursor)
>   		return;
>
> -	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
> -		    get_color(vc, info, c, 0));
> +	ops->cursor(vc, info, enable,
> +		    get_fg_color(vc, info, c),
> +		    get_bg_color(vc, info, c));
>   }
>
>   static int scrollback_phys_max = 0;

Re: [PATCH 4/6] fbcon: fbcon_is_inactive() -> fbcon_is_active()

[Helge Deller]

On 9/23/24 17:57, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Invert fbcon_is_inactive() into fbcon_is_active(). Much easier
> on the poor brain when you don't have to do dobule negations
> all over the place.

Agreed.
Acked-by: Helge Deller <deller@gmx.de>


> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/video/fbdev/core/fbcon.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index eb30aa872371..2a78cca3e9de 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -281,12 +281,12 @@ static bool fbcon_skip_panic(struct fb_info *info)
>   #endif
>   }
>
> -static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
> +static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>
> -	return (info->state != FBINFO_STATE_RUNNING ||
> -		vc->vc_mode != KD_TEXT || ops->graphics || fbcon_skip_panic(info));
> +	return info->state == FBINFO_STATE_RUNNING &&
> +		vc->vc_mode == KD_TEXT && !ops->graphics && !fbcon_skip_panic(info);
>   }
>
>   static int get_color(struct vc_data *vc, struct fb_info *info,
> @@ -1253,7 +1253,7 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	u_int y_break;
>
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>
>   	if (!height || !width)
> @@ -1295,7 +1295,7 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fbcon_ops *ops = info->fbcon_par;
>
> -	if (!fbcon_is_inactive(vc, info))
> +	if (fbcon_is_active(vc, info))
>   		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
>   			   get_color(vc, info, scr_readw(s), 1),
>   			   get_color(vc, info, scr_readw(s), 0));
> @@ -1306,7 +1306,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
>   	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>
> -	if (!fbcon_is_inactive(vc, info))
> +	if (fbcon_is_active(vc, info))
>   		ops->clear_margins(vc, info, margin_color, bottom_only);
>   }
>
> @@ -1318,7 +1318,7 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
>
>   	ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
>
> -	if (fbcon_is_inactive(vc, info) || vc->vc_deccm != 1)
> +	if (!fbcon_is_active(vc, info) || vc->vc_deccm != 1)
>   		return;
>
>   	if (vc->vc_cursor_type & CUR_SW)
> @@ -1724,7 +1724,7 @@ static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
>   	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>
>   	if (!width || !height)
> @@ -1748,7 +1748,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
>
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return true;
>
>   	fbcon_cursor(vc, false);
> @@ -2132,7 +2132,7 @@ static bool fbcon_switch(struct vc_data *vc)
>   			fbcon_del_cursor_work(old_info);
>   	}
>
> -	if (fbcon_is_inactive(vc, info) ||
> +	if (!fbcon_is_active(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
>   		fbcon_del_cursor_work(info);
>   	else
> @@ -2172,7 +2172,7 @@ static bool fbcon_switch(struct vc_data *vc)
>   	scrollback_max = 0;
>   	scrollback_current = 0;
>
> -	if (!fbcon_is_inactive(vc, info)) {
> +	if (fbcon_is_active(vc, info)) {
>   	    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
>   	    ops->update_start(info);
>   	}
> @@ -2228,7 +2228,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
>   		}
>   	}
>
> - 	if (!fbcon_is_inactive(vc, info)) {
> +	if (fbcon_is_active(vc, info)) {
>   		if (ops->blank_state != blank) {
>   			ops->blank_state = blank;
>   			fbcon_cursor(vc, !blank);
> @@ -2242,7 +2242,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
>   			update_screen(vc);
>   	}
>
> -	if (mode_switch || fbcon_is_inactive(vc, info) ||
> +	if (mode_switch || !fbcon_is_active(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
>   		fbcon_del_cursor_work(info);
>   	else
> @@ -2572,7 +2572,7 @@ static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table)
>   	int i, j, k, depth;
>   	u8 val;
>
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>
>   	if (!con_is_visible(vc))
> @@ -2672,7 +2672,7 @@ static void fbcon_modechanged(struct fb_info *info)
>   		scrollback_max = 0;
>   		scrollback_current = 0;
>
> -		if (!fbcon_is_inactive(vc, info)) {
> +		if (fbcon_is_active(vc, info)) {
>   		    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
>   		    ops->update_start(info);
>   		}

Re: [PATCH 3/6] fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink

[Helge Deller]

On 9/23/24 17:57, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Invert fbcon_cursor_noblink into fbcon_cursor_blink so that:
> - it matches the sysfs attribute exactly
> - avoids having to do these NOT operations all over the place
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/video/fbdev/core/fbcon.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index bbe332572ca7..eb30aa872371 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -166,7 +166,7 @@ static const struct consw fb_con;
>
>   #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
>
> -static int fbcon_cursor_noblink;
> +static int fbcon_cursor_blink;

Shouldn't it default then to value 1, e.g.
+static int fbcon_cursor_blink = 1;

Looks good otherwise.

Helge

>
>   #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
>
> @@ -399,7 +399,7 @@ static void fbcon_add_cursor_work(struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>
> -	if (!fbcon_cursor_noblink)
> +	if (fbcon_cursor_blink)
>   		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
>   				   ops->cur_blink_jiffies);
>   }
> @@ -3214,7 +3214,7 @@ static ssize_t rotate_show(struct device *device,
>   static ssize_t cursor_blink_show(struct device *device,
>   				 struct device_attribute *attr, char *buf)
>   {
> -	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
> +	return sysfs_emit(buf, "%d\n", fbcon_cursor_blink);
>   }
>
>   static ssize_t cursor_blink_store(struct device *device,
> @@ -3230,7 +3230,7 @@ static ssize_t cursor_blink_store(struct device *device,
>   	console_lock();
>   	idx = con2fb_map[fg_console];
>
> -	fbcon_cursor_noblink = !blink;
> +	fbcon_cursor_blink = blink;
>
>   	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
>   		goto err;

Re: [PATCH 6/6] fbcon: Use 'bool' where appopriate

[Helge Deller]

On 9/23/24 17:57, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use 'bool' type where it makes more sense than 'int'.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks good.

Acked-by: Helge Deller <deller@gmx.de>

Helge

> ---
>   drivers/video/fbdev/core/fbcon.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 17540cdf1edf..03d48e665bba 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -129,9 +129,9 @@ static int logo_shown = FBCON_LOGO_CANSHOW;
>   /* console mappings */
>   static unsigned int first_fb_vc;
>   static unsigned int last_fb_vc = MAX_NR_CONSOLES - 1;
> -static int fbcon_is_default = 1;
> +static bool fbcon_is_default = true;
>   static int primary_device = -1;
> -static int fbcon_has_console_bind;
> +static bool fbcon_has_console_bind;
>
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
>   static int map_override;
> @@ -166,7 +166,7 @@ static const struct consw fb_con;
>
>   #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
>
> -static int fbcon_cursor_blink;
> +static bool fbcon_cursor_blink;
>
>   #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
>
> @@ -281,7 +281,7 @@ static bool fbcon_skip_panic(struct fb_info *info)
>   #endif
>   }
>
> -static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
> +static inline bool fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>
> @@ -290,7 +290,7 @@ static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   }
>
>   static int get_color(struct vc_data *vc, struct fb_info *info,
> -	      u16 c, int is_fg)
> +		     u16 c, bool is_fg)
>   {
>   	int depth = fb_get_color_depth(&info->var, &info->fix);
>   	int color = 0;
> @@ -358,12 +358,12 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
>
>   static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
>   {
> -	return get_color(vc, info, c, 1);
> +	return get_color(vc, info, c, true);
>   }
>
>   static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
>   {
> -	return get_color(vc, info, c, 0);
> +	return get_color(vc, info, c, false);
>   }
>
>   static void fb_flashcursor(struct work_struct *work)
> @@ -467,7 +467,7 @@ static int __init fb_console_setup(char *this_opt)
>   				last_fb_vc = simple_strtoul(options, &options, 10) - 1;
>   			if (last_fb_vc < first_fb_vc || last_fb_vc >= MAX_NR_CONSOLES)
>   				last_fb_vc = MAX_NR_CONSOLES - 1;
> -			fbcon_is_default = 0;
> +			fbcon_is_default = false;
>   			continue;
>   		}
>
> @@ -558,7 +558,7 @@ static int do_fbcon_takeover(int show_logo)
>   			con2fb_map[i] = -1;
>   		info_idx = -1;
>   	} else {
> -		fbcon_has_console_bind = 1;
> +		fbcon_has_console_bind = true;
>   	}
>
>   	return err;
> @@ -2802,7 +2802,7 @@ static void fbcon_unbind(void)
>   				fbcon_is_default);
>
>   	if (!ret)
> -		fbcon_has_console_bind = 0;
> +		fbcon_has_console_bind = false;
>   }
>   #else
>   static inline void fbcon_unbind(void) {}
> @@ -3234,8 +3234,9 @@ static ssize_t cursor_blink_store(struct device *device,
>   				  const char *buf, size_t count)
>   {
>   	struct fb_info *info;
> -	int blink, idx;
>   	char **last = NULL;
> +	bool blink;
> +	int idx;
>
>   	blink = simple_strtoul(buf, last, 0);
>

Re: [PATCH 3/6] fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink

[Ville Syrjälä]

On Mon, Sep 23, 2024 at 09:46:03PM +0200, Helge Deller wrote:
> On 9/23/24 17:57, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Invert fbcon_cursor_noblink into fbcon_cursor_blink so that:
> > - it matches the sysfs attribute exactly
> > - avoids having to do these NOT operations all over the place
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/video/fbdev/core/fbcon.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index bbe332572ca7..eb30aa872371 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -166,7 +166,7 @@ static const struct consw fb_con;
> >
> >   #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
> >
> > -static int fbcon_cursor_noblink;
> > +static int fbcon_cursor_blink;
> 
> Shouldn't it default then to value 1, e.g.
> +static int fbcon_cursor_blink = 1;

Indeed, good catch. Thanks. 

Had to double check that my udev rule still actually works
and it wasn't just caused by this fumble. Fortunately it
still seems effective. Phew.

> 
> Looks good otherwise.
> 
> Helge
> 
> >
> >   #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
> >
> > @@ -399,7 +399,7 @@ static void fbcon_add_cursor_work(struct fb_info *info)
> >   {
> >   	struct fbcon_ops *ops = info->fbcon_par;
> >
> > -	if (!fbcon_cursor_noblink)
> > +	if (fbcon_cursor_blink)
> >   		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
> >   				   ops->cur_blink_jiffies);
> >   }
> > @@ -3214,7 +3214,7 @@ static ssize_t rotate_show(struct device *device,
> >   static ssize_t cursor_blink_show(struct device *device,
> >   				 struct device_attribute *attr, char *buf)
> >   {
> > -	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
> > +	return sysfs_emit(buf, "%d\n", fbcon_cursor_blink);
> >   }
> >
> >   static ssize_t cursor_blink_store(struct device *device,
> > @@ -3230,7 +3230,7 @@ static ssize_t cursor_blink_store(struct device *device,
> >   	console_lock();
> >   	idx = con2fb_map[fg_console];
> >
> > -	fbcon_cursor_noblink = !blink;
> > +	fbcon_cursor_blink = blink;
> >
> >   	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> >   		goto err;

-- 
Ville Syrjälä
Intel

[PATCH v2 3/6] fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Invert fbcon_cursor_noblink into fbcon_cursor_blink so that:
- it matches the sysfs attribute exactly
- avoids having to do these NOT operations all over the place

v2: Set the initial value to 1 to keep the same default
    behaviour (Helge)

Cc: Helge Deller <deller@gmx.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bbe332572ca7..0681ac7900a2 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -166,7 +166,7 @@ static const struct consw fb_con;
 
 #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
 
-static int fbcon_cursor_noblink;
+static int fbcon_cursor_blink = 1;
 
 #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
 
@@ -399,7 +399,7 @@ static void fbcon_add_cursor_work(struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!fbcon_cursor_noblink)
+	if (fbcon_cursor_blink)
 		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
 				   ops->cur_blink_jiffies);
 }
@@ -3214,7 +3214,7 @@ static ssize_t rotate_show(struct device *device,
 static ssize_t cursor_blink_show(struct device *device,
 				 struct device_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
+	return sysfs_emit(buf, "%d\n", fbcon_cursor_blink);
 }
 
 static ssize_t cursor_blink_store(struct device *device,
@@ -3230,7 +3230,7 @@ static ssize_t cursor_blink_store(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	fbcon_cursor_noblink = !blink;
+	fbcon_cursor_blink = blink;
 
 	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
-- 
2.44.2

[PATCH v2 6/6] fbcon: Use 'bool' where appopriate

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use 'bool' type where it makes more sense than 'int'.

v2: Rebase due to corrected 'fbcon_cursor_blink' initial value

Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/fbdev/core/fbcon.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index b905996c3588..b3c83a43fe48 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -129,9 +129,9 @@ static int logo_shown = FBCON_LOGO_CANSHOW;
 /* console mappings */
 static unsigned int first_fb_vc;
 static unsigned int last_fb_vc = MAX_NR_CONSOLES - 1;
-static int fbcon_is_default = 1;
+static bool fbcon_is_default = true;
 static int primary_device = -1;
-static int fbcon_has_console_bind;
+static bool fbcon_has_console_bind;
 
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
 static int map_override;
@@ -166,7 +166,7 @@ static const struct consw fb_con;
 
 #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
 
-static int fbcon_cursor_blink = 1;
+static bool fbcon_cursor_blink = true;
 
 #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
 
@@ -281,7 +281,7 @@ static bool fbcon_skip_panic(struct fb_info *info)
 #endif
 }
 
-static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
+static inline bool fbcon_is_active(struct vc_data *vc, struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
@@ -290,7 +290,7 @@ static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
 }
 
 static int get_color(struct vc_data *vc, struct fb_info *info,
-	      u16 c, int is_fg)
+		     u16 c, bool is_fg)
 {
 	int depth = fb_get_color_depth(&info->var, &info->fix);
 	int color = 0;
@@ -358,12 +358,12 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
 
 static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
 {
-	return get_color(vc, info, c, 1);
+	return get_color(vc, info, c, true);
 }
 
 static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
 {
-	return get_color(vc, info, c, 0);
+	return get_color(vc, info, c, false);
 }
 
 static void fb_flashcursor(struct work_struct *work)
@@ -467,7 +467,7 @@ static int __init fb_console_setup(char *this_opt)
 				last_fb_vc = simple_strtoul(options, &options, 10) - 1;
 			if (last_fb_vc < first_fb_vc || last_fb_vc >= MAX_NR_CONSOLES)
 				last_fb_vc = MAX_NR_CONSOLES - 1;
-			fbcon_is_default = 0;
+			fbcon_is_default = false;
 			continue;
 		}
 
@@ -558,7 +558,7 @@ static int do_fbcon_takeover(int show_logo)
 			con2fb_map[i] = -1;
 		info_idx = -1;
 	} else {
-		fbcon_has_console_bind = 1;
+		fbcon_has_console_bind = true;
 	}
 
 	return err;
@@ -2802,7 +2802,7 @@ static void fbcon_unbind(void)
 				fbcon_is_default);
 
 	if (!ret)
-		fbcon_has_console_bind = 0;
+		fbcon_has_console_bind = false;
 }
 #else
 static inline void fbcon_unbind(void) {}
@@ -3234,8 +3234,9 @@ static ssize_t cursor_blink_store(struct device *device,
 				  const char *buf, size_t count)
 {
 	struct fb_info *info;
-	int blink, idx;
 	char **last = NULL;
+	bool blink;
+	int idx;
 
 	blink = simple_strtoul(buf, last, 0);
 
-- 
2.44.2

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

On 9/23/24 17:57, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently setting cursor_blink attribute to 0 before any fb
> devices are around does absolutely nothing. When fb devices appear
> and fbcon becomes active the cursor starts blinking. Fix the problem
> by recoding the desired state of the attribute even if no fb devices
> are present at the time.
>
> Also adjust the show() method such that it shows the current
> state of the attribute even when no fb devices are in use.
>
> Note that store_cursor_blink() is still a bit dodgy:
> - seems to be missing some of the other checks that we do
>    elsewhere when deciding whether the cursor should be
>    blinking or not
> - when set to 0 when the cursor is currently invisible due
>    to blinking, the cursor will remains invisible. We should
>    either explicitly make it visible here, or wait until the
>    full blink cycle has finished.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/video/fbdev/core/fbcon.c | 34 +++++++-------------------------
>   1 file changed, 7 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2e093535884b..8936fa79b9e0 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -3217,26 +3217,7 @@ static ssize_t show_rotate(struct device *device,
>   static ssize_t show_cursor_blink(struct device *device,
>   				 struct device_attribute *attr, char *buf)
>   {
> -	struct fb_info *info;
> -	struct fbcon_ops *ops;
> -	int idx, blink = -1;
> -
> -	console_lock();
> -	idx = con2fb_map[fg_console];
> -
> -	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> -		goto err;
> -
> -	info = fbcon_registered_fb[idx];
> -	ops = info->fbcon_par;
> -
> -	if (!ops)
> -		goto err;
> -
> -	blink = delayed_work_pending(&ops->cursor_work);
> -err:
> -	console_unlock();
> -	return sysfs_emit(buf, "%d\n", blink);
> +	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);

I might be wrong and mix up things, but I think the previous code allowed
to show/set the blink value *per* registered framebuffer console,
while now you generally enable/disable blinking for all
framebuffer drivers at once?
Just thinking of a multiseat setup with different fb drivers
attached to different monitors with own mouse/keyboards...?!?


>   }
>
>   static ssize_t store_cursor_blink(struct device *device,
> @@ -3247,9 +3228,13 @@ static ssize_t store_cursor_blink(struct device *device,
>   	int blink, idx;
>   	char **last = NULL;
>
> +	blink = simple_strtoul(buf, last, 0);
> +
>   	console_lock();
>   	idx = con2fb_map[fg_console];
>
> +	fbcon_cursor_noblink = !blink;
> +
>   	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
>   		goto err;
>
> @@ -3258,15 +3243,10 @@ static ssize_t store_cursor_blink(struct device *device,
>   	if (!info->fbcon_par)
>   		goto err;
>
> -	blink = simple_strtoul(buf, last, 0);
> -
> -	if (blink) {
> -		fbcon_cursor_noblink = 0;
> +	if (blink)
>   		fbcon_add_cursor_work(info);
> -	} else {
> -		fbcon_cursor_noblink = 1;
> +	else
>   		fbcon_del_cursor_work(info);
> -	}
>
>   err:
>   	console_unlock();

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjälä]

On Mon, Sep 23, 2024 at 11:04:55PM +0200, Helge Deller wrote:
> On 9/23/24 17:57, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently setting cursor_blink attribute to 0 before any fb
> > devices are around does absolutely nothing. When fb devices appear
> > and fbcon becomes active the cursor starts blinking. Fix the problem
> > by recoding the desired state of the attribute even if no fb devices
> > are present at the time.
> >
> > Also adjust the show() method such that it shows the current
> > state of the attribute even when no fb devices are in use.
> >
> > Note that store_cursor_blink() is still a bit dodgy:
> > - seems to be missing some of the other checks that we do
> >    elsewhere when deciding whether the cursor should be
> >    blinking or not
> > - when set to 0 when the cursor is currently invisible due
> >    to blinking, the cursor will remains invisible. We should
> >    either explicitly make it visible here, or wait until the
> >    full blink cycle has finished.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/video/fbdev/core/fbcon.c | 34 +++++++-------------------------
> >   1 file changed, 7 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index 2e093535884b..8936fa79b9e0 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -3217,26 +3217,7 @@ static ssize_t show_rotate(struct device *device,
> >   static ssize_t show_cursor_blink(struct device *device,
> >   				 struct device_attribute *attr, char *buf)
> >   {
> > -	struct fb_info *info;
> > -	struct fbcon_ops *ops;
> > -	int idx, blink = -1;
> > -
> > -	console_lock();
> > -	idx = con2fb_map[fg_console];
> > -
> > -	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> > -		goto err;
> > -
> > -	info = fbcon_registered_fb[idx];
> > -	ops = info->fbcon_par;
> > -
> > -	if (!ops)
> > -		goto err;
> > -
> > -	blink = delayed_work_pending(&ops->cursor_work);
> > -err:
> > -	console_unlock();
> > -	return sysfs_emit(buf, "%d\n", blink);
> > +	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
> 
> I might be wrong and mix up things, but I think the previous code allowed
> to show/set the blink value *per* registered framebuffer console,
> while now you generally enable/disable blinking for all
> framebuffer drivers at once?
> Just thinking of a multiseat setup with different fb drivers
> attached to different monitors with own mouse/keyboards...?!?

There is just a single fbcon device in sysfs, so not really.
It's true that it only ever operated on the fg_console, so
on a first glane it may look like it might be some kind of
per-fb thing. But the state was only recorded in the
fbcon_cursor_noblink singleton, so when another vt becomes
active it'll look at that an not start the blinker.
So I think the per-fb aspect was just an illusion.

The whole interface is a bit of a mess. But I don't
really see why anyone would want to use this on a
per-fb device basis anyway. Either you want blinking
stuff and extra power draw, or you don't.

I think there are ways to turn of blinking via some escape
sequences or something as well, so those could probably
be used on a per-console basis. But I like to use this
sysfs instead because it can't accidentally be re-enabled
when random programs mess around with ttys.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

On 9/23/24 23:30, Ville Syrjälä wrote:
> On Mon, Sep 23, 2024 at 11:04:55PM +0200, Helge Deller wrote:
>> On 9/23/24 17:57, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Currently setting cursor_blink attribute to 0 before any fb
>>> devices are around does absolutely nothing. When fb devices appear
>>> and fbcon becomes active the cursor starts blinking. Fix the problem
>>> by recoding the desired state of the attribute even if no fb devices
>>> are present at the time.
>>>
>>> Also adjust the show() method such that it shows the current
>>> state of the attribute even when no fb devices are in use.
>>>
>>> Note that store_cursor_blink() is still a bit dodgy:
>>> - seems to be missing some of the other checks that we do
>>>     elsewhere when deciding whether the cursor should be
>>>     blinking or not
>>> - when set to 0 when the cursor is currently invisible due
>>>     to blinking, the cursor will remains invisible. We should
>>>     either explicitly make it visible here, or wait until the
>>>     full blink cycle has finished.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/video/fbdev/core/fbcon.c | 34 +++++++-------------------------
>>>    1 file changed, 7 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>>> index 2e093535884b..8936fa79b9e0 100644
>>> --- a/drivers/video/fbdev/core/fbcon.c
>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>> @@ -3217,26 +3217,7 @@ static ssize_t show_rotate(struct device *device,
>>>    static ssize_t show_cursor_blink(struct device *device,
>>>    				 struct device_attribute *attr, char *buf)
>>>    {
>>> -	struct fb_info *info;
>>> -	struct fbcon_ops *ops;
>>> -	int idx, blink = -1;
>>> -
>>> -	console_lock();
>>> -	idx = con2fb_map[fg_console];
>>> -
>>> -	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
>>> -		goto err;
>>> -
>>> -	info = fbcon_registered_fb[idx];
>>> -	ops = info->fbcon_par;
>>> -
>>> -	if (!ops)
>>> -		goto err;
>>> -
>>> -	blink = delayed_work_pending(&ops->cursor_work);
>>> -err:
>>> -	console_unlock();
>>> -	return sysfs_emit(buf, "%d\n", blink);
>>> +	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
>>
>> I might be wrong and mix up things, but I think the previous code allowed
>> to show/set the blink value *per* registered framebuffer console,
>> while now you generally enable/disable blinking for all
>> framebuffer drivers at once?
>> Just thinking of a multiseat setup with different fb drivers
>> attached to different monitors with own mouse/keyboards...?!?
>
> There is just a single fbcon device in sysfs, so not really.
> It's true that it only ever operated on the fg_console, so
> on a first glane it may look like it might be some kind of
> per-fb thing. But the state was only recorded in the
> fbcon_cursor_noblink singleton, so when another vt becomes
> active it'll look at that an not start the blinker.

True.

> So I think the per-fb aspect was just an illusion.

No, I think in the past it worked, but I assume the per-fb
think got lost in all recent fbcon changes....
So, it's not a problem of your patch series.

> The whole interface is a bit of a mess. But I don't
> really see why anyone would want to use this on a
> per-fb device basis anyway. Either you want blinking
> stuff and extra power draw, or you don't.
>
> I think there are ways to turn of blinking via some escape
> sequences or something as well, so those could probably
> be used on a per-console basis. But I like to use this
> sysfs instead because it can't accidentally be re-enabled
> when random programs mess around with ttys.

I've added your patch series to the fbdev for-next git tree
to get some feedback from the autobuilders and testsuites.
I had to manually adjust patch #4 and #6 (after applying your v2
patches), so maybe you send a v3 of your whole series at some point.
Let's see what we get ...

Helge

Re: [PATCH 2/6] fbcon: Add sysfs attributes before registering the device

[Thomas Zimmermann]

Am 23.09.24 um 17:57 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently fbcon adds the attributes after registering the device,
> which means the attributes may not be there by the time udev
> gets the ADD uevent. Fix the race by switching over to
> device_create_with_groups().
>
> With this one can reliably turn off the power wasting cursor
> blink with a udev rule such as:
> ACTION=="add", SUBSYSTEM=="graphics", TEST=="cursor_blink", ATTR{cursor_blink}="0"
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 73 +++++++++-----------------------
>   1 file changed, 19 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 8936fa79b9e0..bbe332572ca7 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -160,7 +160,6 @@ static int info_idx = -1;
>   
>   /* console rotation */
>   static int initial_rotation = -1;
> -static int fbcon_has_sysfs;
>   static int margin_color;
>   
>   static const struct consw fb_con;
> @@ -188,8 +187,6 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
>   static void fbcon_modechanged(struct fb_info *info);
>   static void fbcon_set_all_vcs(struct fb_info *info);
>   
> -static struct device *fbcon_device;
> -
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION
>   static inline void fbcon_set_rotation(struct fb_info *info)
>   {
> @@ -3151,7 +3148,7 @@ static const struct consw fb_con = {
>   	.con_debug_leave	= fbcon_debug_leave,
>   };
>   
> -static ssize_t store_rotate(struct device *device,
> +static ssize_t rotate_store(struct device *device,
>   			    struct device_attribute *attr, const char *buf,
>   			    size_t count)
>   {
> @@ -3173,7 +3170,7 @@ static ssize_t store_rotate(struct device *device,
>   	return count;
>   }
>   
> -static ssize_t store_rotate_all(struct device *device,
> +static ssize_t rotate_all_store(struct device *device,
>   				struct device_attribute *attr,const char *buf,
>   				size_t count)
>   {
> @@ -3195,7 +3192,7 @@ static ssize_t store_rotate_all(struct device *device,
>   	return count;
>   }
>   
> -static ssize_t show_rotate(struct device *device,
> +static ssize_t rotate_show(struct device *device,
>   			   struct device_attribute *attr,char *buf)
>   {
>   	struct fb_info *info;
> @@ -3214,13 +3211,13 @@ static ssize_t show_rotate(struct device *device,
>   	return sysfs_emit(buf, "%d\n", rotate);
>   }
>   
> -static ssize_t show_cursor_blink(struct device *device,
> +static ssize_t cursor_blink_show(struct device *device,
>   				 struct device_attribute *attr, char *buf)
>   {
>   	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
>   }
>   
> -static ssize_t store_cursor_blink(struct device *device,
> +static ssize_t cursor_blink_store(struct device *device,
>   				  struct device_attribute *attr,
>   				  const char *buf, size_t count)
>   {
> @@ -3253,35 +3250,17 @@ static ssize_t store_cursor_blink(struct device *device,
>   	return count;
>   }
>   
> -static struct device_attribute device_attrs[] = {
> -	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
> -	__ATTR(rotate_all, S_IWUSR, NULL, store_rotate_all),
> -	__ATTR(cursor_blink, S_IRUGO|S_IWUSR, show_cursor_blink,
> -	       store_cursor_blink),
> -};
> -
> -static int fbcon_init_device(void)
> -{
> -	int i, error = 0;
> -
> -	fbcon_has_sysfs = 1;
> -
> -	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
> -		error = device_create_file(fbcon_device, &device_attrs[i]);
> -
> -		if (error)
> -			break;
> -	}
> -
> -	if (error) {
> -		while (--i >= 0)
> -			device_remove_file(fbcon_device, &device_attrs[i]);
> -
> -		fbcon_has_sysfs = 0;
> -	}
> +static DEVICE_ATTR_RW(rotate);
> +static DEVICE_ATTR_WO(rotate_all);
> +static DEVICE_ATTR_RW(cursor_blink);
>   
> -	return 0;
> -}
> +static struct attribute *device_attrs[] = {
> +	&dev_attr_rotate.attr,
> +	&dev_attr_rotate_all.attr,
> +	&dev_attr_cursor_blink.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(device);
>   
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>   static void fbcon_register_existing_fbs(struct work_struct *work)
> @@ -3336,19 +3315,18 @@ static void fbcon_start(void)
>   
>   void __init fb_console_init(void)
>   {
> +	struct device *fbcon_device;
>   	int i;
>   
>   	console_lock();
> -	fbcon_device = device_create(fb_class, NULL, MKDEV(0, 0), NULL,
> -				     "fbcon");
>   
> +	fbcon_device = device_create_with_groups(fb_class, NULL, MKDEV(0, 0),
> +						 NULL, device_groups, "fbcon");
>   	if (IS_ERR(fbcon_device)) {
>   		printk(KERN_WARNING "Unable to create device "
>   		       "for fbcon; errno = %ld\n",
>   		       PTR_ERR(fbcon_device));
> -		fbcon_device = NULL;
> -	} else
> -		fbcon_init_device();
> +	}
>   
>   	for (i = 0; i < MAX_NR_CONSOLES; i++)
>   		con2fb_map[i] = -1;
> @@ -3359,18 +3337,6 @@ void __init fb_console_init(void)
>   
>   #ifdef MODULE
>   
> -static void __exit fbcon_deinit_device(void)
> -{
> -	int i;
> -
> -	if (fbcon_has_sysfs) {
> -		for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
> -			device_remove_file(fbcon_device, &device_attrs[i]);
> -
> -		fbcon_has_sysfs = 0;
> -	}
> -}
> -
>   void __exit fb_console_exit(void)
>   {
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> @@ -3383,7 +3349,6 @@ void __exit fb_console_exit(void)
>   #endif
>   
>   	console_lock();
> -	fbcon_deinit_device();
>   	device_destroy(fb_class, MKDEV(0, 0));
>   
>   	do_unregister_con_driver(&fb_con);

-- 
--
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)

Re: [PATCH 3/6] fbcon: fbcon_cursor_noblink -> fbcon_cursor_blink

[Thomas Zimmermann]

Am 23.09.24 um 17:57 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Invert fbcon_cursor_noblink into fbcon_cursor_blink so that:
> - it matches the sysfs attribute exactly
> - avoids having to do these NOT operations all over the place
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

With the inversion of the default fixed:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index bbe332572ca7..eb30aa872371 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -166,7 +166,7 @@ static const struct consw fb_con;
>   
>   #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
>   
> -static int fbcon_cursor_noblink;
> +static int fbcon_cursor_blink;
>   
>   #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
>   
> @@ -399,7 +399,7 @@ static void fbcon_add_cursor_work(struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	if (!fbcon_cursor_noblink)
> +	if (fbcon_cursor_blink)
>   		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
>   				   ops->cur_blink_jiffies);
>   }
> @@ -3214,7 +3214,7 @@ static ssize_t rotate_show(struct device *device,
>   static ssize_t cursor_blink_show(struct device *device,
>   				 struct device_attribute *attr, char *buf)
>   {
> -	return sysfs_emit(buf, "%d\n", !fbcon_cursor_noblink);
> +	return sysfs_emit(buf, "%d\n", fbcon_cursor_blink);
>   }
>   
>   static ssize_t cursor_blink_store(struct device *device,
> @@ -3230,7 +3230,7 @@ static ssize_t cursor_blink_store(struct device *device,
>   	console_lock();
>   	idx = con2fb_map[fg_console];
>   
> -	fbcon_cursor_noblink = !blink;
> +	fbcon_cursor_blink = blink;
>   
>   	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
>   		goto err;

-- 
--
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)

Re: [PATCH 4/6] fbcon: fbcon_is_inactive() -> fbcon_is_active()

[Thomas Zimmermann]

Am 23.09.24 um 17:57 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Invert fbcon_is_inactive() into fbcon_is_active(). Much easier
> on the poor brain when you don't have to do dobule negations
> all over the place.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index eb30aa872371..2a78cca3e9de 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -281,12 +281,12 @@ static bool fbcon_skip_panic(struct fb_info *info)
>   #endif
>   }
>   
> -static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
> +static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	return (info->state != FBINFO_STATE_RUNNING ||
> -		vc->vc_mode != KD_TEXT || ops->graphics || fbcon_skip_panic(info));
> +	return info->state == FBINFO_STATE_RUNNING &&
> +		vc->vc_mode == KD_TEXT && !ops->graphics && !fbcon_skip_panic(info);
>   }
>   
>   static int get_color(struct vc_data *vc, struct fb_info *info,
> @@ -1253,7 +1253,7 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	u_int y_break;
>   
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>   
>   	if (!height || !width)
> @@ -1295,7 +1295,7 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	if (!fbcon_is_inactive(vc, info))
> +	if (fbcon_is_active(vc, info))
>   		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
>   			   get_color(vc, info, scr_readw(s), 1),
>   			   get_color(vc, info, scr_readw(s), 0));
> @@ -1306,7 +1306,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
>   	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	if (!fbcon_is_inactive(vc, info))
> +	if (fbcon_is_active(vc, info))
>   		ops->clear_margins(vc, info, margin_color, bottom_only);
>   }
>   
> @@ -1318,7 +1318,7 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
>   
>   	ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
>   
> -	if (fbcon_is_inactive(vc, info) || vc->vc_deccm != 1)
> +	if (!fbcon_is_active(vc, info) || vc->vc_deccm != 1)
>   		return;
>   
>   	if (vc->vc_cursor_type & CUR_SW)
> @@ -1724,7 +1724,7 @@ static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
>   	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>   
>   	if (!width || !height)
> @@ -1748,7 +1748,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
>   
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return true;
>   
>   	fbcon_cursor(vc, false);
> @@ -2132,7 +2132,7 @@ static bool fbcon_switch(struct vc_data *vc)
>   			fbcon_del_cursor_work(old_info);
>   	}
>   
> -	if (fbcon_is_inactive(vc, info) ||
> +	if (!fbcon_is_active(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
>   		fbcon_del_cursor_work(info);
>   	else
> @@ -2172,7 +2172,7 @@ static bool fbcon_switch(struct vc_data *vc)
>   	scrollback_max = 0;
>   	scrollback_current = 0;
>   
> -	if (!fbcon_is_inactive(vc, info)) {
> +	if (fbcon_is_active(vc, info)) {
>   	    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
>   	    ops->update_start(info);
>   	}
> @@ -2228,7 +2228,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
>   		}
>   	}
>   
> - 	if (!fbcon_is_inactive(vc, info)) {
> +	if (fbcon_is_active(vc, info)) {
>   		if (ops->blank_state != blank) {
>   			ops->blank_state = blank;
>   			fbcon_cursor(vc, !blank);
> @@ -2242,7 +2242,7 @@ static bool fbcon_blank(struct vc_data *vc, enum vesa_blank_mode blank,
>   			update_screen(vc);
>   	}
>   
> -	if (mode_switch || fbcon_is_inactive(vc, info) ||
> +	if (mode_switch || !fbcon_is_active(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
>   		fbcon_del_cursor_work(info);
>   	else
> @@ -2572,7 +2572,7 @@ static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table)
>   	int i, j, k, depth;
>   	u8 val;
>   
> -	if (fbcon_is_inactive(vc, info))
> +	if (!fbcon_is_active(vc, info))
>   		return;
>   
>   	if (!con_is_visible(vc))
> @@ -2672,7 +2672,7 @@ static void fbcon_modechanged(struct fb_info *info)
>   		scrollback_max = 0;
>   		scrollback_current = 0;
>   
> -		if (!fbcon_is_inactive(vc, info)) {
> +		if (fbcon_is_active(vc, info)) {
>   		    ops->var.xoffset = ops->var.yoffset = p->yscroll = 0;
>   		    ops->update_start(info);
>   		}

-- 
--
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)

Re: [PATCH 5/6] fbcon: Introduce get_{fg,bg}_color()

[Thomas Zimmermann]

Am 23.09.24 um 17:57 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the code more legible by adding get_{fg,bg}_color()
> which hide the obscure 'is_fg' parameter of get_color()
> from the caller.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2a78cca3e9de..17540cdf1edf 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -356,6 +356,16 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
>   	return color;
>   }
>   
> +static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
> +{
> +	return get_color(vc, info, c, 1);
> +}
> +
> +static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
> +{
> +	return get_color(vc, info, c, 0);
> +}
> +
>   static void fb_flashcursor(struct work_struct *work)
>   {
>   	struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work);
> @@ -387,8 +397,9 @@ static void fb_flashcursor(struct work_struct *work)
>   
>   	c = scr_readw((u16 *) vc->vc_pos);
>   	enable = ops->cursor_flash && !ops->cursor_state.enable;
> -	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
> -		    get_color(vc, info, c, 0));
> +	ops->cursor(vc, info, enable,
> +		    get_fg_color(vc, info, c),
> +		    get_bg_color(vc, info, c));
>   	console_unlock();
>   
>   	queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
> @@ -1297,8 +1308,8 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
>   
>   	if (fbcon_is_active(vc, info))
>   		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
> -			   get_color(vc, info, scr_readw(s), 1),
> -			   get_color(vc, info, scr_readw(s), 0));
> +			   get_fg_color(vc, info, scr_readw(s)),
> +			   get_bg_color(vc, info, scr_readw(s)));
>   }
>   
>   static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
> @@ -1331,8 +1342,9 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
>   	if (!ops->cursor)
>   		return;
>   
> -	ops->cursor(vc, info, enable, get_color(vc, info, c, 1),
> -		    get_color(vc, info, c, 0));
> +	ops->cursor(vc, info, enable,
> +		    get_fg_color(vc, info, c),
> +		    get_bg_color(vc, info, c));
>   }
>   
>   static int scrollback_phys_max = 0;

-- 
--
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)

Re: [PATCH 6/6] fbcon: Use 'bool' where appopriate

[Thomas Zimmermann]

Am 23.09.24 um 17:57 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use 'bool' type where it makes more sense than 'int'.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 17540cdf1edf..03d48e665bba 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -129,9 +129,9 @@ static int logo_shown = FBCON_LOGO_CANSHOW;
>   /* console mappings */
>   static unsigned int first_fb_vc;
>   static unsigned int last_fb_vc = MAX_NR_CONSOLES - 1;
> -static int fbcon_is_default = 1;
> +static bool fbcon_is_default = true;
>   static int primary_device = -1;
> -static int fbcon_has_console_bind;
> +static bool fbcon_has_console_bind;
>   
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
>   static int map_override;
> @@ -166,7 +166,7 @@ static const struct consw fb_con;
>   
>   #define advance_row(p, delta) (unsigned short *)((unsigned long)(p) + (delta) * vc->vc_size_row)
>   
> -static int fbcon_cursor_blink;
> +static bool fbcon_cursor_blink;
>   
>   #define divides(a, b)	((!(a) || (b)%(a)) ? 0 : 1)
>   
> @@ -281,7 +281,7 @@ static bool fbcon_skip_panic(struct fb_info *info)
>   #endif
>   }
>   
> -static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
> +static inline bool fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> @@ -290,7 +290,7 @@ static inline int fbcon_is_active(struct vc_data *vc, struct fb_info *info)
>   }
>   
>   static int get_color(struct vc_data *vc, struct fb_info *info,
> -	      u16 c, int is_fg)
> +		     u16 c, bool is_fg)
>   {
>   	int depth = fb_get_color_depth(&info->var, &info->fix);
>   	int color = 0;
> @@ -358,12 +358,12 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
>   
>   static int get_fg_color(struct vc_data *vc, struct fb_info *info, u16 c)
>   {
> -	return get_color(vc, info, c, 1);
> +	return get_color(vc, info, c, true);
>   }
>   
>   static int get_bg_color(struct vc_data *vc, struct fb_info *info, u16 c)
>   {
> -	return get_color(vc, info, c, 0);
> +	return get_color(vc, info, c, false);
>   }
>   
>   static void fb_flashcursor(struct work_struct *work)
> @@ -467,7 +467,7 @@ static int __init fb_console_setup(char *this_opt)
>   				last_fb_vc = simple_strtoul(options, &options, 10) - 1;
>   			if (last_fb_vc < first_fb_vc || last_fb_vc >= MAX_NR_CONSOLES)
>   				last_fb_vc = MAX_NR_CONSOLES - 1;
> -			fbcon_is_default = 0;
> +			fbcon_is_default = false;
>   			continue;
>   		}
>   
> @@ -558,7 +558,7 @@ static int do_fbcon_takeover(int show_logo)
>   			con2fb_map[i] = -1;
>   		info_idx = -1;
>   	} else {
> -		fbcon_has_console_bind = 1;
> +		fbcon_has_console_bind = true;
>   	}
>   
>   	return err;
> @@ -2802,7 +2802,7 @@ static void fbcon_unbind(void)
>   				fbcon_is_default);
>   
>   	if (!ret)
> -		fbcon_has_console_bind = 0;
> +		fbcon_has_console_bind = false;
>   }
>   #else
>   static inline void fbcon_unbind(void) {}
> @@ -3234,8 +3234,9 @@ static ssize_t cursor_blink_store(struct device *device,
>   				  const char *buf, size_t count)
>   {
>   	struct fb_info *info;
> -	int blink, idx;
>   	char **last = NULL;
> +	bool blink;
> +	int idx;
>   
>   	blink = simple_strtoul(buf, last, 0);
>   

-- 
--
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)

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

Ville,

On 9/23/24 23:50, Helge Deller wrote:
> I've added your patch series to the fbdev for-next git tree
> to get some feedback from the autobuilders and testsuites.
> I had to manually adjust patch #4 and #6 (after applying your v2
> patches), so maybe you send a v3 of your whole series at some point.

Your (fixed) patch series was OK. I had to update to latest git head
from Linus to get it applied.

I applied the series again, including Thomas Zimmermanns R-b tag, so
no action needed from your side for now.

Thanks!
Helge

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjälä]

On Tue, Sep 24, 2024 at 10:27:02AM +0200, Helge Deller wrote:
> Ville,
> 
> On 9/23/24 23:50, Helge Deller wrote:
> > I've added your patch series to the fbdev for-next git tree
> > to get some feedback from the autobuilders and testsuites.
> > I had to manually adjust patch #4 and #6 (after applying your v2
> > patches), so maybe you send a v3 of your whole series at some point.
> 
> Your (fixed) patch series was OK. I had to update to latest git head
> from Linus to get it applied.
> 
> I applied the series again, including Thomas Zimmermanns R-b tag, so
> no action needed from your side for now.

Cool. Thanks.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

Hi Ville,

On 9/23/24 17:57, Ville Syrjala wrote:
> Currently setting cursor_blink attribute to 0 before any fb
> devices are around does absolutely nothing. When fb devices appear
> and fbcon becomes active the cursor starts blinking. Fix the problem
> by recoding the desired state of the attribute even if no fb devices
> are present at the time.
>
> Also adjust the show() method such that it shows the current
> state of the attribute even when no fb devices are in use.
>
> Note that store_cursor_blink() is still a bit dodgy:
> - seems to be missing some of the other checks that we do
>    elsewhere when deciding whether the cursor should be
>    blinking or not
> - when set to 0 when the cursor is currently invisible due
>    to blinking, the cursor will remains invisible. We should
>    either explicitly make it visible here, or wait until the
>    full blink cycle has finished.

are you planning to send follow-up patches to address those shortcomings?

Helge

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
> Hi Ville,
> 
> On 9/23/24 17:57, Ville Syrjala wrote:
> > Currently setting cursor_blink attribute to 0 before any fb
> > devices are around does absolutely nothing. When fb devices appear
> > and fbcon becomes active the cursor starts blinking. Fix the problem
> > by recoding the desired state of the attribute even if no fb devices
> > are present at the time.
> >
> > Also adjust the show() method such that it shows the current
> > state of the attribute even when no fb devices are in use.
> >
> > Note that store_cursor_blink() is still a bit dodgy:
> > - seems to be missing some of the other checks that we do
> >    elsewhere when deciding whether the cursor should be
> >    blinking or not
> > - when set to 0 when the cursor is currently invisible due
> >    to blinking, the cursor will remains invisible. We should
> >    either explicitly make it visible here, or wait until the
> >    full blink cycle has finished.
> 
> are you planning to send follow-up patches to address those shortcomings?

Nope. I don't really care about those as I never plan to 
turn the cursor blinking back on after turning it off via
udev.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

On 9/26/24 11:57, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
>> Hi Ville,
>>
>> On 9/23/24 17:57, Ville Syrjala wrote:
>>> Currently setting cursor_blink attribute to 0 before any fb
>>> devices are around does absolutely nothing. When fb devices appear
>>> and fbcon becomes active the cursor starts blinking. Fix the problem
>>> by recoding the desired state of the attribute even if no fb devices
>>> are present at the time.
>>>
>>> Also adjust the show() method such that it shows the current
>>> state of the attribute even when no fb devices are in use.
>>>
>>> Note that store_cursor_blink() is still a bit dodgy:
>>> - seems to be missing some of the other checks that we do
>>>     elsewhere when deciding whether the cursor should be
>>>     blinking or not
>>> - when set to 0 when the cursor is currently invisible due
>>>     to blinking, the cursor will remains invisible. We should
>>>     either explicitly make it visible here, or wait until the
>>>     full blink cycle has finished.
>>
>> are you planning to send follow-up patches to address those shortcomings?
>
> Nope. I don't really care about those as I never plan to
> turn the cursor blinking back on after turning it off via
> udev.

Sad, but OK. I will look into this when I find time.
I'd hoped to push those patches upstream during this merge window,
but then I think I have to delay them at least until kernel 6.13.

Thanks!
Helge

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
> On 9/26/24 11:57, Ville Syrjälä wrote:
> > On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
> >> Hi Ville,
> >>
> >> On 9/23/24 17:57, Ville Syrjala wrote:
> >>> Currently setting cursor_blink attribute to 0 before any fb
> >>> devices are around does absolutely nothing. When fb devices appear
> >>> and fbcon becomes active the cursor starts blinking. Fix the problem
> >>> by recoding the desired state of the attribute even if no fb devices
> >>> are present at the time.
> >>>
> >>> Also adjust the show() method such that it shows the current
> >>> state of the attribute even when no fb devices are in use.
> >>>
> >>> Note that store_cursor_blink() is still a bit dodgy:
> >>> - seems to be missing some of the other checks that we do
> >>>     elsewhere when deciding whether the cursor should be
> >>>     blinking or not
> >>> - when set to 0 when the cursor is currently invisible due
> >>>     to blinking, the cursor will remains invisible. We should
> >>>     either explicitly make it visible here, or wait until the
> >>>     full blink cycle has finished.
> >>
> >> are you planning to send follow-up patches to address those shortcomings?
> >
> > Nope. I don't really care about those as I never plan to
> > turn the cursor blinking back on after turning it off via
> > udev.
> 
> Sad, but OK. I will look into this when I find time.
> I'd hoped to push those patches upstream during this merge window,
> but then I think I have to delay them at least until kernel 6.13.

What happened to these? Not seeing them anywhere...

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

On 2/13/25 15:42, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
>> On 9/26/24 11:57, Ville Syrjälä wrote:
>>> On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
>>>> Hi Ville,
>>>>
>>>> On 9/23/24 17:57, Ville Syrjala wrote:
>>>>> Currently setting cursor_blink attribute to 0 before any fb
>>>>> devices are around does absolutely nothing. When fb devices appear
>>>>> and fbcon becomes active the cursor starts blinking. Fix the problem
>>>>> by recoding the desired state of the attribute even if no fb devices
>>>>> are present at the time.
>>>>>
>>>>> Also adjust the show() method such that it shows the current
>>>>> state of the attribute even when no fb devices are in use.
>>>>>
>>>>> Note that store_cursor_blink() is still a bit dodgy:
>>>>> - seems to be missing some of the other checks that we do
>>>>>      elsewhere when deciding whether the cursor should be
>>>>>      blinking or not
>>>>> - when set to 0 when the cursor is currently invisible due
>>>>>      to blinking, the cursor will remains invisible. We should
>>>>>      either explicitly make it visible here, or wait until the
>>>>>      full blink cycle has finished.
>>>>
>>>> are you planning to send follow-up patches to address those shortcomings?
>>>
>>> Nope. I don't really care about those as I never plan to
>>> turn the cursor blinking back on after turning it off via
>>> udev.
>>
>> Sad, but OK. I will look into this when I find time.
>> I'd hoped to push those patches upstream during this merge window,
>> but then I think I have to delay them at least until kernel 6.13.
>
> What happened to these? Not seeing them anywhere...

The issues above are not fixed yet, so they are still parked in my for-next-next tree:
https://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git/log/?h=for-next-next

Helge

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Ville Syrjälä]

On Thu, Feb 13, 2025 at 11:47:42PM +0100, Helge Deller wrote:
> On 2/13/25 15:42, Ville Syrjälä wrote:
> > On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
> >> On 9/26/24 11:57, Ville Syrjälä wrote:
> >>> On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
> >>>> Hi Ville,
> >>>>
> >>>> On 9/23/24 17:57, Ville Syrjala wrote:
> >>>>> Currently setting cursor_blink attribute to 0 before any fb
> >>>>> devices are around does absolutely nothing. When fb devices appear
> >>>>> and fbcon becomes active the cursor starts blinking. Fix the problem
> >>>>> by recoding the desired state of the attribute even if no fb devices
> >>>>> are present at the time.
> >>>>>
> >>>>> Also adjust the show() method such that it shows the current
> >>>>> state of the attribute even when no fb devices are in use.
> >>>>>
> >>>>> Note that store_cursor_blink() is still a bit dodgy:
> >>>>> - seems to be missing some of the other checks that we do
> >>>>>      elsewhere when deciding whether the cursor should be
> >>>>>      blinking or not
> >>>>> - when set to 0 when the cursor is currently invisible due
> >>>>>      to blinking, the cursor will remains invisible. We should
> >>>>>      either explicitly make it visible here, or wait until the
> >>>>>      full blink cycle has finished.
> >>>>
> >>>> are you planning to send follow-up patches to address those shortcomings?
> >>>
> >>> Nope. I don't really care about those as I never plan to
> >>> turn the cursor blinking back on after turning it off via
> >>> udev.
> >>
> >> Sad, but OK. I will look into this when I find time.
> >> I'd hoped to push those patches upstream during this merge window,
> >> but then I think I have to delay them at least until kernel 6.13.
> >
> > What happened to these? Not seeing them anywhere...
> 
> The issues above are not fixed yet, so they are still parked in my for-next-next tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git/log/?h=for-next-next

Those issues are already present in the current code, so
I'm sure what's the point of holding this up due to them.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear

[Helge Deller]

On 2/14/25 18:41, Ville Syrjälä wrote:
> On Thu, Feb 13, 2025 at 11:47:42PM +0100, Helge Deller wrote:
>> On 2/13/25 15:42, Ville Syrjälä wrote:
>>> On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
>>>> On 9/26/24 11:57, Ville Syrjälä wrote:
>>>>> On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
>>>>>> Hi Ville,
>>>>>>
>>>>>> On 9/23/24 17:57, Ville Syrjala wrote:
>>>>>>> Currently setting cursor_blink attribute to 0 before any fb
>>>>>>> devices are around does absolutely nothing. When fb devices appear
>>>>>>> and fbcon becomes active the cursor starts blinking. Fix the problem
>>>>>>> by recoding the desired state of the attribute even if no fb devices
>>>>>>> are present at the time.
>>>>>>>
>>>>>>> Also adjust the show() method such that it shows the current
>>>>>>> state of the attribute even when no fb devices are in use.
>>>>>>>
>>>>>>> Note that store_cursor_blink() is still a bit dodgy:
>>>>>>> - seems to be missing some of the other checks that we do
>>>>>>>       elsewhere when deciding whether the cursor should be
>>>>>>>       blinking or not
>>>>>>> - when set to 0 when the cursor is currently invisible due
>>>>>>>       to blinking, the cursor will remains invisible. We should
>>>>>>>       either explicitly make it visible here, or wait until the
>>>>>>>       full blink cycle has finished.
>>>>>>
>>>>>> are you planning to send follow-up patches to address those shortcomings?
>>>>>
>>>>> Nope. I don't really care about those as I never plan to
>>>>> turn the cursor blinking back on after turning it off via
>>>>> udev.
>>>>
>>>> Sad, but OK. I will look into this when I find time.
>>>> I'd hoped to push those patches upstream during this merge window,
>>>> but then I think I have to delay them at least until kernel 6.13.
>>>
>>> What happened to these? Not seeing them anywhere...
>>
>> The issues above are not fixed yet, so they are still parked in my for-next-next tree:
>> https://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git/log/?h=for-next-next
>
> Those issues are already present in the current code, so
> I'm sure what's the point of holding this up due to them.

Let's try to fix it while we touch it.

Helge