RE: [Xen-devel] Re: [Xen-changelog] Add VGA acceleration support forcirrus logic device model

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

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@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.
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel