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.kis...@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