1. Patchew
  2. QEMU
  3. [PULL 00/49] Misc HW patches for 2026-03-08
  4. View patch

[PULL 22/49] ati-vga: Add scissor clipping register support

Philippe Mathieu-Daudé posted 49 patches 1 month ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Christian Schoenebeck <qemu_oss@crudebyte.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Cédric Le Goater" <clg@kaod.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Jamin Lin <jamin_lin@aspeedtech.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>, Thomas Huth <th.huth+qemu@posteo.eu>, Jason Wang <jasowang@redhat.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Ilya Leoshkevich <iii@linux.ibm.com>, David Hildenbrand <david@kernel.org>, Farhan Ali <alifm@linux.ibm.com>, Laurent Vivier <laurent@vivier.eu>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Marcelo Tosatti <mtosatti@redhat.com>, Fabiano Rosas <farosas@suse.de>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
[PULL 22/49] ati-vga: Add scissor clipping register support
Posted by Philippe Mathieu-Daudé 1 month ago 
From: Chad Jablonski <chad@jablonski.xyz>

Implement read and write operations on SC_TOP_LEFT, SC_BOTTOM_RIGHT,
and SRC_SC_BOTTOM_RIGHT registers. These registers are also updated
when the src and/or dst clipping fields on DP_GUI_MASTER_CNTL are set
to default clipping.

Scissor clipping is used when rendering text in X.org. The r128 driver
sends host data much wider than is necessary to draw a glyph and cuts it
down to size using clipping before rendering. The actual clipping
implementation follows in a future patch.

