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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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