1. Patchew
  2. QEMU
  3. View series

[PATCH] ati-vga: Separate default control bit for source

BALATON Zoltan posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251007195435.8ACAC56F2A3@zero.eik.bme.hu
hw/display/ati_2d.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
[PATCH] ati-vga: Separate default control bit for source
Posted by BALATON Zoltan 1 month, 1 week ago 
The DP_GUI_MASTER_CNTL register has separate bits for src and dest but
we were only looking at the dest bit. Use the correct bit for source.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati_2d.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 309bb5ccb6..e69b15b570 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -43,7 +43,8 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
     }
 }
 
-#define DEFAULT_CNTL (s->regs.dp_gui_master_cntl & GMC_DST_PITCH_OFFSET_CNTL)
+#define DFLT_CNTL_SRC (s->regs.dp_gui_master_cntl & GMC_SRC_PITCH_OFFSET_CNTL)
+#define DFLT_CNTL_DST (s->regs.dp_gui_master_cntl & GMC_DST_PITCH_OFFSET_CNTL)
 
 void ati_2d_blt(ATIVGAState *s)
 {
@@ -63,12 +64,12 @@ void ati_2d_blt(ATIVGAState *s)
         qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
         return;
     }
-    int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : s->regs.default_pitch;
+    int dst_stride = DFLT_CNTL_DST ? s->regs.dst_pitch : s->regs.default_pitch;
     if (!dst_stride) {
         qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
         return;
     }
-    uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
+    uint8_t *dst_bits = s->vga.vram_ptr + (DFLT_CNTL_DST ?
                         s->regs.dst_offset : s->regs.default_offset);
 
     if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
@@ -97,13 +98,13 @@ void ati_2d_blt(ATIVGAState *s)
                        s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
         unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
                        s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
-        int src_stride = DEFAULT_CNTL ?
+        int src_stride = DFLT_CNTL_SRC ?
                          s->regs.src_pitch : s->regs.default_pitch;
         if (!src_stride) {
             qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
             return;
         }
-        uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
+        uint8_t *src_bits = s->vga.vram_ptr + (DFLT_CNTL_SRC ?
                             s->regs.src_offset : s->regs.default_offset);
 
         if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
-- 
2.41.3
Re: [PATCH] ati-vga: Separate default control bit for source
Posted by Chad Jablonski 1 week, 4 days ago 
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
Tested-by: Chad Jablonski <chad@jablonski.xyz>

Hey Balaton!

This fix looks correct to me. Source and destination should use
their respective bits. I applied my latest HOST_DATA changes on
top of this and saw no problems when rendering text in xterm
with rage 128.

There is one thing I _did_ notice. I implemented similar behavior with the
GMC_SRC_CLIPPING and GMC_DST_CLIPPING fields (default and leave alone).
But I implemented them as a copy of the default values during the write
when the bits are set to default. Both approaches seem to work. If you want
to keep them consistent I can test the actual hardware to see which approach
it uses or I can just update my patch to match this style.
Re: [PATCH] ati-vga: Separate default control bit for source
Posted by BALATON Zoltan 1 week, 4 days ago 
On Sun, 2 Nov 2025, Chad Jablonski wrote:
> Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
> Tested-by: Chad Jablonski <chad@jablonski.xyz>
>
> Hey Balaton!
>
> This fix looks correct to me. Source and destination should use
> their respective bits. I applied my latest HOST_DATA changes on
> top of this and saw no problems when rendering text in xterm
> with rage 128.
>
> There is one thing I _did_ notice. I implemented similar behavior with the
> GMC_SRC_CLIPPING and GMC_DST_CLIPPING fields (default and leave alone).
> But I implemented them as a copy of the default values during the write
> when the bits are set to default. Both approaches seem to work. If you want
> to keep them consistent I can test the actual hardware to see which approach
> it uses or I can just update my patch to match this style.

Matching hardware is more important than matching style, after all this is 
supposed to emulate hardware and we already found places where the docs 
were wrong. So if you can test and match what hardware does that would be 
best.

Regards,
BALATON Zoltan
Re: [PATCH] ati-vga: Separate default control bit for source
Posted by Chad Jablonski 1 week, 3 days ago 
>
> Matching hardware is more important than matching style, after all this is
> supposed to emulate hardware and we already found places where the docs
> were wrong. So if you can test and match what hardware does that would be
> best.
>