This also includes a very minor refactor of the combined
default_sc_bottom_right field in the registers struct to
default_sc_bottom and default_sc_right. This was done to
stay consistent with the other scissor registers and prevent repeated
masking and extraction.

Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-ID: <20260303024730.1489136-7-chad@jablonski.xyz>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/ati_int.h  |  9 +++++-
 hw/display/ati_regs.h |  2 ++
 hw/display/ati.c      | 70 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 708cc1dd3a3..98f57ca5fa4 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -78,14 +78,21 @@ typedef struct ATIVGARegs {
     uint32_t dp_brush_frgd_clr;
     uint32_t dp_src_frgd_clr;
     uint32_t dp_src_bkgd_clr;
+    uint16_t sc_top;
+    uint16_t sc_left;
+    uint16_t sc_bottom;
+    uint16_t sc_right;
+    uint16_t src_sc_bottom;
+    uint16_t src_sc_right;
     uint32_t dp_cntl;
     uint32_t dp_datatype;
     uint32_t dp_mix;
     uint32_t dp_write_mask;
     uint32_t default_offset;
     uint32_t default_pitch;
+    uint16_t default_sc_bottom;
+    uint16_t default_sc_right;
     uint32_t default_tile;
-    uint32_t default_sc_bottom_right;
 } ATIVGARegs;
 
 struct ATIVGAState {
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 0a0825db048..3999edb9b71 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -397,6 +397,8 @@
 #define GMC_DST_PITCH_OFFSET_CNTL               0x00000002
 #define GMC_SRC_CLIP_DEFAULT                    0x00000000
 #define GMC_DST_CLIP_DEFAULT                    0x00000000
+#define GMC_SRC_CLIPPING                        0x00000004
+#define GMC_DST_CLIPPING                        0x00000008
 #define GMC_BRUSH_SOLIDCOLOR                    0x000000d0
 #define GMC_SRC_DSTCOLOR                        0x00003000
 #define GMC_BYTE_ORDER_MSB_TO_LSB               0x00000000
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 26fc74b19b3..6cf243bcf95 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -514,7 +514,32 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
         val |= s->regs.default_tile << 16;
         break;
     case DEFAULT_SC_BOTTOM_RIGHT:
-        val = s->regs.default_sc_bottom_right;
+        val = (s->regs.default_sc_bottom << 16) |
+              s->regs.default_sc_right;
+        break;
+    case SC_TOP:
+        val = s->regs.sc_top;
+        break;
+    case SC_LEFT:
+        val = s->regs.sc_left;
+        break;
+    case SC_BOTTOM:
+        val = s->regs.sc_bottom;
+        break;
+    case SC_RIGHT:
+        val = s->regs.sc_right;
+        break;
+    case SRC_SC_BOTTOM:
+        val = s->regs.src_sc_bottom;
+        break;
+    case SRC_SC_RIGHT:
+        val = s->regs.src_sc_right;
+        break;
+    case SC_TOP_LEFT:
+    case SC_BOTTOM_RIGHT:
+    case SRC_SC_BOTTOM_RIGHT:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Read from write-only register 0x%x\n", (unsigned)addr);
         break;
     default:
         break;
@@ -877,6 +902,16 @@ static void ati_mm_write(void *opaque, hwaddr addr,
             s->regs.dst_offset = s->regs.default_offset;
             s->regs.dst_pitch = s->regs.default_pitch;
         }
+        if (!(data & GMC_SRC_CLIPPING)) {
+            s->regs.src_sc_right = s->regs.default_sc_right;
+            s->regs.src_sc_bottom = s->regs.default_sc_bottom;
+        }
+        if (!(data & GMC_DST_CLIPPING)) {
+            s->regs.sc_top = 0;
+            s->regs.sc_left = 0;
+            s->regs.sc_right = s->regs.default_sc_right;
+            s->regs.sc_bottom = s->regs.default_sc_bottom;
+        }
         break;
     case DST_WIDTH_X:
         s->regs.dst_x = data & 0x3fff;
@@ -956,7 +991,38 @@ static void ati_mm_write(void *opaque, hwaddr addr,
         }
         break;
     case DEFAULT_SC_BOTTOM_RIGHT:
-        s->regs.default_sc_bottom_right = data & 0x3fff3fff;
+        s->regs.default_sc_right = data & 0x3fff;
+        s->regs.default_sc_bottom = (data >> 16) & 0x3fff;
+        break;
+    case SC_TOP_LEFT:
+        s->regs.sc_left = data & 0x3fff;
+        s->regs.sc_top = (data >> 16) & 0x3fff;
+        break;
+    case SC_LEFT:
+        s->regs.sc_left = data & 0x3fff;
+        break;
+    case SC_TOP:
+        s->regs.sc_top = data & 0x3fff;
+        break;
+    case SC_BOTTOM_RIGHT:
+        s->regs.sc_right = data & 0x3fff;
+        s->regs.sc_bottom = (data >> 16) & 0x3fff;
+        break;
+    case SC_RIGHT:
+        s->regs.sc_right = data & 0x3fff;
+        break;
+    case SC_BOTTOM:
+        s->regs.sc_bottom = data & 0x3fff;
+        break;
+    case SRC_SC_BOTTOM_RIGHT:
+        s->regs.src_sc_right = data & 0x3fff;
+        s->regs.src_sc_bottom = (data >> 16) & 0x3fff;
+        break;
+    case SRC_SC_RIGHT:
+        s->regs.src_sc_right = data & 0x3fff;
+        break;
+    case SRC_SC_BOTTOM:
+        s->regs.src_sc_bottom = data & 0x3fff;
         break;
     default:
         break;
-- 
2.53.0
Re: [PULL 22/49] ati-vga: Add scissor clipping register support
Posted by Philippe Mathieu-Daudé 1 month ago 
Hi,

On 8/3/26 23:34, Philippe Mathieu-Daudé wrote:
> From: Chad Jablonski <chad@jablonski.xyz>
> 
> Implement read and write operations on SC_TOP_LEFT, SC_BOTTOM_RIGHT,
> and SRC_SC_BOTTOM_RIGHT registers. These registers are also updated
> when the src and/or dst clipping fields on DP_GUI_MASTER_CNTL are set
> to default clipping.
> 
> Scissor clipping is used when rendering text in X.org. The r128 driver
> sends host data much wider than is necessary to draw a glyph and cuts it
> down to size using clipping before rendering. The actual clipping
> implementation follows in a future patch.
> 
> This also includes a very minor refactor of the combined
> default_sc_bottom_right field in the registers struct to
> default_sc_bottom and default_sc_right. This was done to
> stay consistent with the other scissor registers and prevent repeated
> masking and extraction.
> 
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> Message-ID: <20260303024730.1489136-7-chad@jablonski.xyz>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/display/ati_int.h  |  9 +++++-
>   hw/display/ati_regs.h |  2 ++
>   hw/display/ati.c      | 70 +++++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 78 insertions(+), 3 deletions(-)


> diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
> index 0a0825db048..3999edb9b71 100644
> --- a/hw/display/ati_regs.h
> +++ b/hw/display/ati_regs.h
> @@ -397,6 +397,8 @@
>   #define GMC_DST_PITCH_OFFSET_CNTL               0x00000002
>   #define GMC_SRC_CLIP_DEFAULT                    0x00000000
>   #define GMC_DST_CLIP_DEFAULT                    0x00000000
> +#define GMC_SRC_CLIPPING                        0x00000004
> +#define GMC_DST_CLIPPING                        0x00000008
>   #define GMC_BRUSH_SOLIDCOLOR                    0x000000d0
>   #define GMC_SRC_DSTCOLOR                        0x00003000
>   #define GMC_BYTE_ORDER_MSB_TO_LSB               0x00000000
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 26fc74b19b3..6cf243bcf95 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -514,7 +514,32 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>           val |= s->regs.default_tile << 16;
>           break;
>       case DEFAULT_SC_BOTTOM_RIGHT:
> -        val = s->regs.default_sc_bottom_right;
> +        val = (s->regs.default_sc_bottom << 16) |
> +              s->regs.default_sc_right;

Coverity is reporting an integer handling issues (SIGN_EXTENSION,
CID 1645615):

 >>>     CID 1645615:         Integer handling issues  (SIGN_EXTENSION)
 >>>     Suspicious implicit sign extension: "s->regs.default_sc_bottom" 
with type "uint16_t" (16 bits, unsigned) is promoted in 
"(s->regs.default_sc_bottom << 16) | s->regs.default_sc_right" to type 
"int" (32 bits, signed), then sign-extended to type "unsigned long" (64 
bits, unsigned).  If "(s->regs.default_sc_bottom << 16) | 
s->regs.default_sc_right" is greater than 0x7FFFFFFF, the upper bits of 
the result will all be 1.

Do you mind posting a fix?

Thanks,

Phil.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.