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

[Helge Deller <deller@gmx.de>]

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