[Xen-devel] Re: [Xen-changelog] Add VGA acceleration support for cirrus logic device model

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

I didn't review the whole patch, but some comments from a quick skim:

On Sep 23, 2005, at 7:46 AM, Xen patchbot -unstable wrote:

> # HG changeset patch
> # User kaf24@xxxxxxxxxxxxxxxxxxxx
> # Node ID 8a757f283fb8013e220bd89848d78fdbd2362195
> # Parent  94c6fc048d8ed548b552be415f23c68162f30ab0
> Add VGA acceleration support for cirrus logic device model
>
> Signed-off-by: Xiaofeng Ling <xiaofeng.ling@xxxxxxxxx>
> Signed-off-by: Yunhong Jiang <yunhong.jiang@xxxxxxxxx>
> Signed-off-by: Asit Mallick  <asit.k.mallick@xxxxxxxxx>
>
> diff -r 94c6fc048d8e -r 8a757f283fb8 tools/ioemu/hw/cirrus_vga.c
> --- a/tools/ioemu/hw/cirrus_vga.c       Fri Sep 23 11:52:43 2005
> +++ b/tools/ioemu/hw/cirrus_vga.c       Fri Sep 23 12:30:54 2005
> @@ -2447,6 +2449,10 @@
>  {
>      unsigned mode;
>
> +    extern void unset_vram_mapping(unsigned long addr, unsigned long 
> end);
> +    extern void set_vram_mapping(unsigned long addr, unsigned long 
> end);
> +    extern int vga_accelerate;
It would be better style to move this stuff into a header. All too 
often function prototypes are changed without finding and updating all 
the local declarations.
>      if ((s->sr[0x17] & 0x44) == 0x44) {
>          goto generic_io;
>      } else if (s->cirrus_srcptr != s->cirrus_srcptr_end) {
> @@ -2454,17 +2460,21 @@
>      } else {
>         if ((s->gr[0x0B] & 0x14) == 0x14) {
>              goto generic_io;
> -       } else if (s->gr[0x0B] & 0x02) {
> -            goto generic_io;
> -        }
> -
> -       mode = s->gr[0x05] & 0x7;
> -       if (mode < 4 || mode > 5 || ((s->gr[0x0B] & 0x4) == 0)) {
> +    } else if (s->gr[0x0B] & 0x02) {
> +        goto generic_io;
> +    }
> +
> +    mode = s->gr[0x05] & 0x7;
> +    if (mode < 4 || mode > 5 || ((s->gr[0x0B] & 0x4) == 0)) {
> +            if (vga_accelerate && s->cirrus_lfb_addr && 
> s->cirrus_lfb_end)
> +                set_vram_mapping(s->cirrus_lfb_addr, 
> s->cirrus_lfb_end);
That's an awful lot of magic numbers. Some #defines please?

> diff -r 94c6fc048d8e -r 8a757f283fb8 tools/ioemu/hw/vga.c
> --- a/tools/ioemu/hw/vga.c      Fri Sep 23 11:52:43 2005
> +++ b/tools/ioemu/hw/vga.c      Fri Sep 23 12:30:54 2005
> @@ -1568,6 +1568,8 @@
>              s->graphic_mode = graphic_mode;
>              full_update = 1;
>          }
> +
> +        full_update = 1;
>          switch(graphic_mode) {
>          case GMODE_TEXT:
>              vga_draw_text(s, full_update);
That "full_update" stuff should be removed entirely, and vga_draw_*() 
functions updated to match. It's worthless code now.
> @@ -1848,6 +1850,7 @@
>                       unsigned long vga_ram_offset, int vga_ram_size)
>  {
>      int i, j, v, b;
> +    extern void* shared_vram;
>
>      for(i = 0;i < 256; i++) {
>          v = 0;
Again, belongs in a header file...

--
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel