[PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[Vivek Kasireddy]

The temporary egl fb scanout_tex_fb is only needed to facilitate the
blit to the display surface's texture (ssd->ds->texture). Therefore,
destroy it after the blit is submitted. And, also make sure that it
is empty initialized before it is actually used.

Fixes: f851cd65 ("ui/spice: Blit the scanout texture if its memory layout is not linear")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
v2:
- Make scanout_tex_fb local to spice_gl_blit_scanout_texture() since
  it is not used outside of it (Peter)
---
 ui/spice-display.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 9ce622cefc..669832c561 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1183,20 +1183,20 @@ static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
     egl_dmabuf_release_texture(dmabuf);
 }
 
-static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
-                                          egl_fb *scanout_tex_fb)
+static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd)
 {
     uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
     int fds[DMABUF_MAX_PLANES], num_planes, fourcc;
+    egl_fb scanout_tex_fb = {};
     uint64_t modifier;
     bool ret;
 
-    egl_fb_destroy(scanout_tex_fb);
-    egl_fb_setup_for_tex(scanout_tex_fb,
+    egl_fb_setup_for_tex(&scanout_tex_fb,
                          surface_width(ssd->ds), surface_height(ssd->ds),
                          ssd->ds->texture, false);
-    egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
+    egl_fb_blit(&scanout_tex_fb, &ssd->guest_fb, false);
     glFlush();
+    egl_fb_destroy(&scanout_tex_fb);
 
     if (!ssd->new_scanout_texture) {
         return true;
@@ -1330,9 +1330,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     }
 
     if (spice_remote_client && ssd->blit_scanout_texture) {
-        egl_fb scanout_tex_fb;
-
-        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
+        ret = spice_gl_blit_scanout_texture(ssd);
         if (!ret) {
             return;
         }
-- 
2.49.0

Re: [PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[Marc-André Lureau]

On Sat, Jul 19, 2025 at 3:43 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> The temporary egl fb scanout_tex_fb is only needed to facilitate the
> blit to the display surface's texture (ssd->ds->texture). Therefore,
> destroy it after the blit is submitted. And, also make sure that it
> is empty initialized before it is actually used.
>
> Fixes: f851cd65 ("ui/spice: Blit the scanout texture if its memory layout is not linear")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
> v2:
> - Make scanout_tex_fb local to spice_gl_blit_scanout_texture() since
>   it is not used outside of it (Peter)
> ---
>  ui/spice-display.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9ce622cefc..669832c561 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1183,20 +1183,20 @@ static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
>      egl_dmabuf_release_texture(dmabuf);
>  }
>
> -static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
> -                                          egl_fb *scanout_tex_fb)
> +static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd)
>  {
>      uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
>      int fds[DMABUF_MAX_PLANES], num_planes, fourcc;
> +    egl_fb scanout_tex_fb = {};
>      uint64_t modifier;
>      bool ret;
>
> -    egl_fb_destroy(scanout_tex_fb);
> -    egl_fb_setup_for_tex(scanout_tex_fb,
> +    egl_fb_setup_for_tex(&scanout_tex_fb,
>                           surface_width(ssd->ds), surface_height(ssd->ds),
>                           ssd->ds->texture, false);
> -    egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
> +    egl_fb_blit(&scanout_tex_fb, &ssd->guest_fb, false);
>      glFlush();
> +    egl_fb_destroy(&scanout_tex_fb);
>
>      if (!ssd->new_scanout_texture) {
>          return true;
> @@ -1330,9 +1330,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      }
>
>      if (spice_remote_client && ssd->blit_scanout_texture) {
> -        egl_fb scanout_tex_fb;
> -
> -        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
> +        ret = spice_gl_blit_scanout_texture(ssd);
>          if (!ret) {
>              return;
>          }
> --
> 2.49.0
>
>


-- 
Marc-André Lureau

Re: [PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[Stefan Hajnoczi]

On Fri, Jul 18, 2025 at 7:42 PM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> The temporary egl fb scanout_tex_fb is only needed to facilitate the
> blit to the display surface's texture (ssd->ds->texture). Therefore,
> destroy it after the blit is submitted. And, also make sure that it
> is empty initialized before it is actually used.
>
> Fixes: f851cd65 ("ui/spice: Blit the scanout texture if its memory layout is not linear")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
> v2:
> - Make scanout_tex_fb local to spice_gl_blit_scanout_texture() since
>   it is not used outside of it (Peter)
> ---
>  ui/spice-display.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Applied for QEMU v10.1.0-rc3. Thanks!

Stefan

> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9ce622cefc..669832c561 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1183,20 +1183,20 @@ static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
>      egl_dmabuf_release_texture(dmabuf);
>  }
>
> -static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
> -                                          egl_fb *scanout_tex_fb)
> +static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd)
>  {
>      uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
>      int fds[DMABUF_MAX_PLANES], num_planes, fourcc;
> +    egl_fb scanout_tex_fb = {};
>      uint64_t modifier;
>      bool ret;
>
> -    egl_fb_destroy(scanout_tex_fb);
> -    egl_fb_setup_for_tex(scanout_tex_fb,
> +    egl_fb_setup_for_tex(&scanout_tex_fb,
>                           surface_width(ssd->ds), surface_height(ssd->ds),
>                           ssd->ds->texture, false);
> -    egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
> +    egl_fb_blit(&scanout_tex_fb, &ssd->guest_fb, false);
>      glFlush();
> +    egl_fb_destroy(&scanout_tex_fb);
>
>      if (!ssd->new_scanout_texture) {
>          return true;
> @@ -1330,9 +1330,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      }
>
>      if (spice_remote_client && ssd->blit_scanout_texture) {
> -        egl_fb scanout_tex_fb;
> -
> -        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
> +        ret = spice_gl_blit_scanout_texture(ssd);
>          if (!ret) {
>              return;
>          }
> --
> 2.49.0
>
>

[PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[Vivek Kasireddy]

The temporary egl fb scanout_tex_fb is only needed to facilitate the
blit to the display surface's texture (ssd->ds->texture). Therefore,
destroy it after the blit is submitted. And, also make sure that it
is empty initialized before it is actually used.

Fixes: f851cd65 ("ui/spice: Blit the scanout texture if its memory layout is not linear")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/spice-display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 9ce622cefc..401d6c4aba 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1191,12 +1191,12 @@ static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
     uint64_t modifier;
     bool ret;
 
-    egl_fb_destroy(scanout_tex_fb);
     egl_fb_setup_for_tex(scanout_tex_fb,
                          surface_width(ssd->ds), surface_height(ssd->ds),
                          ssd->ds->texture, false);
     egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
     glFlush();
+    egl_fb_destroy(scanout_tex_fb);
 
     if (!ssd->new_scanout_texture) {
         return true;
@@ -1330,7 +1330,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     }
 
     if (spice_remote_client && ssd->blit_scanout_texture) {
-        egl_fb scanout_tex_fb;
+        egl_fb scanout_tex_fb = {};
 
         ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
         if (!ret) {
-- 
2.49.0

Re: [PATCH] ui/spice: Destroy the temporary egl fb after the blit is submitted

[Peter Maydell]

On Fri, 18 Jul 2025 at 07:29, Vivek Kasireddy <vivek.kasireddy@intel.com> wrote:
>
> The temporary egl fb scanout_tex_fb is only needed to facilitate the
> blit to the display surface's texture (ssd->ds->texture). Therefore,
> destroy it after the blit is submitted. And, also make sure that it
> is empty initialized before it is actually used.
>
> Fixes: f851cd65 ("ui/spice: Blit the scanout texture if its memory layout is not linear")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  ui/spice-display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9ce622cefc..401d6c4aba 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1191,12 +1191,12 @@ static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
>      uint64_t modifier;
>      bool ret;
>
> -    egl_fb_destroy(scanout_tex_fb);
>      egl_fb_setup_for_tex(scanout_tex_fb,
>                           surface_width(ssd->ds), surface_height(ssd->ds),
>                           ssd->ds->texture, false);
>      egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
>      glFlush();
> +    egl_fb_destroy(scanout_tex_fb);
>
>      if (!ssd->new_scanout_texture) {
>          return true;
> @@ -1330,7 +1330,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      }
>
>      if (spice_remote_client && ssd->blit_scanout_texture) {
> -        egl_fb scanout_tex_fb;
> +        egl_fb scanout_tex_fb = {};
>
>          ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
>          if (!ret) {

It looks like we only call spice_gl_blit_scanout_texture()
once -- would it make more sense to put the scanout_tx_fb
variable inside that function rather than making its only
callsite set it up? That way the function owns, sets up,
uses and destroys the egl_fb.

thanks
-- PMM