Re: [Qemu-devel] [5336] Optional "precise" VGA retrace support

[Krumme, Chris]

Hello,

Could we get better comments here.

I know most people are experts on VGA and the exact use of
retrace... :-)

Especially when adding a new option there needs to be documentation
about the new option.

The options parsing code can lead you to the invalid vga tag, but there
is not clue on what is valid, except by reading the code.  The current
error message would be "Unknown vga type: std", when the error had to do
with precise.

Thanks

Chris

On Sun, 2008-09-28 at 00:42 +0000, malc wrote:
> Revision: 5336
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5336
> Author:   malc
> Date:     2008-09-28 00:42:12 +0000 (Sun, 28 Sep 2008)
> 
> Log Message:
> -----------
> Optional "precise" VGA retrace support
> 
> Selected via: -vga <name>,retrace=precise
> 
> Modified Paths:
> --------------
>     trunk/hw/cirrus_vga.c
>     trunk/hw/pc.h
>     trunk/hw/vga.c
>     trunk/hw/vga_int.h
>     trunk/vl.c
> 
> Modified: trunk/hw/cirrus_vga.c
> ===================================================================
> --- trunk/hw/cirrus_vga.c     2008-09-28 00:42:05 UTC (rev 5335)
> +++ trunk/hw/cirrus_vga.c     2008-09-28 00:42:12 UTC (rev 5336)
> @@ -2744,8 +2744,7 @@
>       case 0x3ba:
>       case 0x3da:
>           /* just toggle to fool polling */
> -         s->st01 ^= ST01_V_RETRACE | ST01_DISP_ENABLE;
> -         val = s->st01;
> +         val = s->st01 = s->retrace((VGAState *) s);
>           s->ar_flip_flop = 0;
>           break;
>       default:
> @@ -2808,6 +2807,7 @@
>       break;
>      case 0x3c2:
>       s->msr = val & ~0x10;
> +     s->update_retrace_info((VGAState *) s);
>       break;
>      case 0x3c4:
>       s->sr_index = val;
> @@ -2819,6 +2819,7 @@
>       printf("vga: write SR%x = 0x%02x\n", s->sr_index, val);
>  #endif
>       s->sr[s->sr_index] = val & sr_mask[s->sr_index];
> +     if (s->sr_index == 1) s->update_retrace_info((VGAState *) s);
>       break;
>      case 0x3c6:
>       cirrus_write_hidden_dac(s, val);
> @@ -2886,6 +2887,18 @@
>           s->cr[s->cr_index] = val;
>           break;
>       }
> +
> +     switch(s->cr_index) {
> +     case 0x00:
> +     case 0x04:
> +     case 0x05:
> +     case 0x06:
> +     case 0x07:
> +     case 0x11:
> +     case 0x17:
> +         s->update_retrace_info((VGAState *) s);
> +         break;
> +     }
>       break;
>      case 0x3ba:
>      case 0x3da:
> 
> Modified: trunk/hw/pc.h
> ===================================================================
> --- trunk/hw/pc.h     2008-09-28 00:42:05 UTC (rev 5335)
> +++ trunk/hw/pc.h     2008-09-28 00:42:12 UTC (rev 5336)
> @@ -108,7 +108,13 @@
>  int piix4_init(PCIBus *bus, int devfn);
>  
>  /* vga.c */
> +enum vga_retrace_method {
> +    VGA_RETRACE_DUMB,
> +    VGA_RETRACE_PRECISE
> +};
>  
> +extern enum vga_retrace_method vga_retrace_method;
> +
>  #ifndef TARGET_SPARC
>  #define VGA_RAM_SIZE (8192 * 1024)
>  #else
> 
> Modified: trunk/hw/vga.c
> ===================================================================
> --- trunk/hw/vga.c    2008-09-28 00:42:05 UTC (rev 5335)
> +++ trunk/hw/vga.c    2008-09-28 00:42:12 UTC (rev 5336)
> @@ -27,6 +27,7 @@
>  #include "pci.h"
>  #include "vga_int.h"
>  #include "pixel_ops.h"
> +#include "qemu-timer.h"
>  
>  //#define DEBUG_VGA
>  //#define DEBUG_VGA_MEM
> @@ -149,6 +150,139 @@
>  
>  static void vga_screen_dump(void *opaque, const char *filename);
>  
> +static void vga_dumb_update_retrace_info(VGAState *s)
> +{
> +    (void) s;
> +}
> +
> +static void vga_precise_update_retrace_info(VGAState *s)
> +{
> +    int htotal_chars;
> +    int hretr_start_char;
> +    int hretr_skew_chars;
> +    int hretr_end_char;
> +
> +    int vtotal_lines;
> +    int vretr_start_line;
> +    int vretr_end_line;
> +
> +    int div2, sldiv2, dots;
> +    int clocking_mode;
> +    int clock_sel;
> +    const int hz[] = {25175000, 28322000, 25175000, 25175000};
> +    int64_t chars_per_sec;
> +    struct vga_precise_retrace *r = &s->retrace_info.precise;
> +
> +    htotal_chars = s->cr[0x00] + 5;
> +    hretr_start_char = s->cr[0x04];
> +    hretr_skew_chars = (s->cr[0x05] >> 5) & 3;
> +    hretr_end_char = s->cr[0x05] & 0x1f;
> +
> +    vtotal_lines = (s->cr[0x06]
> +                    | (((s->cr[0x07] & 1) | ((s->cr[0x07] >> 4) & 2)) << 8)) 
> + 2
> +        ;
> +    vretr_start_line = s->cr[0x10]
> +        | ((((s->cr[0x07] >> 2) & 1) | ((s->cr[0x07] >> 6) & 2)) << 8)
> +        ;
> +    vretr_end_line = s->cr[0x11] & 0xf;
> +
> +
> +    div2 = (s->cr[0x17] >> 2) & 1;
> +    sldiv2 = (s->cr[0x17] >> 3) & 1;
> +
> +    clocking_mode = (s->sr[0x01] >> 3) & 1;
> +    clock_sel = (s->msr >> 2) & 3;
> +    dots = (s->msr & 1) ? 9 : 8;
> +
> +    chars_per_sec = hz[clock_sel] / dots;
> +
> +    htotal_chars <<= clocking_mode;
> +
> +    r->total_chars = vtotal_lines * htotal_chars;
> +    r->total_chars = (vretr_start_line + vretr_end_line + 1) * htotal_chars;
> +    if (r->freq) {
> +        r->ticks_per_char = ticks_per_sec / (r->total_chars * r->freq);
> +    } else {
> +        r->ticks_per_char = ticks_per_sec / chars_per_sec;
> +    }
> +
> +    r->vstart = vretr_start_line;
> +    r->vend = r->vstart + vretr_end_line + 1;
> +
> +    r->hstart = hretr_start_char + hretr_skew_chars;
> +    r->hend = r->hstart + hretr_end_char + 1;
> +    r->htotal = htotal_chars;
> +
> +    printf("hz=%f\n",
> +           (double) ticks_per_sec / (r->ticks_per_char * r->total_chars));
> +#if 0 /* def DEBUG_RETRACE */
> +    printf("hz=%f\n",
> +           (double) ticks_per_sec / (r->ticks_per_char * r->total_chars));
> +    printf (
> +        "htotal = %d\n"
> +        "hretr_start = %d\n"
> +        "hretr_skew = %d\n"
> +        "hretr_end = %d\n"
> +        "vtotal = %d\n"
> +        "vretr_start = %d\n"
> +        "vretr_end = %d\n"
> +        "div2 = %d sldiv2 = %d\n"
> +        "clocking_mode = %d\n"
> +        "clock_sel = %d %d\n"
> +        "dots = %d\n"
> +        "ticks/char = %lld\n"
> +        "\n",
> +        htotal_chars,
> +        hretr_start_char,
> +        hretr_skew_chars,
> +        hretr_end_char,
> +        vtotal_lines,
> +        vretr_start_line,
> +        vretr_end_line,
> +        div2, sldiv2,
> +        clocking_mode,
> +        clock_sel,
> +        hz[clock_sel],
> +        dots,
> +        r->ticks_per_char
> +        );
> +#endif
> +}
> +
> +static uint8_t vga_precise_retrace(VGAState *s)
> +{
> +    struct vga_precise_retrace *r = &s->retrace_info.precise;
> +    uint8_t val = s->st01 & ~(ST01_V_RETRACE | ST01_DISP_ENABLE);
> +
> +    if (r->total_chars) {
> +        int cur_line, cur_line_char, cur_char;
> +        int64_t cur_tick;
> +
> +        cur_tick = qemu_get_clock(vm_clock);
> +
> +        cur_char = (cur_tick / r->ticks_per_char) % r->total_chars;
> +        cur_line = cur_char / r->htotal;
> +
> +        if (cur_line >= r->vstart && cur_line <= r->vend) {
> +            val |= ST01_V_RETRACE | ST01_DISP_ENABLE;
> +        }
> +
> +        cur_line_char = cur_char % r->htotal;
> +        if (cur_line_char >= r->hstart && cur_line_char <= r->hend) {
> +            val |= ST01_DISP_ENABLE;
> +        }
> +
> +        return val;
> +    } else {
> +        return s->st01 ^ (ST01_V_RETRACE | ST01_DISP_ENABLE);
> +    }
> +}
> +
> +static uint8_t vga_dumb_retrace(VGAState *s)
> +{
> +    return s->st01 ^ (ST01_V_RETRACE | ST01_DISP_ENABLE);
> +}
> +
>  static uint32_t vga_ioport_read(void *opaque, uint32_t addr)
>  {
>      VGAState *s = opaque;
> @@ -228,8 +362,7 @@
>          case 0x3ba:
>          case 0x3da:
>              /* just toggle to fool polling */
> -            s->st01 ^= ST01_V_RETRACE | ST01_DISP_ENABLE;
> -            val = s->st01;
> +            val = s->st01 = s->retrace(s);
>              s->ar_flip_flop = 0;
>              break;
>          default:
> @@ -291,6 +424,7 @@
>          break;
>      case 0x3c2:
>          s->msr = val & ~0x10;
> +        s->update_retrace_info(s);
>          break;
>      case 0x3c4:
>          s->sr_index = val & 7;
> @@ -300,6 +434,7 @@
>          printf("vga: write SR%x = 0x%02x\n", s->sr_index, val);
>  #endif
>          s->sr[s->sr_index] = val & sr_mask[s->sr_index];
> +        if (s->sr_index == 1) s->update_retrace_info(s);
>          break;
>      case 0x3c7:
>          s->dac_read_index = val;
> @@ -357,6 +492,18 @@
>              s->cr[s->cr_index] = val;
>              break;
>          }
> +
> +        switch(s->cr_index) {
> +        case 0x00:
> +        case 0x04:
> +        case 0x05:
> +        case 0x06:
> +        case 0x07:
> +        case 0x11:
> +        case 0x17:
> +            s->update_retrace_info(s);
> +            break;
> +        }
>          break;
>      case 0x3ba:
>      case 0x3da:
> @@ -2001,6 +2148,18 @@
>      s->invalidate = vga_invalidate_display;
>      s->screen_dump = vga_screen_dump;
>      s->text_update = vga_update_text;
> +    switch (vga_retrace_method) {
> +    case VGA_RETRACE_DUMB:
> +        s->retrace = vga_dumb_retrace;
> +        s->update_retrace_info = vga_dumb_update_retrace_info;
> +        break;
> +
> +    case VGA_RETRACE_PRECISE:
> +        s->retrace = vga_precise_retrace;
> +        s->update_retrace_info = vga_precise_update_retrace_info;
> +        memset(&s->retrace_info, 0, sizeof (s->retrace_info));
> +        break;
> +    }
>  }
>  
>  /* used by both ISA and PCI */
> 
> Modified: trunk/hw/vga_int.h
> ===================================================================
> --- trunk/hw/vga_int.h        2008-09-28 00:42:05 UTC (rev 5335)
> +++ trunk/hw/vga_int.h        2008-09-28 00:42:12 UTC (rev 5336)
> @@ -79,6 +79,25 @@
>  #define CH_ATTR_SIZE (160 * 100)
>  #define VGA_MAX_HEIGHT 2048
>  
> +struct vga_precise_retrace {
> +    int64_t ticks_per_char;
> +    int64_t total_chars;
> +    int htotal;
> +    int hstart;
> +    int hend;
> +    int vstart;
> +    int vend;
> +    int freq;
> +};
> +
> +union vga_retrace {
> +    struct vga_precise_retrace precise;
> +};
> +
> +struct VGAState;
> +typedef uint8_t (* vga_retrace_fn)(struct VGAState *s);
> +typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
> +
>  #define VGA_STATE_COMMON                                                \
>      uint8_t *vram_ptr;                                                  \
>      unsigned long vram_offset;                                          \
> @@ -147,7 +166,11 @@
>      void (*cursor_draw_line)(struct VGAState *s, uint8_t *d, int y);    \
>      /* tell for each page if it has been updated since the last time */ \
>      uint32_t last_palette[256];                                         \
> -    uint32_t last_ch_attr[CH_ATTR_SIZE]; /* XXX: make it dynamic */
> +    uint32_t last_ch_attr[CH_ATTR_SIZE]; /* XXX: make it dynamic */     \
> +    /* retrace */                                                       \
> +    vga_retrace_fn retrace;                                             \
> +    vga_update_retrace_info_fn update_retrace_info;                     \
> +    union vga_retrace retrace_info;
>  
> 
>  typedef struct VGAState {
> 
> Modified: trunk/vl.c
> ===================================================================
> --- trunk/vl.c        2008-09-28 00:42:05 UTC (rev 5335)
> +++ trunk/vl.c        2008-09-28 00:42:12 UTC (rev 5336)
> @@ -174,6 +174,7 @@
>  /* point to the block driver where the snapshots are managed */
>  BlockDriverState *bs_snapshots;
>  int vga_ram_size;
> +enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>  static DisplayState display_state;
>  int nographic;
>  int curses;
> @@ -8203,7 +8204,19 @@
>          fprintf(stderr, "Unknown vga type: %s\n", p);
>          exit(1);
>      }
> -    if (*opts) goto invalid_vga;
> +    while (*opts) {
> +        const char *nextopt;
> +
> +        if (strstart(opts, ",retrace=", &nextopt)) {
> +            opts = nextopt;
> +            if (strstart(opts, "dumb", &nextopt))
> +                vga_retrace_method = VGA_RETRACE_DUMB;
> +            else if (strstart(opts, "precise", &nextopt))
> +                vga_retrace_method = VGA_RETRACE_PRECISE;
> +            else goto invalid_vga;
> +        } else goto invalid_vga;
> +        opts = nextopt;
> +    }
>  }
>  
>  #ifdef _WIN32
> 
> 
> 
>