diff mbox series

[4/7] ati-vga: Fix cursor color with guest_hwcursor=true

Message ID d99f9e07923a74932dbb15e93dd50aa8d2816b19.1565558093.git.balaton@eik.bme.hu
State New
Headers show
Series Resend of all outstanding ati-vga patches | expand

Commit Message

BALATON Zoltan Aug. 11, 2019, 9:14 p.m. UTC
Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Aug. 12, 2019, 9:03 a.m. UTC | #1
On 8/11/19 11:14 PM, BALATON Zoltan wrote:
> Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 699f38223b..b849f5d510 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -207,7 +207,7 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
>                  }
>              } else {
>                  color = (xbits & BIT(7) ? s->regs.cur_color1 :
> -                                          s->regs.cur_color0) << 8 | 0xff;
> +                                          s->regs.cur_color0) | 0xff000000;
>              }
>              if (vga->hw_cursor_x + i * 8 + j >= h) {
>                  return; /* end of screen, don't span to next line */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
BALATON Zoltan Aug. 12, 2019, 10:28 a.m. UTC | #2
On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
> On 8/11/19 11:14 PM, BALATON Zoltan wrote:
>> Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/ati.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 699f38223b..b849f5d510 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -207,7 +207,7 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
>>                  }
>>              } else {
>>                  color = (xbits & BIT(7) ? s->regs.cur_color1 :
>> -                                          s->regs.cur_color0) << 8 | 0xff;
>> +                                          s->regs.cur_color0) | 0xff000000;
>>              }
>>              if (vga->hw_cursor_x + i * 8 + j >= h) {
>>                  return; /* end of screen, don't span to next line */
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks. I've noticed that cursor color may still be wrong with MacOS that 
uses big endian frame buffer so maybe this will also need to take frame 
buffer endianness into account somehow but this patch corrects a 
difference between guest_hwcursor true and false values, reproducible with 
some Linux versions (as far as I remember). While the wrong cursor color 
with MacOS is now consistent after this patch with both guest_hwcursor 
true or false so that likely needs a different fix that should be applied 
both to this place and to ati_cursor_define. (MacOS does not yet boot 
fully, it needs patches to OpenBIOS to be able to run an FCode ROM and 
will probably need the VBlank interrupt implemented in ati-vga without 
which it displays a desktop but freezes there).

Regards,
BALATON Zoltan
Philippe Mathieu-Daudé Aug. 12, 2019, 10:36 a.m. UTC | #3
On 8/12/19 12:28 PM, BALATON Zoltan wrote:
> On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
>> On 8/11/19 11:14 PM, BALATON Zoltan wrote:
>>> Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/display/ati.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>> index 699f38223b..b849f5d510 100644
>>> --- a/hw/display/ati.c
>>> +++ b/hw/display/ati.c
>>> @@ -207,7 +207,7 @@ static void ati_cursor_draw_line(VGACommonState
>>> *vga, uint8_t *d, int scr_y)
>>>                  }
>>>              } else {
>>>                  color = (xbits & BIT(7) ? s->regs.cur_color1 :
>>> -                                          s->regs.cur_color0) << 8 |
>>> 0xff;
>>> +                                          s->regs.cur_color0) |
>>> 0xff000000;
>>>              }
>>>              if (vga->hw_cursor_x + i * 8 + j >= h) {
>>>                  return; /* end of screen, don't span to next line */
>>>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks. I've noticed that cursor color may still be wrong with MacOS
> that uses big endian frame buffer so maybe this will also need to take
> frame buffer endianness into account somehow but this patch corrects a
> difference between guest_hwcursor true and false values, reproducible
> with some Linux versions (as far as I remember). While the wrong cursor
> color with MacOS is now consistent after this patch with both
> guest_hwcursor true or false so that likely needs a different fix that
> should be applied both to this place and to ati_cursor_define. (MacOS
> does not yet boot fully, it needs patches to OpenBIOS to be able to run
> an FCode ROM and will probably need the VBlank interrupt implemented in
> ati-vga without which it displays a desktop but freezes there).

If you remember which Linux version had this problem, or you can link to
roms that behave incorrectly, please share, so we can add display
regression tests.

Thanks,

Phil.
BALATON Zoltan Aug. 12, 2019, 10:55 a.m. UTC | #4
On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
> On 8/12/19 12:28 PM, BALATON Zoltan wrote:
>> On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
>>> On 8/11/19 11:14 PM, BALATON Zoltan wrote:
>>>> Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> ?hw/display/ati.c | 2 +-
>>>> ?1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>>> index 699f38223b..b849f5d510 100644
>>>> --- a/hw/display/ati.c
>>>> +++ b/hw/display/ati.c
>>>> @@ -207,7 +207,7 @@ static void ati_cursor_draw_line(VGACommonState
>>>> *vga, uint8_t *d, int scr_y)
>>>> ???????????????? }
>>>> ???????????? } else {
>>>> ???????????????? color = (xbits & BIT(7) ? s->regs.cur_color1 :
>>>> -????????????????????????????????????????? s->regs.cur_color0) << 8 |
>>>> 0xff;
>>>> +????????????????????????????????????????? s->regs.cur_color0) |
>>>> 0xff000000;
>>>> ???????????? }
>>>> ???????????? if (vga->hw_cursor_x + i * 8 + j >= h) {
>>>> ???????????????? return; /* end of screen, don't span to next line */
>>>>
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Thanks. I've noticed that cursor color may still be wrong with MacOS
>> that uses big endian frame buffer so maybe this will also need to take
>> frame buffer endianness into account somehow but this patch corrects a
>> difference between guest_hwcursor true and false values, reproducible
>> with some Linux versions (as far as I remember). While the wrong cursor
>> color with MacOS is now consistent after this patch with both
>> guest_hwcursor true or false so that likely needs a different fix that
>> should be applied both to this place and to ati_cursor_define. (MacOS
>> does not yet boot fully, it needs patches to OpenBIOS to be able to run
>> an FCode ROM and will probably need the VBlank interrupt implemented in
>> ati-vga without which it displays a desktop but freezes there).
>
> If you remember which Linux version had this problem, or you can link to
> roms that behave incorrectly, please share, so we can add display
> regression tests.

Unfortunately I don't. I think it was Andrew who found this so maybe he 
can remember.

(You may also need latest vgabios-ati.bin from Gerd's repo to get Linux 
drivers load and for rage128p that has to be in pc-bios dir because PCI 
IDs are only patched in that way.)

Regards,
BALATON Zoltan
Andrew Randrianasulu Aug. 12, 2019, 12:27 p.m. UTC | #5
В сообщении от Monday 12 August 2019 13:55:45 BALATON Zoltan написал(а):
> On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
> > On 8/12/19 12:28 PM, BALATON Zoltan wrote:
> >> On Mon, 12 Aug 2019, Philippe Mathieu-Daudé wrote:
> >>> On 8/11/19 11:14 PM, BALATON Zoltan wrote:
> >>>> Fixes: a38127414bd007c5b6ae64c664d9e8839393277e
> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >>>> ---
> >>>> ?hw/display/ati.c | 2 +-
> >>>> ?1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
> >>>> index 699f38223b..b849f5d510 100644
> >>>> --- a/hw/display/ati.c
> >>>> +++ b/hw/display/ati.c
> >>>> @@ -207,7 +207,7 @@ static void ati_cursor_draw_line(VGACommonState
> >>>> *vga, uint8_t *d, int scr_y)
> >>>> ???????????????? }
> >>>> ???????????? } else {
> >>>> ???????????????? color = (xbits & BIT(7) ? s->regs.cur_color1 :
> >>>> -????????????????????????????????????????? s->regs.cur_color0) << 8 |
> >>>> 0xff;
> >>>> +????????????????????????????????????????? s->regs.cur_color0) |
> >>>> 0xff000000;
> >>>> ???????????? }
> >>>> ???????????? if (vga->hw_cursor_x + i * 8 + j >= h) {
> >>>> ???????????????? return; /* end of screen, don't span to next line */
> >>>>
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>
> >> Thanks. I've noticed that cursor color may still be wrong with MacOS
> >> that uses big endian frame buffer so maybe this will also need to take
> >> frame buffer endianness into account somehow but this patch corrects a
> >> difference between guest_hwcursor true and false values, reproducible
> >> with some Linux versions (as far as I remember). While the wrong cursor
> >> color with MacOS is now consistent after this patch with both
> >> guest_hwcursor true or false so that likely needs a different fix that
> >> should be applied both to this place and to ati_cursor_define. (MacOS
> >> does not yet boot fully, it needs patches to OpenBIOS to be able to run
> >> an FCode ROM and will probably need the VBlank interrupt implemented in
> >> ati-vga without which it displays a desktop but freezes there).
> >
> > If you remember which Linux version had this problem, or you can link to
> > roms that behave incorrectly, please share, so we can add display
> > regression tests.
> 
> Unfortunately I don't. I think it was Andrew who found this so maybe he 
> can remember.

Blue cursor was seen on specific dvd (x86) image I did for myself, 
because it was  using plain X cursor (arrow or X-shaped), not colored 
versions used by default in many distributions.

may be 'startx -- -retro" will show it briefly too?

from man Xserver (1.19.7):

-retro  starts  the  server  with  the  classic stipple and cursor visible.  The default is to start with a black root window, and to suppress display of the cursor until the
               first time an application calls XDefineCursor(). 


https://yadi.sk/d/F8cbINWzWJ-DkA

users: root and guest
passwords: toor and guest

You need to boot it to syslinux and type 'slax' there, because default will boot x86-64 kernel without aty128fb .... (my fault)
Unfortunately, i don't have fresh qemu compiled (previous tests were done from tmpfs copy of qemu source tree),
 will recompile from git and re-test. from memory, loading aty128fb and
setting config fragment in /etc/X11/xorg.conf.d for EXA AccelMethod and "Option "UseFBDev" "1" ' allowed device (ati-vga) to work.



> 
> (You may also need latest vgabios-ati.bin from Gerd's repo to get Linux 
> drivers load and for rage128p that has to be in pc-bios dir because PCI 
> IDs are only patched in that way.)
> 
> Regards,
> BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 699f38223b..b849f5d510 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -207,7 +207,7 @@  static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
                 }
             } else {
                 color = (xbits & BIT(7) ? s->regs.cur_color1 :
-                                          s->regs.cur_color0) << 8 | 0xff;
+                                          s->regs.cur_color0) | 0xff000000;
             }
             if (vga->hw_cursor_x + i * 8 + j >= h) {
                 return; /* end of screen, don't span to next line */