RE: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Michael Scherle <michael.scherle@rz.uni-freiburg.de>]

Hi Vivek,

On 27.05.25 06:20, Kasireddy, Vivek wrote:
> 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! I’ve added it and made a few additional adjustments here:
https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/commit/859d500761b35cc785fcadd9d554a78712309e88

Feel free to merge it into your patches if you want.

Best regards,
Michael

> 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 <kraxel@redhat.com>
>>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> Cc: Frediano Ziglio <freddy77@gmail.com>
>>>> Cc: Dongwon Kim <dongwon.kim@intel.com>
>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@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
>