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

On 9/23/24 23:30, Ville Syrjälä wrote:

On Mon, Sep 23, 2024 at 11:04:55PM +0200, Helge Deller wrote:

quoted

On 9/23/24 17:57, Ville Syrjala wrote:

quoted

From: Ville Syrjälä <redacted>

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ä <redacted>
---
   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