Patchwork [v2] vga: Implement blinking of text cursor

login
register
mail settings
Submitter Jan Kiszka
Date July 4, 2012, 1:40 p.m.
Message ID <4FF447B9.2010103@siemens.com>
Download mbox | patch
Permalink /patch/168985/
State New
Headers show

Comments

Jan Kiszka - July 4, 2012, 1:40 p.m.
Let the text cursor blink at 5 Hz. No timer is used, instead we rely on
the fact that the display is updated periodically.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - avoid full screen updates for cursor blinking
 - convert cursor_blink_state to cursor_visible_phase

 hw/vga.c     |   14 ++++++++++++--
 hw/vga_int.h |    2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)
Stefan Weil - July 4, 2012, 4:26 p.m.
Hi Jan,

please see my comments below.


Am 04.07.2012 15:40, schrieb Jan Kiszka:
> Let the text cursor blink at 5 Hz. No timer is used, instead we rely on
> the fact that the display is updated periodically.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
> Changes in v2:
>   - avoid full screen updates for cursor blinking
>   - convert cursor_blink_state to cursor_visible_phase
>
>   hw/vga.c     |   14 ++++++++++++--
>   hw/vga_int.h |    2 ++
>   2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vga.c b/hw/vga.c
> index acb3f7d..a1644f3 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -38,6 +38,8 @@
>
>   //#define DEBUG_BOCHS_VBE
>
> +#define VGA_TEXT_CURSOR_PERIOD  200
> +
>   /*
>    * Video Graphics Array (VGA)
>    *
> @@ -1300,6 +1302,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>       uint32_t *ch_attr_ptr;
>       vga_draw_glyph8_func *vga_draw_glyph8;
>       vga_draw_glyph9_func *vga_draw_glyph9;
> +    int64_t now = qemu_get_clock_ms(vm_clock);
>
>       /* compute font data address (in plane 2) */
>       v = s->sr[VGA_SEQ_CHARACTER_MAP];
> @@ -1370,6 +1373,10 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>           s->cursor_end = s->cr[VGA_CRTC_CURSOR_END];
>       }
>       cursor_ptr = s->vram_ptr + (s->start_addr + cursor_offset) * 4;
> +    if (now>= s->cursor_blink_time) {
> +        s->cursor_blink_time = now + VGA_TEXT_CURSOR_PERIOD;
> +        s->cursor_visible_phase = !s->cursor_visible_phase;
> +    }
>    

It's named VGA_TEXT_CURSOR_PERIOD, but it looks like it is not the
period, but the time for each of two states (on/off), so it's 1/2 the 
period.

With your patch, I'd expect a blink frequency of 2.5 Hz.

