| Message ID | 20240923155749.30846-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fbcon: Fix 'cursor_blink' sysfs attribute | expand |
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();
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.
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
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
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.
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
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.
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
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();