Hi Michael,
> Subject: Re: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture
> with linear memory layout
>
> Hi all,
>
> I just noticed that Dmitry Osipenko had already pointed out a similar issue
> earlier-so my message was somewhat redundant. Apologies for the
> duplication.
Yeah, looks like you and Dmitry identified the leak independently, almost at the
same time.
>
> Also, I made a small mistake in the patch I proposed:
> The call to glDeleteMemoryObjectsEXT(1, &mem_obj); should be placed
> above the #endif, not after it. Sorry about that oversight!
Your patch from Open Source VDI repo seems like a better solution, so, I'll add
it
to this series and send it out for review in v5. Please add description/commit
message
and your signed-off-by line to it.
Thanks,
Vivek
>
> Best regards,
> Michael
>
>
> On 26.05.25 15:06, Michael Scherle wrote:
> > Great to see this patch making progress.
> >
> > I've tested it extensively, and unfortunately, I've noticed a memory
> > leak in surface_gl_create_texture_from_fd(). The memory leak is hard
> > to see since the memory is owned by the gpu driver.
> > On Intel hardware, it's possible to observe the leak using:
> >
> > cat /sys/module/i915/refcnt
> > or
> > xpu-smi ps
> >
> > In on of my use case-which involves frequent scanout disable/enable
> > cycles-the leak is quite apparent. However, in more typical scenarios,
> > it might be difficult to catch.
> >
> > The issue stems from the mem_obj not being deleted after use. I've put
> > together a minimal modification to address it:
> >
> >
> >
> > On 15.05.25 04:45, Vivek Kasireddy wrote:
> >> There are cases where we do not want the memory layout of a texture
> >> to be tiled as the component processing the texture would not know
> >> how to de-tile either via software or hardware. Therefore, ensuring
> >> that the memory backing the texture has a linear layout is absolutely
> >> necessary in these situations.
> >>
> >> Cc: Gerd Hoffmann <kra...@redhat.com>
> >> Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> >> Cc: Dmitry Osipenko <dmitry.osipe...@collabora.com>
> >> Cc: Frediano Ziglio <fredd...@gmail.com>
> >> Cc: Dongwon Kim <dongwon....@intel.com>
> >> Signed-off-by: Vivek Kasireddy <vivek.kasire...@intel.com>
> >> ---
> >> include/ui/console.h | 2 ++
> >> ui/console-gl.c | 32 ++++++++++++++++++++++++++++++++
> >> 2 files changed, 34 insertions(+)
> >>
> >> diff --git a/include/ui/console.h b/include/ui/console.h index
> >> 46b3128185..5cfa6ae215 100644
> >> --- a/include/ui/console.h
> >> +++ b/include/ui/console.h
> >> @@ -422,6 +422,8 @@ bool
> >> console_gl_check_format(DisplayChangeListener
> >> *dcl,
> >> pixman_format_code_t format);
> >> void surface_gl_create_texture(QemuGLShader *gls,
> >> DisplaySurface *surface);
> >> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> >> + int fd, GLuint *texture);
> >> void surface_gl_update_texture(QemuGLShader *gls,
> >> DisplaySurface *surface,
> >> int x, int y, int w, int h); diff
> >> --git a/ui/console-gl.c b/ui/console-gl.c index
> >> 103b954017..97f7989651 100644
> >> --- a/ui/console-gl.c
> >> +++ b/ui/console-gl.c
> >> @@ -25,6 +25,7 @@
> >> * THE SOFTWARE.
> >> */
> >> #include "qemu/osdep.h"
> >> +#include "qemu/error-report.h"
> >> #include "ui/console.h"
> >> #include "ui/shader.h"
> >> @@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
> >> glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> >> GL_LINEAR);
> >> }
> >> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> >> + int fd, GLuint *texture) {
> >> + unsigned long size = surface_stride(surface) *
> >> surface_height(surface);
> >> + GLenum err = glGetError();
> >> + GLuint mem_obj;
> >
> > + GLuint mem_obj = 0;
> >
> >> +
> >> + if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
> >> + !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
> >> + return false;
> >> + }
> >> +
> >> +#ifdef GL_EXT_memory_object_fd
> >> + glCreateMemoryObjectsEXT(1, &mem_obj);
> >> + glImportMemoryFdEXT(mem_obj, size,
> GL_HANDLE_TYPE_OPAQUE_FD_EXT,
> >> fd);
> >> +
> >> + err = glGetError();
> >> + if (err != GL_NO_ERROR) {
> >
> > + if (mem_obj) {
> > + glDeleteMemoryObjectsEXT(1, &mem_obj);
> > + }
> >
> >> + error_report("spice: cannot import memory object from fd");
> >> + return false;
> >> + }
> >> +
> >> + glGenTextures(1, texture);
> >> + glBindTexture(GL_TEXTURE_2D, *texture);
> >> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT,
> >> GL_LINEAR_TILING_EXT);
> >> + glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8,
> >> surface_width(surface),
> >> + surface_height(surface), mem_obj, 0);
> >> +#endif
> >
> > + glDeleteMemoryObjectsEXT(1, &mem_obj);
> >
> >> + return *texture > 0 && glGetError() == GL_NO_ERROR; }
> >> +
> >> void surface_gl_update_texture(QemuGLShader *gls,
> >> DisplaySurface *surface,
> >> int x, int y, int w, int h)
> >
> >
> >
> > That said, my OpenGL knowledge is somewhat limited, and the
> > documentation wasn't entirely clear to me on whether deleting the
> > memory object while the texture is still being used, is always safe.
> > Based on a quick look at the iris and llvmpipe implementations, it
> > appears to be acceptable.
> >
> > If that's not the case, an alternative fix could follow this approach
> > instead: https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/
> > commit/4ca806871c141089be16af25c1820d3e04f3e27d
> >
> > Greetings Michael