According to the VGA documentation (I used
http://www.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/textcur.htm#blink),
the time for each state should be 16 / (60 Hz) or 266 ms resulting in
a blink frequency of 1.875 Hz.


>
>       depth_index = get_depth_index(s->ds);
>       if (cw == 16)
> @@ -1390,7 +1397,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>           cx_max = -1;
>           for(cx = 0; cx<  width; cx++) {
>               ch_attr = *(uint16_t *)src;
> -            if (full_update || ch_attr != *ch_attr_ptr) {
> +            if (full_update || ch_attr != *ch_attr_ptr || src == cursor_ptr) {
>                   if (cx<  cx_min)
>                       cx_min = cx;
>                   if (cx>  cx_max)
> @@ -1420,7 +1427,8 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>                                       font_ptr, cheight, fgcol, bgcol, dup9);
>                   }
>                   if (src == cursor_ptr&&
> -                    !(s->cr[VGA_CRTC_CURSOR_START]&  0x20)) {
> +                    !(s->cr[VGA_CRTC_CURSOR_START]&  0x20)&&
> +                    s->cursor_visible_phase) {
>                       int line_start, line_last, h;
>                       /* draw the cursor */
>                       line_start = s->cr[VGA_CRTC_CURSOR_START]&  0x1f;
> @@ -1884,6 +1892,8 @@ static void vga_update_display(void *opaque)
>           }
>           if (graphic_mode != s->graphic_mode) {
>               s->graphic_mode = graphic_mode;
> +            s->cursor_blink_time =
> +                qemu_get_clock_ms(vm_clock) + VGA_TEXT_CURSOR_PERIOD;
>               full_update = 1;
>           }
>           switch(graphic_mode) {
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 3b38764..8938093 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -156,6 +156,8 @@ typedef struct VGACommonState {
>       uint32_t last_scr_width, last_scr_height; /* in pixels */
>       uint32_t last_depth; /* in bits */
>       uint8_t cursor_start, cursor_end;
> +    bool cursor_visible_phase;
> +    int64_t cursor_blink_time;
>       uint32_t cursor_offset;
>       unsigned int (*rgb_to_pixel)(unsigned int r,
>                                    unsigned int g, unsigned b);
>    

As far as I can tell, there are more cursor features missing:
do we support an invisible cursor (your patch doesn't) or
cursor shape? See also this URL for more information:
http://www.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/textcur.htm

Blinking is also missing for text with the blink attribute.
I don't know whether real hardware synchronizes text and
cursor blinking. If it does, we should do that, too.

What about vertical blank interrupt? It is the base of text
cursor blinking. We don't support it currently.

Regards,
Stefan
Jan Kiszka - July 4, 2012, 5:45 p.m.
On 2012-07-04 18:26, Stefan Weil wrote:
> Hi Jan,
> 
> please see my comments below.
> 
> 
> Am 04.07.2012 15:40, schrieb Jan Kiszka:
>> Let the text cursor blink at 5 Hz. No timer is used, instead we rely on
>> the fact that the display is updated periodically.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v2:
>>   - avoid full screen updates for cursor blinking
>>   - convert cursor_blink_state to cursor_visible_phase
>>
>>   hw/vga.c     |   14 ++++++++++++--
>>   hw/vga_int.h |    2 ++
>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vga.c b/hw/vga.c
>> index acb3f7d..a1644f3 100644
>> --- a/hw/vga.c
>> +++ b/hw/vga.c
>> @@ -38,6 +38,8 @@
>>
>>   //#define DEBUG_BOCHS_VBE
>>
>> +#define VGA_TEXT_CURSOR_PERIOD  200
>> +
>>   /*
>>    * Video Graphics Array (VGA)
>>    *
>> @@ -1300,6 +1302,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>>       uint32_t *ch_attr_ptr;
>>       vga_draw_glyph8_func *vga_draw_glyph8;
>>       vga_draw_glyph9_func *vga_draw_glyph9;
>> +    int64_t now = qemu_get_clock_ms(vm_clock);
>>
>>       /* compute font data address (in plane 2) */
>>       v = s->sr[VGA_SEQ_CHARACTER_MAP];
>> @@ -1370,6 +1373,10 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>>           s->cursor_end = s->cr[VGA_CRTC_CURSOR_END];
>>       }
>>       cursor_ptr = s->vram_ptr + (s->start_addr + cursor_offset) * 4;
>> +    if (now>= s->cursor_blink_time) {
>> +        s->cursor_blink_time = now + VGA_TEXT_CURSOR_PERIOD;
>> +        s->cursor_visible_phase = !s->cursor_visible_phase;
>> +    }
>>    
> 
> It's named VGA_TEXT_CURSOR_PERIOD, but it looks like it is not the
> period, but the time for each of two states (on/off), so it's 1/2 the 
> period.

Err, of course.

> 
> With your patch, I'd expect a blink frequency of 2.5 Hz.
> 
> According to the VGA documentation (I used
> http://www.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/textcur.htm#blink),
> the time for each state should be 16 / (60 Hz) or 266 ms resulting in
> a blink frequency of 1.875 Hz.

Interesting. The real hardware I was looking at is using a rate
definitely larger than 2 HZ. But I can change this to the original rate.

> 
> 
>>
>>       depth_index = get_depth_index(s->ds);
>>       if (cw == 16)
>> @@ -1390,7 +1397,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>>           cx_max = -1;
>>           for(cx = 0; cx<  width; cx++) {
>>               ch_attr = *(uint16_t *)src;
>> -            if (full_update || ch_attr != *ch_attr_ptr) {
>> +            if (full_update || ch_attr != *ch_attr_ptr || src == cursor_ptr) {
>>                   if (cx<  cx_min)
>>                       cx_min = cx;
>>                   if (cx>  cx_max)
>> @@ -1420,7 +1427,8 @@ static void vga_draw_text(VGACommonState *s, int full_update)
>>                                       font_ptr, cheight, fgcol, bgcol, dup9);
>>                   }
>>                   if (src == cursor_ptr&&
>> -                    !(s->cr[VGA_CRTC_CURSOR_START]&  0x20)) {
>> +                    !(s->cr[VGA_CRTC_CURSOR_START]&  0x20)&&
>> +                    s->cursor_visible_phase) {
>>                       int line_start, line_last, h;
>>                       /* draw the cursor */
>>                       line_start = s->cr[VGA_CRTC_CURSOR_START]&  0x1f;
>> @@ -1884,6 +1892,8 @@ static void vga_update_display(void *opaque)
>>           }
>>           if (graphic_mode != s->graphic_mode) {
>>               s->graphic_mode = graphic_mode;
>> +            s->cursor_blink_time =
>> +                qemu_get_clock_ms(vm_clock) + VGA_TEXT_CURSOR_PERIOD;
>>               full_update = 1;
>>           }
>>           switch(graphic_mode) {
>> diff --git a/hw/vga_int.h b/hw/vga_int.h
>> index 3b38764..8938093 100644
>> --- a/hw/vga_int.h
>> +++ b/hw/vga_int.h
>> @@ -156,6 +156,8 @@ typedef struct VGACommonState {
>>       uint32_t last_scr_width, last_scr_height; /* in pixels */
>>       uint32_t last_depth; /* in bits */
>>       uint8_t cursor_start, cursor_end;
>> +    bool cursor_visible_phase;
>> +    int64_t cursor_blink_time;
>>       uint32_t cursor_offset;
>>       unsigned int (*rgb_to_pixel)(unsigned int r,
>>                                    unsigned int g, unsigned b);
>>    
> 
> As far as I can tell, there are more cursor features missing:
> do we support an invisible cursor (your patch doesn't) or
> cursor shape? See also this URL for more information:
> http://www.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/textcur.htm
> 
> Blinking is also missing for text with the blink attribute.
> I don't know whether real hardware synchronizes text and
> cursor blinking. If it does, we should do that, too.
> 
> What about vertical blank interrupt? It is the base of text
> cursor blinking. We don't support it currently.

As always: patches are welcome :). This covers at least the most
prominent use case.

Thanks,
Jan

Patch

diff --git a/hw/vga.c b/hw/vga.c
index acb3f7d..a1644f3 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -38,6 +38,8 @@ 
 
 //#define DEBUG_BOCHS_VBE
 
+#define VGA_TEXT_CURSOR_PERIOD  200
+
 /*
  * Video Graphics Array (VGA)
  *
@@ -1300,6 +1302,7 @@  static void vga_draw_text(VGACommonState *s, int full_update)
     uint32_t *ch_attr_ptr;
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
+    int64_t now = qemu_get_clock_ms(vm_clock);
 
     /* compute font data address (in plane 2) */
     v = s->sr[VGA_SEQ_CHARACTER_MAP];
@@ -1370,6 +1373,10 @@  static void vga_draw_text(VGACommonState *s, int full_update)
         s->cursor_end = s->cr[VGA_CRTC_CURSOR_END];
     }
     cursor_ptr = s->vram_ptr + (s->start_addr + cursor_offset) * 4;
+    if (now >= s->cursor_blink_time) {
+        s->cursor_blink_time = now + VGA_TEXT_CURSOR_PERIOD;
+        s->cursor_visible_phase = !s->cursor_visible_phase;
+    }
 
     depth_index = get_depth_index(s->ds);
     if (cw == 16)
@@ -1390,7 +1397,7 @@  static void vga_draw_text(VGACommonState *s, int full_update)
         cx_max = -1;
         for(cx = 0; cx < width; cx++) {
             ch_attr = *(uint16_t *)src;
-            if (full_update || ch_attr != *ch_attr_ptr) {
+            if (full_update || ch_attr != *ch_attr_ptr || src == cursor_ptr) {
                 if (cx < cx_min)
                     cx_min = cx;
                 if (cx > cx_max)
@@ -1420,7 +1427,8 @@  static void vga_draw_text(VGACommonState *s, int full_update)
                                     font_ptr, cheight, fgcol, bgcol, dup9);
                 }
                 if (src == cursor_ptr &&
-                    !(s->cr[VGA_CRTC_CURSOR_START] & 0x20)) {
+                    !(s->cr[VGA_CRTC_CURSOR_START] & 0x20) &&
+                    s->cursor_visible_phase) {
                     int line_start, line_last, h;
                     /* draw the cursor */
                     line_start = s->cr[VGA_CRTC_CURSOR_START] & 0x1f;
@@ -1884,6 +1892,8 @@  static void vga_update_display(void *opaque)
         }
         if (graphic_mode != s->graphic_mode) {
             s->graphic_mode = graphic_mode;
+            s->cursor_blink_time =
+                qemu_get_clock_ms(vm_clock) + VGA_TEXT_CURSOR_PERIOD;
             full_update = 1;
         }
         switch(graphic_mode) {
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 3b38764..8938093 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -156,6 +156,8 @@  typedef struct VGACommonState {
     uint32_t last_scr_width, last_scr_height; /* in pixels */
     uint32_t last_depth; /* in bits */
     uint8_t cursor_start, cursor_end;
+    bool cursor_visible_phase;
+    int64_t cursor_blink_time;
     uint32_t cursor_offset;
     unsigned int (*rgb_to_pixel)(unsigned int r,
                                  unsigned int g, unsigned b);