I just tested this using the same environment I tested the clipping
fields with and it looks like offset and pitch also latch to default
values:

Test SRC pitch and offset
====================================

** Initializing DEFAULT_OFFSET to 0x0 **
** Initializing DEFAULT_PITCH to 0x0 **
** Initializing SRC_OFFSET to 0x0 **
** Initializing SRC_PITCH to 0x0 **

Initial State
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x00000000
DEFAULT_PITCH:           0x00000000
SRC_OFFSET:              0x00000000
SRC_PITCH:               0x00000000

** Setting DEFAULT_OFFSET to 0x000000aa **
** Setting DEFAULT_PITCH to 0x000000bb **
** Setting SRC_OFFSET to 0x11 **
** Setting SRC_PITCH to 0x22 **

State After Setting
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x000000a0  <======= 4:0 are hardwired to 0
DEFAULT_PITCH:           0x000000bb           just like the docs say
SRC_OFFSET:              0x00000010  <======= 4:0 are hardwired to 0
SRC_PITCH:               0x00000022           just like the docs say

** Setting GMC_SRC_PITCH_OFFSET_CNTL to default **

State After Setting Default
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00c
DEFAULT_OFFSET:          0x000000a0
DEFAULT_PITCH:           0x000000bb
SRC_OFFSET:              0x000000a0  <======= Set to default
SRC_PITCH:               0x000000bb  <======= Set to default

** Setting GMC_SRC_PITCH_OFFSET_CNTL to leave alone **

State After Setting Leave Alone
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x000000a0
DEFAULT_PITCH:           0x000000bb
SRC_OFFSET:              0x000000a0  <======= STILL default
SRC_PITCH:               0x000000bb  <======= STILL default

Test DST pitch and offset
====================================

** Initializing DEFAULT_OFFSET to 0x0 **
** Initializing DEFAULT_PITCH to 0x0 **
** Initializing DST_OFFSET to 0x0 **
** Initializing DST_PITCH to 0x0 **

Initial State
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x00000000
DEFAULT_PITCH:           0x00000000
DST_OFFSET:              0x00000000
DST_PITCH:               0x00000000

** Setting DEFAULT_OFFSET to 0x000000aa **
** Setting DEFAULT_PITCH to 0x000000bb **
** Setting DST_OFFSET to 0x11 **
** Setting DST_PITCH to 0x22 **

State After Setting
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x000000a0  <======= 4:0 are hardwired to 0
DEFAULT_PITCH:           0x000000bb           just like the docs say
DST_OFFSET:              0x00000010  <======= 4:0 are hardwired to 0
DST_PITCH:               0x00000022           just like the docs say

** Setting GMC_DST_PITCH_OFFSET_CNTL to default **

State After Setting Default
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00c
DEFAULT_OFFSET:          0x000000a0
DEFAULT_PITCH:           0x000000bb
DST_OFFSET:              0x000000a0  <======= Set to default
DST_PITCH:               0x000000bb  <======= Set to default

** Setting GMC_DST_PITCH_OFFSET_CNTL to leave alone **

State After Setting Leave Alone
------------------------------------
DP_GUI_MASTER_CNTL:      0x30f0e00d
DEFAULT_OFFSET:          0x000000a0
DEFAULT_PITCH:           0x000000bb
DST_OFFSET:              0x000000a0  <======= STILL default
DST_PITCH:               0x000000bb  <======= STILL default
Re: [PATCH] ati-vga: Separate default control bit for source
Posted by BALATON Zoltan 3 weeks, 1 day ago 
On Tue, 7 Oct 2025, BALATON Zoltan wrote:
> The DP_GUI_MASTER_CNTL register has separate bits for src and dest but
> we were only looking at the dest bit. Use the correct bit for source.

