Jiang, Yunhong
2005-Sep-25 16:07 UTC
RE: [Xen-devel] Re: [Xen-changelog] Add VGA acceleration support forcirrus logic device model
Hollis Blanchard wrote:> 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.I will change it later. Thanks very much.> >> 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?Thanks for your suggestion very much, but I think it will not be some #define just for this part, but a totoally clean for the whole cirrus_vga.c/vga.c, because too much bit operation for vga code and all of them are hardcoded. ( I dind''t change the code, but just a few tabs->space changes) . Just concern that if we change the DM code too much, it will cause difficult when updating to newer qemu code (maybe I should left this code unchanged, although it makes the patch has a bit problem on format).> >> 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.No, this full_update is needed here, before we provide a better solution for it. The reason is, currently, the vga_draw_*() will only update those field that has changed since last refresh. However, when we add the vga acceleration, the framebuffer is accessed by guest directly, the DM will have no idea what part is changed, so the DM need to update every time. One direct result whithout this change is, when windows, sometimes the window will refresh just 1/3 screen. Another solution is to using the shadow page to notify the DM what part is changed. It is much more complex, and is still on way. But this small change also provide accepetable, or it can be called good user experience :-)> >> @@ -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..._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel