Patchwork console: Implementing blinking of cursor

login
register
mail settings
Submitter Jan Kiszka
Date July 2, 2012, 8:20 a.m.
Message ID <4FF159C1.8080508@siemens.com>
Download mbox | patch
Permalink /patch/168504/
State New
Headers show

Comments

Jan Kiszka - July 2, 2012, 8:20 a.m.
Let the text console cursor blink at 5 HZ.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 console.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)
Alon Levy - July 3, 2012, 8:59 a.m.
On Mon, Jul 02, 2012 at 10:20:17AM +0200, Jan Kiszka wrote:

One comment below.

Reviewed-by: Alon Levy <alevy@redhat.com>

> Let the text console cursor blink at 5 HZ.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  console.c |   26 +++++++++++++++++++++++++-
>  1 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/console.c b/console.c
> index 6a463f5..29b0f1c 100644
> --- a/console.c
> +++ b/console.c
> @@ -28,6 +28,7 @@
>  //#define DEBUG_CONSOLE
>  #define DEFAULT_BACKSCROLL 512
>  #define MAX_CONSOLES 12
> +#define CONSOLE_CURSOR_PERIOD 200
>  
>  #define QEMU_RGBA(r, g, b, a) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))
>  #define QEMU_RGB(r, g, b) QEMU_RGBA(r, g, b, 0xff)
> @@ -139,6 +140,8 @@ struct TextConsole {
>      TextCell *cells;
>      int text_x[2], text_y[2], cursor_invalidate;
>      int echo;
> +    int cursor_blink_state;
> +    QEMUTimer *cursor_timer;
>  
>      int update_x0;
>      int update_y0;
> @@ -615,7 +618,7 @@ static void console_show_cursor(TextConsole *s, int show)
>              y += s->total_height;
>          if (y < s->height) {
>              c = &s->cells[y1 * s->width + x];
> -            if (show) {
> +            if (show && s->cursor_blink_state) {
>                  TextAttributes t_attrib = s->t_attrib_default;
>                  t_attrib.invers = !(t_attrib.invers); /* invert fg and bg */
>                  vga_putcharxy(s->ds, x, y, c->ch, &t_attrib);
> @@ -1083,6 +1086,10 @@ void console_select(unsigned int index)
>      s = consoles[index];
>      if (s) {
>          DisplayState *ds = s->ds;
> +
> +        if (active_console->cursor_timer) {
> +            qemu_del_timer(active_console->cursor_timer);
> +        }
>          active_console = s;
>          if (ds_get_bits_per_pixel(s->ds)) {
>              ds->surface = qemu_resize_displaysurface(ds, s->g_width, s->g_height);
> @@ -1090,6 +1097,10 @@ void console_select(unsigned int index)
>              s->ds->surface->width = s->width;
>              s->ds->surface->height = s->height;
>          }
> +        if (s->cursor_timer) {
> +            qemu_mod_timer(s->cursor_timer,
> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
> +        }
>          dpy_resize(s->ds);
>          vga_hw_invalidate();
>      }
> @@ -1454,6 +1465,16 @@ static void text_console_set_echo(CharDriverState *chr, bool echo)
>      s->echo = echo;
>  }
>  
> +static void text_console_update_cursor(void *opaque)
> +{
> +    TextConsole *s = opaque;
> +
> +    s->cursor_blink_state ^= 1;
> +    vga_hw_invalidate();
> +    qemu_mod_timer(s->cursor_timer,
> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
> +}
> +
>  static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>  {
>      TextConsole *s;
> @@ -1482,6 +1503,9 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>          s->g_height = ds_get_height(s->ds);
>      }
>  
> +    s->cursor_timer =
> +        qemu_new_timer_ms(rt_clock, text_console_update_cursor, s);
> +
missing initialization of cursor_blink_state

>      s->hw_invalidate = text_console_invalidate;
>      s->hw_text_update = text_console_update;
>      s->hw = s;
> -- 
> 1.7.3.4
>
Jan Kiszka - July 3, 2012, 9:05 a.m.
On 2012-07-03 10:59, Alon Levy wrote:
> On Mon, Jul 02, 2012 at 10:20:17AM +0200, Jan Kiszka wrote:
> 
> One comment below.
> 
> Reviewed-by: Alon Levy <alevy@redhat.com>
> 
>> Let the text console cursor blink at 5 HZ.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  console.c |   26 +++++++++++++++++++++++++-
>>  1 files changed, 25 insertions(+), 1 deletions(-)
>>
>> diff --git a/console.c b/console.c
>> index 6a463f5..29b0f1c 100644
>> --- a/console.c
>> +++ b/console.c
>> @@ -28,6 +28,7 @@
>>  //#define DEBUG_CONSOLE
>>  #define DEFAULT_BACKSCROLL 512
>>  #define MAX_CONSOLES 12
>> +#define CONSOLE_CURSOR_PERIOD 200
>>  
>>  #define QEMU_RGBA(r, g, b, a) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))
>>  #define QEMU_RGB(r, g, b) QEMU_RGBA(r, g, b, 0xff)
>> @@ -139,6 +140,8 @@ struct TextConsole {
>>      TextCell *cells;
>>      int text_x[2], text_y[2], cursor_invalidate;
>>      int echo;
>> +    int cursor_blink_state;
>> +    QEMUTimer *cursor_timer;
>>  
>>      int update_x0;
>>      int update_y0;
>> @@ -615,7 +618,7 @@ static void console_show_cursor(TextConsole *s, int show)
>>              y += s->total_height;
>>          if (y < s->height) {
>>              c = &s->cells[y1 * s->width + x];
>> -            if (show) {
>> +            if (show && s->cursor_blink_state) {
>>                  TextAttributes t_attrib = s->t_attrib_default;
>>                  t_attrib.invers = !(t_attrib.invers); /* invert fg and bg */
>>                  vga_putcharxy(s->ds, x, y, c->ch, &t_attrib);
>> @@ -1083,6 +1086,10 @@ void console_select(unsigned int index)
>>      s = consoles[index];
>>      if (s) {
>>          DisplayState *ds = s->ds;
>> +
>> +        if (active_console->cursor_timer) {
>> +            qemu_del_timer(active_console->cursor_timer);
>> +        }
>>          active_console = s;
>>          if (ds_get_bits_per_pixel(s->ds)) {
>>              ds->surface = qemu_resize_displaysurface(ds, s->g_width, s->g_height);
>> @@ -1090,6 +1097,10 @@ void console_select(unsigned int index)
>>              s->ds->surface->width = s->width;
>>              s->ds->surface->height = s->height;
>>          }
>> +        if (s->cursor_timer) {
>> +            qemu_mod_timer(s->cursor_timer,
>> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
>> +        }
>>          dpy_resize(s->ds);
>>          vga_hw_invalidate();
>>      }
>> @@ -1454,6 +1465,16 @@ static void text_console_set_echo(CharDriverState *chr, bool echo)
>>      s->echo = echo;
>>  }
>>  
>> +static void text_console_update_cursor(void *opaque)
>> +{
>> +    TextConsole *s = opaque;
>> +
>> +    s->cursor_blink_state ^= 1;
>> +    vga_hw_invalidate();
>> +    qemu_mod_timer(s->cursor_timer,
>> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
>> +}
>> +
>>  static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>>  {
>>      TextConsole *s;
>> @@ -1482,6 +1503,9 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>>          s->g_height = ds_get_height(s->ds);
>>      }
>>  
>> +    s->cursor_timer =
>> +        qemu_new_timer_ms(rt_clock, text_console_update_cursor, s);
>> +
> missing initialization of cursor_blink_state

TextConsole is zero-initialized, so it does not really matter. Can add
it if preferred though.

Thanks for reviewing,
Jan
Alon Levy - July 3, 2012, 9:49 a.m.
On Tue, Jul 03, 2012 at 11:05:50AM +0200, Jan Kiszka wrote:
> On 2012-07-03 10:59, Alon Levy wrote:
> > On Mon, Jul 02, 2012 at 10:20:17AM +0200, Jan Kiszka wrote:
> > 
> > One comment below.
> > 
> > Reviewed-by: Alon Levy <alevy@redhat.com>
> > 
> >> Let the text console cursor blink at 5 HZ.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  console.c |   26 +++++++++++++++++++++++++-
> >>  1 files changed, 25 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/console.c b/console.c
> >> index 6a463f5..29b0f1c 100644
> >> --- a/console.c
> >> +++ b/console.c
> >> @@ -28,6 +28,7 @@
> >>  //#define DEBUG_CONSOLE
> >>  #define DEFAULT_BACKSCROLL 512
> >>  #define MAX_CONSOLES 12
> >> +#define CONSOLE_CURSOR_PERIOD 200
> >>  
> >>  #define QEMU_RGBA(r, g, b, a) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))
> >>  #define QEMU_RGB(r, g, b) QEMU_RGBA(r, g, b, 0xff)
> >> @@ -139,6 +140,8 @@ struct TextConsole {
> >>      TextCell *cells;
> >>      int text_x[2], text_y[2], cursor_invalidate;
> >>      int echo;
> >> +    int cursor_blink_state;
> >> +    QEMUTimer *cursor_timer;
> >>  
> >>      int update_x0;
> >>      int update_y0;
> >> @@ -615,7 +618,7 @@ static void console_show_cursor(TextConsole *s, int show)
> >>              y += s->total_height;
> >>          if (y < s->height) {
> >>              c = &s->cells[y1 * s->width + x];
> >> -            if (show) {
> >> +            if (show && s->cursor_blink_state) {
> >>                  TextAttributes t_attrib = s->t_attrib_default;
> >>                  t_attrib.invers = !(t_attrib.invers); /* invert fg and bg */
> >>                  vga_putcharxy(s->ds, x, y, c->ch, &t_attrib);
> >> @@ -1083,6 +1086,10 @@ void console_select(unsigned int index)
> >>      s = consoles[index];
> >>      if (s) {
> >>          DisplayState *ds = s->ds;
> >> +
> >> +        if (active_console->cursor_timer) {
> >> +            qemu_del_timer(active_console->cursor_timer);
> >> +        }
> >>          active_console = s;
> >>          if (ds_get_bits_per_pixel(s->ds)) {
> >>              ds->surface = qemu_resize_displaysurface(ds, s->g_width, s->g_height);
> >> @@ -1090,6 +1097,10 @@ void console_select(unsigned int index)
> >>              s->ds->surface->width = s->width;
> >>              s->ds->surface->height = s->height;
> >>          }
> >> +        if (s->cursor_timer) {
> >> +            qemu_mod_timer(s->cursor_timer,
> >> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
> >> +        }
> >>          dpy_resize(s->ds);
> >>          vga_hw_invalidate();
> >>      }
> >> @@ -1454,6 +1465,16 @@ static void text_console_set_echo(CharDriverState *chr, bool echo)
> >>      s->echo = echo;
> >>  }
> >>  
> >> +static void text_console_update_cursor(void *opaque)
> >> +{
> >> +    TextConsole *s = opaque;
> >> +
> >> +    s->cursor_blink_state ^= 1;
> >> +    vga_hw_invalidate();
> >> +    qemu_mod_timer(s->cursor_timer,
> >> +                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
> >> +}
> >> +
> >>  static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
> >>  {
> >>      TextConsole *s;
> >> @@ -1482,6 +1503,9 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
> >>          s->g_height = ds_get_height(s->ds);
> >>      }
> >>  
> >> +    s->cursor_timer =
> >> +        qemu_new_timer_ms(rt_clock, text_console_update_cursor, s);
> >> +
> > missing initialization of cursor_blink_state
> 
> TextConsole is zero-initialized, so it does not really matter. Can add
> it if preferred though.

I just thought because you initialized the same named field in the vga patch.
No need if everything is zero initialized, missed that.

> 
> Thanks for reviewing,
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
> 
> 
>
Stefan Weil - July 3, 2012, 2:41 p.m.
Am 02.07.2012 10:20, schrieb Jan Kiszka:
> Let the text console cursor blink at 5 HZ.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> console.c | 26 +++++++++++++++++++++++++-
> 1 files changed, 25 insertions(+), 1 deletions(-)
>

Hi Jan,

I know that there was a "bug" report. Nevertheless I'd like
to ask whether QEMU really needs a blinking text cursor.

Blinking cursors are a matter of taste. I personally don't
want a blinking cursors. Nor do I want programs which
wake my cpu 5 times a second for something which I
don't need.

It's possible to get a blinking cursor without your patch
by redirecting QEMU's text console to one of the common
X terminal emulations (xterm, konsole, ...).

When QEMU gets GTK support, it might use a terminal
emulation which allows configurable cursor blinking.
IMHO that would be better than implementing that feature
in QEMU code.

That's why I suggest not to apply the patch.

Cheers,

Stefan
Jan Kiszka - July 3, 2012, 2:50 p.m.
On 2012-07-03 16:41, Stefan Weil wrote:
> Am 02.07.2012 10:20, schrieb Jan Kiszka:
>> Let the text console cursor blink at 5 HZ.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> console.c | 26 +++++++++++++++++++++++++-
>> 1 files changed, 25 insertions(+), 1 deletions(-)
>>
> 
> Hi Jan,
> 
> I know that there was a "bug" report. Nevertheless I'd like
> to ask whether QEMU really needs a blinking text cursor.
> 
> Blinking cursors are a matter of taste. I personally don't
> want a blinking cursors. Nor do I want programs which
> wake my cpu 5 times a second for something which I
> don't need.
> 
> It's possible to get a blinking cursor without your patch
> by redirecting QEMU's text console to one of the common
> X terminal emulations (xterm, konsole, ...).
> 
> When QEMU gets GTK support, it might use a terminal
> emulation which allows configurable cursor blinking.
> IMHO that would be better than implementing that feature
> in QEMU code.
> 
> That's why I suggest not to apply the patch.

Well, *if* there should be heavy resistance against this patch, we may
postpone it. But I consider the feature useful to identify the cursor
position and check visually the liveliness of QEMU.

However, the VGA blinking is a feature that should go in, specifically
as there is no impact on wakeups and it fixes an emulation deficit.

Jan

Patch

diff --git a/console.c b/console.c
index 6a463f5..29b0f1c 100644
--- a/console.c
+++ b/console.c
@@ -28,6 +28,7 @@ 
 //#define DEBUG_CONSOLE
 #define DEFAULT_BACKSCROLL 512
 #define MAX_CONSOLES 12
+#define CONSOLE_CURSOR_PERIOD 200
 
 #define QEMU_RGBA(r, g, b, a) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))
 #define QEMU_RGB(r, g, b) QEMU_RGBA(r, g, b, 0xff)
@@ -139,6 +140,8 @@  struct TextConsole {
     TextCell *cells;
     int text_x[2], text_y[2], cursor_invalidate;
     int echo;
+    int cursor_blink_state;
+    QEMUTimer *cursor_timer;
 
     int update_x0;
     int update_y0;
@@ -615,7 +618,7 @@  static void console_show_cursor(TextConsole *s, int show)
             y += s->total_height;
         if (y < s->height) {
             c = &s->cells[y1 * s->width + x];
-            if (show) {
+            if (show && s->cursor_blink_state) {
                 TextAttributes t_attrib = s->t_attrib_default;
                 t_attrib.invers = !(t_attrib.invers); /* invert fg and bg */
                 vga_putcharxy(s->ds, x, y, c->ch, &t_attrib);
@@ -1083,6 +1086,10 @@  void console_select(unsigned int index)
     s = consoles[index];
     if (s) {
         DisplayState *ds = s->ds;
+
+        if (active_console->cursor_timer) {
+            qemu_del_timer(active_console->cursor_timer);
+        }
         active_console = s;
         if (ds_get_bits_per_pixel(s->ds)) {
             ds->surface = qemu_resize_displaysurface(ds, s->g_width, s->g_height);
@@ -1090,6 +1097,10 @@  void console_select(unsigned int index)
             s->ds->surface->width = s->width;
             s->ds->surface->height = s->height;
         }
+        if (s->cursor_timer) {
+            qemu_mod_timer(s->cursor_timer,
+                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
+        }
         dpy_resize(s->ds);
         vga_hw_invalidate();
     }
@@ -1454,6 +1465,16 @@  static void text_console_set_echo(CharDriverState *chr, bool echo)
     s->echo = echo;
 }
 
+static void text_console_update_cursor(void *opaque)
+{
+    TextConsole *s = opaque;
+
+    s->cursor_blink_state ^= 1;
+    vga_hw_invalidate();
+    qemu_mod_timer(s->cursor_timer,
+                   qemu_get_clock_ms(rt_clock) + CONSOLE_CURSOR_PERIOD);
+}
+
 static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
 {
     TextConsole *s;
@@ -1482,6 +1503,9 @@  static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
         s->g_height = ds_get_height(s->ds);
     }
 
+    s->cursor_timer =
+        qemu_new_timer_ms(rt_clock, text_console_update_cursor, s);
+
     s->hw_invalidate = text_console_invalidate;
     s->hw_text_update = text_console_update;
     s->hw = s;