Ping?

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/ati_2d.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 309bb5ccb6..e69b15b570 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -43,7 +43,8 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
>     }
> }
>
> -#define DEFAULT_CNTL (s->regs.dp_gui_master_cntl & GMC_DST_PITCH_OFFSET_CNTL)
> +#define DFLT_CNTL_SRC (s->regs.dp_gui_master_cntl & GMC_SRC_PITCH_OFFSET_CNTL)
> +#define DFLT_CNTL_DST (s->regs.dp_gui_master_cntl & GMC_DST_PITCH_OFFSET_CNTL)
>
> void ati_2d_blt(ATIVGAState *s)
> {
> @@ -63,12 +64,12 @@ void ati_2d_blt(ATIVGAState *s)
>         qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
>         return;
>     }
> -    int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : s->regs.default_pitch;
> +    int dst_stride = DFLT_CNTL_DST ? s->regs.dst_pitch : s->regs.default_pitch;
>     if (!dst_stride) {
>         qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
>         return;
>     }
> -    uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
> +    uint8_t *dst_bits = s->vga.vram_ptr + (DFLT_CNTL_DST ?
>                         s->regs.dst_offset : s->regs.default_offset);
>
>     if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> @@ -97,13 +98,13 @@ void ati_2d_blt(ATIVGAState *s)
>                        s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
>         unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
>                        s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
> -        int src_stride = DEFAULT_CNTL ?
> +        int src_stride = DFLT_CNTL_SRC ?
>                          s->regs.src_pitch : s->regs.default_pitch;
>         if (!src_stride) {
>             qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
>             return;
>         }
> -        uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
> +        uint8_t *src_bits = s->vga.vram_ptr + (DFLT_CNTL_SRC ?
>                             s->regs.src_offset : s->regs.default_offset);
>
>         if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
>
Re: [PATCH] ati-vga: Separate default control bit for source
Posted by BALATON Zoltan 2 weeks, 2 days ago 
On Thu, 23 Oct 2025, BALATON Zoltan wrote:
> On Tue, 7 Oct 2025, BALATON Zoltan wrote:
>> The DP_GUI_MASTER_CNTL register has separate bits for src and dest but
>> we were only looking at the dest bit. Use the correct bit for source.
>
> Ping?

Ping^2
Is there anybody sending a pull request with this and other ati-vga patch 
before the freeze?

>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/display/ati_2d.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>> index 309bb5ccb6..e69b15b570 100644
>> --- a/hw/display/ati_2d.c
>> +++ b/hw/display/ati_2d.c
>> @@ -43,7 +43,8 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
>>     }
>> }
>> 
>> -#define DEFAULT_CNTL (s->regs.dp_gui_master_cntl & 
>> GMC_DST_PITCH_OFFSET_CNTL)
>> +#define DFLT_CNTL_SRC (s->regs.dp_gui_master_cntl & 
>> GMC_SRC_PITCH_OFFSET_CNTL)
>> +#define DFLT_CNTL_DST (s->regs.dp_gui_master_cntl & 
>> GMC_DST_PITCH_OFFSET_CNTL)
>> 
>> void ati_2d_blt(ATIVGAState *s)
>> {
>> @@ -63,12 +64,12 @@ void ati_2d_blt(ATIVGAState *s)
>>         qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
>>         return;
>>     }
>> -    int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : 
>> s->regs.default_pitch;
>> +    int dst_stride = DFLT_CNTL_DST ? s->regs.dst_pitch : 
>> s->regs.default_pitch;
>>     if (!dst_stride) {
>>         qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
>>         return;
>>     }
>> -    uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
>> +    uint8_t *dst_bits = s->vga.vram_ptr + (DFLT_CNTL_DST ?
>>                         s->regs.dst_offset : s->regs.default_offset);
>>
>>     if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
>> @@ -97,13 +98,13 @@ void ati_2d_blt(ATIVGAState *s)
>>                        s->regs.src_x : s->regs.src_x + 1 - 
>> s->regs.dst_width);
>>         unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
>>                        s->regs.src_y : s->regs.src_y + 1 - 
>> s->regs.dst_height);
>> -        int src_stride = DEFAULT_CNTL ?
>> +        int src_stride = DFLT_CNTL_SRC ?
>>                          s->regs.src_pitch : s->regs.default_pitch;
>>         if (!src_stride) {
>>             qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
>>             return;
>>         }
>> -        uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
>> +        uint8_t *src_bits = s->vga.vram_ptr + (DFLT_CNTL_SRC ?
>>                             s->regs.src_offset : s->regs.default_offset);
>>
>>         if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
>> 
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.