Hollis Blanchard
2005-Sep-23 15:16 UTC
[Xen-devel] Re: [Xen-changelog] Add VGA acceleration support for cirrus logic device model
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@firebug.cl.cam.ac.uk > # Node ID 8a757f283fb8013e220bd89848d78fdbd2362195 > # Parent 94c6fc048d8ed548b552be415f23c68162f30ab0 > Add VGA acceleration support for cirrus logic device model > > Signed-off-by: Xiaofeng Ling <xiaofeng.ling@intel.com> > Signed-off-by: Yunhong Jiang <yunhong.jiang@intel.com> > Signed-off-by: Asit Mallick <asit.k.mallick@intel.com> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel