Re: [Qemu-devel] [PATCH v2] vga: Implement blinking of text cursor

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux