From: Lukas Wunner <lukas@wunner.de>
To: Nicolas Pitre <nico@fluxnic.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.com>,
Mikulas Patocka <mpatocka@redhat.com>,
Matthew Whitehead <tedheadster@gmail.com>,
Daniel Vetter <daniel.vetter@ffwll.ch>,
Arvind Sankar <nivedita@alum.mit.edu>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vt: Fix non-blinking cursor regression
Date: Sun, 26 Jan 2020 13:48:34 +0100 [thread overview]
Message-ID: <20200126124834.ea7uxyprz2uaek5x@wunner.de> (raw)
In-Reply-To: <nycvar.YSQ.7.76.2001221032210.1655@knanqh.ubzr>
On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote:
> On Wed, 22 Jan 2020, Lukas Wunner wrote:
> > Since commit a6dbe4427559 ("vt: perform safe console erase in the right
> > order"), when userspace clears both the scrollback buffer and the screen
> > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking
> > if that virtual console is not in the foreground. I'm witnessing this
> > on every boot of Raspbian since updating to v4.19.37+ because agetty
> > writes the sequence to /dev/tty6 while the console is still switched to
> > /dev/tty1. Switching consoles once makes the cursor blink again.
> >
> > The commit added an invocation of ->con_switch() to flush_scrollback().
> > Normally this is only invoked from switch_screen() to switch consoles.
> > switch_screen() updates *vc->vc_display_fg to the new console and
> > fbcon_switch() updates ops->currcon. Because the commit only invokes
> > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an
> > incomplete console switch.
> >
> > When fb_flashcursor() subsequently blinks the cursor, it retrieves the
> > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't
> > updated, con_is_visible() incorrectly returns false and as a result,
> > fb_flashcursor() bails out without blinking the cursor.
> >
> > The invocation of ->con_switch() appears to have been erroneous. After
> > all, why should a console switch be performed when clearing the screen?
> > The commit message doesn't provide a rationale either. So delete it.
>
> The problem here is that only vgacon provides a con_flush_scrollback
> method. When not provided, the only way to flush the scrollback buffer
> is to invoke the switch method. If you remove it the scrollback buffer
> of the foreground console won't be flushed in the fb case and possibly
> others.
Okay. I guess it's somewhat counter-intuitive that ->con_switch()
is called only because it has the side effect of flushing scrollback.
In particular, this approach doesn't work for nonvisible consoles.
So the proper solution might be to amend the fb_con struct with a
->con_flush_scrollback() hook. Which portions of fbcon_switch()
would have to be duplicated in that hook? The softback code at the
top of the function would seem like an obvious candidate. What about
the invocation of fb_set_var() (which in turn calls fb_pan_display())?
Anything else?
FWIW, even without any call to ->con_switch, writing "\e[3J" does
flush scrollback. Works both on the foreground console as well as
nonvisible consoles. I've tested this with bcm2708_fb which isn't
upstream yet, it's in the Raspberry Pi Foundation's downstream tree:
https://github.com/raspberrypi/linux/blob/rpi-4.19.y/drivers/video/fbdev/bcm2708_fb.c
> @@ -936,10 +936,13 @@ static void flush_scrollback(struct vc_data *vc)
> WARN_CONSOLE_UNLOCKED();
>
> set_origin(vc);
> - if (vc->vc_sw->con_flush_scrollback)
> + if (vc->vc_sw->con_flush_scrollback) {
> vc->vc_sw->con_flush_scrollback(vc);
> - else
> + } else if (con_is_visible(vc)) {
> + hide_cursor(vc);
> vc->vc_sw->con_switch(vc);
> + set_cursor(vc);
> + }
A dumb question perhaps, but why is it necessary to hide the cursor?
Thanks,
Lukas
next prev parent reply other threads:[~2020-01-26 12:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-22 14:17 [PATCH] vt: Fix non-blinking cursor regression Lukas Wunner
2020-01-22 16:40 ` Nicolas Pitre
2020-01-26 12:48 ` Lukas Wunner [this message]
2020-01-26 17:32 ` Nicolas Pitre
2020-01-28 11:11 ` Lukas Wunner
2020-01-28 16:23 ` Nicolas Pitre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200126124834.ea7uxyprz2uaek5x@wunner.de \
--to=lukas@wunner.de \
--cc=daniel.vetter@ffwll.ch \
--cc=gregkh@linuxfoundation.org \
--cc=jslaby@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mpatocka@redhat.com \
--cc=nico@fluxnic.net \
--cc=nivedita@alum.mit.edu \
--cc=tedheadster@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).