[PULL 0/4] Vga 20200615 patches

[PULL 0/4] Vga 20200615 patches

[Gerd Hoffmann]

The following changes since commit 7d3660e79830a069f1848bb4fa1cdf8f666424fb:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-06-12 23:06:22 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/vga-20200615-pull-request

for you to fetch changes up to fca0b885417497a973c56e6d4e0d88f9f7a0e71b:

  sm501: Optimise 1 pixel 2d ops (2020-06-15 10:52:53 +0200)

----------------------------------------------------------------
vga: sm501 fixes and optimizations.

----------------------------------------------------------------

BALATON Zoltan (4):
  sm501: Fix bounds checks
  sm501: Drop unneded variable
  sm501: Ignore no-op blits
  sm501: Optimise 1 pixel 2d ops

 hw/display/sm501.c | 58 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)

-- 
2.18.4

[PULL 1/4] sm501: Fix bounds checks

[Gerd Hoffmann]

From: BALATON Zoltan <balaton@eik.bme.hu>

We don't need to add width to pitch when calculating last point, that
would reject valid ops within the card's local_mem.

Fixes: b15a22bbcbe6a78dc3d88fe3134985e4cdd87de4
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-id: acb431de2d9c7a497d54a548dfc7592eb2b9fe1c.1591471056.git.balaton@eik.bme.hu
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index edd8d24a76c1..5ae320ddc325 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -723,8 +723,8 @@ static void sm501_2d_operation(SM501State *s)
         dst_y -= height - 1;
     }
 
-    if (dst_base >= get_local_mem_size(s) || dst_base +
-        (dst_x + width + (dst_y + height) * (dst_pitch + width)) *
+    if (dst_base >= get_local_mem_size(s) ||
+        dst_base + (dst_x + width + (dst_y + height) * dst_pitch) *
         (1 << format) >= get_local_mem_size(s)) {
         qemu_log_mask(LOG_GUEST_ERROR, "sm501: 2D op dest is outside vram.\n");
         return;
@@ -749,8 +749,8 @@ static void sm501_2d_operation(SM501State *s)
             src_y -= height - 1;
         }
 
-        if (src_base >= get_local_mem_size(s) || src_base +
-            (src_x + width + (src_y + height) * (src_pitch + width)) *
+        if (src_base >= get_local_mem_size(s) ||
+            src_base + (src_x + width + (src_y + height) * src_pitch) *
             (1 << format) >= get_local_mem_size(s)) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "sm501: 2D op src is outside vram.\n");
-- 
2.18.4

[PULL 2/4] sm501: Drop unneded variable

[Gerd Hoffmann]

From: BALATON Zoltan <balaton@eik.bme.hu>

We don't need a separate variable to keep track if we allocated memory
that needs to be freed as we can test the pointer itself.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-id: 7a818701887a77af17995aa062b37f92ffc3d2eb.1591471056.git.balaton@eik.bme.hu
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 5ae320ddc325..85d54b598f80 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -796,13 +796,12 @@ static void sm501_2d_operation(SM501State *s)
             de = db + width + height * (width + dst_pitch);
             if (rtl && ((db >= sb && db <= se) || (de >= sb && de <= se))) {
                 /* regions may overlap: copy via temporary */
-                int free_buf = 0, llb = width * (1 << format);
+                int llb = width * (1 << format);
                 int tmp_stride = DIV_ROUND_UP(llb, sizeof(uint32_t));
                 uint32_t *tmp = tmp_buf;
 
                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
                     tmp = g_malloc(tmp_stride * sizeof(uint32_t) * height);
-                    free_buf = 1;
                 }
                 pixman_blt((uint32_t *)&s->local_mem[src_base], tmp,
                            src_pitch * (1 << format) / sizeof(uint32_t),
@@ -813,7 +812,7 @@ static void sm501_2d_operation(SM501State *s)
                            dst_pitch * (1 << format) / sizeof(uint32_t),
                            8 * (1 << format), 8 * (1 << format),
                            0, 0, dst_x, dst_y, width, height);
-                if (free_buf) {
+                if (tmp != tmp_buf) {
                     g_free(tmp);
                 }
             } else {
-- 
2.18.4

[PULL 3/4] sm501: Ignore no-op blits

[Gerd Hoffmann]

From: BALATON Zoltan <balaton@eik.bme.hu>

Some guests seem to try source copy blits with same source and dest
which are no-op so avoid calling pixman for these.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-id: b62681e58108651b2ca0d2fdbde8281e87185dbb.1591471056.git.balaton@eik.bme.hu
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 85d54b598f80..3397ca9fbf4c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -788,6 +788,11 @@ static void sm501_2d_operation(SM501State *s)
                               (rop2_source_is_pattern ?
                                   " with pattern source" : ""));
             }
+            /* Ignore no-op blits, some guests seem to do this */
+            if (src_base == dst_base && src_pitch == dst_pitch &&
+                src_x == dst_x && src_y == dst_y) {
+                break;
+            }
             /* Check for overlaps, this could be made more exact */
             uint32_t sb, se, db, de;
             sb = src_base + src_x + src_y * (width + src_pitch);
-- 
2.18.4

[PULL 4/4] sm501: Optimise 1 pixel 2d ops

[Gerd Hoffmann]

From: BALATON Zoltan <balaton@eik.bme.hu>

Some guests do 1x1 blits which is faster to do directly than calling a
function for it so avoid overhead in this case.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-id: 9fab4fe6513fe8b921ce86a57f4ff7e0d5a3c3f9.1591471056.git.balaton@eik.bme.hu
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 3397ca9fbf4c..59693fbb5c17 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -793,6 +793,25 @@ static void sm501_2d_operation(SM501State *s)
                 src_x == dst_x && src_y == dst_y) {
                 break;
             }
+            /* Some clients also do 1 pixel blits, avoid overhead for these */
+            if (width == 1 && height == 1) {
+                unsigned int si = (src_x + src_y * src_pitch) * (1 << format);
+                unsigned int di = (dst_x + dst_y * dst_pitch) * (1 << format);
+                switch (format) {
+                case 0:
+                    s->local_mem[dst_base + di] = s->local_mem[src_base + si];
+                    break;
+                case 1:
+                    *(uint16_t *)&s->local_mem[dst_base + di] =
+                                    *(uint16_t *)&s->local_mem[src_base + si];
+                    break;
+                case 2:
+                    *(uint32_t *)&s->local_mem[dst_base + di] =
+                                    *(uint32_t *)&s->local_mem[src_base + si];
+                    break;
+                }
+                break;
+            }
             /* Check for overlaps, this could be made more exact */
             uint32_t sb, se, db, de;
             sb = src_base + src_x + src_y * (width + src_pitch);
@@ -841,9 +860,24 @@ static void sm501_2d_operation(SM501State *s)
             color = cpu_to_le16(color);
         }
 
-        pixman_fill((uint32_t *)&s->local_mem[dst_base],
-                    dst_pitch * (1 << format) / sizeof(uint32_t),
-                    8 * (1 << format), dst_x, dst_y, width, height, color);
+        if (width == 1 && height == 1) {
+            unsigned int i = (dst_x + dst_y * dst_pitch) * (1 << format);
+            switch (format) {
+            case 0:
+                s->local_mem[dst_base + i] = color & 0xff;
+                break;
+            case 1:
+                *(uint16_t *)&s->local_mem[dst_base + i] = color & 0xffff;
+                break;
+            case 2:
+                *(uint32_t *)&s->local_mem[dst_base + i] = color;
+                break;
+            }
+        } else {
+            pixman_fill((uint32_t *)&s->local_mem[dst_base],
+                        dst_pitch * (1 << format) / sizeof(uint32_t),
+                        8 * (1 << format), dst_x, dst_y, width, height, color);
+        }
         break;
     }
     default:
-- 
2.18.4

Re: [PULL 0/4] Vga 20200615 patches

[Peter Maydell]

On Mon, 15 Jun 2020 at 16:16, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit 7d3660e79830a069f1848bb4fa1cdf8f666424fb:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-06-12 23:06:22 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20200615-pull-request
>
> for you to fetch changes up to fca0b885417497a973c56e6d4e0d88f9f7a0e71b:
>
>   sm501: Optimise 1 pixel 2d ops (2020-06-15 10:52:53 +0200)
>
> ----------------------------------------------------------------
> vga: sm501 fixes and optimizations.
>
> ----------------------------------------------------------------
>
> BALATON Zoltan (4):
>   sm501: Fix bounds checks
>   sm501: Drop unneded variable
>   sm501: Ignore no-op blits
>   sm501: Optimise 1 pixel 2d ops

I just sent reviewed-by tags for patches 1-3 which you haven't
picked up; and patch 4 hasn't been reviewed at all yet :-(

-- PMM

Re: [PULL 0/4] Vga 20200615 patches

[BALATON Zoltan]

On Mon, 15 Jun 2020, Gerd Hoffmann wrote:
> The following changes since commit 7d3660e79830a069f1848bb4fa1cdf8f666424fb:
>
>  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-06-12 23:06:22 +0100)
>
> are available in the Git repository at:
>
>  git://git.kraxel.org/qemu tags/vga-20200615-pull-request
>
> for you to fetch changes up to fca0b885417497a973c56e6d4e0d88f9f7a0e71b:
>
>  sm501: Optimise 1 pixel 2d ops (2020-06-15 10:52:53 +0200)
>
> ----------------------------------------------------------------
> vga: sm501 fixes and optimizations.
>
> ----------------------------------------------------------------
>
> BALATON Zoltan (4):
>  sm501: Fix bounds checks
>  sm501: Drop unneded variable
>  sm501: Ignore no-op blits
>  sm501: Optimise 1 pixel 2d ops
>
> hw/display/sm501.c | 58 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 48 insertions(+), 10 deletions(-)

Seems you've crossed mails with Peter. He sent some review comments for 
these patches. Should I respin with those changes or you'll take this as 
it is and I should send followup patches?

Regards,
BALATON Zoltan

Re: [PULL 0/4] Vga 20200615 patches

[Peter Maydell]

On Mon, 15 Jun 2020 at 17:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Mon, 15 Jun 2020, Gerd Hoffmann wrote:
> > The following changes since commit 7d3660e79830a069f1848bb4fa1cdf8f666424fb:
> >
> >  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-06-12 23:06:22 +0100)
> >
> > are available in the Git repository at:
> >
> >  git://git.kraxel.org/qemu tags/vga-20200615-pull-request
> >
> > for you to fetch changes up to fca0b885417497a973c56e6d4e0d88f9f7a0e71b:
> >
> >  sm501: Optimise 1 pixel 2d ops (2020-06-15 10:52:53 +0200)
> >
> > ----------------------------------------------------------------
> > vga: sm501 fixes and optimizations.
> >
> > ----------------------------------------------------------------
> >
> > BALATON Zoltan (4):
> >  sm501: Fix bounds checks
> >  sm501: Drop unneded variable
> >  sm501: Ignore no-op blits
> >  sm501: Optimise 1 pixel 2d ops
> >
> > hw/display/sm501.c | 58 ++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 48 insertions(+), 10 deletions(-)
>
> Seems you've crossed mails with Peter. He sent some review comments for
> these patches. Should I respin with those changes or you'll take this as
> it is and I should send followup patches?

I've dropped the pullreq from my to-process queue; if you could respin
the patchset that's probably the best thing I think.

thanks
-- PMM