diff mbox series

[12/16] ui/surface: allocate shared memory on !win32

Message ID 20241003112244.3340697-13-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show
Series UI-related fixes & shareable 2d memory with -display dbus | expand

Commit Message

Marc-André Lureau Oct. 3, 2024, 11:22 a.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Use qemu_memfd_alloc() to allocate the display surface memory, which
will fallback on tmpfile/mmap() on systems without memfd, and allow to
share the display with other processes.

This is similar to how display memory is allocated on win32 since commit
09b4c198 ("console/win32: allocate shareable display surface").

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/surface.h |  8 ++++++++
 ui/console.c         | 30 ++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)

Comments

Akihiko Odaki Oct. 5, 2024, 8:59 a.m. UTC | #1 
On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Use qemu_memfd_alloc() to allocate the display surface memory, which
> will fallback on tmpfile/mmap() on systems without memfd, and allow to
> share the display with other processes.
> 
> This is similar to how display memory is allocated on win32 since commit
> 09b4c198 ("console/win32: allocate shareable display surface").
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/ui/surface.h |  8 ++++++++
>   ui/console.c         | 30 ++++++++++++++++++++++++++++--
>   2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/include/ui/surface.h b/include/ui/surface.h
> index 345b19169d..dacf12ffe2 100644
> --- a/include/ui/surface.h
> +++ b/include/ui/surface.h
> @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
>       GLenum gltype;
>       GLuint texture;
>   #endif
> +#ifndef WIN32
> +    int shmfd;
> +    uint32_t shmfd_offset;
> +#endif
>   #ifdef WIN32
>       HANDLE handle;

What about defining a new struct that contains either of shmfd or 
handle? We can then have a unified set of functions that uses the struct 
to allocate/free a shared pixman image and to set one to DisplaySurface.

>       uint32_t handle_offset;
> @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>   DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
>   DisplaySurface *qemu_create_placeholder_surface(int w, int h,
>                                                   const char *msg);
> +#ifndef WIN32
> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> +                                   int shmfd, uint32_t offset);
> +#endif
>   #ifdef WIN32
>   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>                                             HANDLE h, uint32_t offset);
> diff --git a/ui/console.c b/ui/console.c
> index fdd76c2be4..56f2462c3d 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -37,6 +37,7 @@
>   #include "trace.h"
>   #include "exec/memory.h"
>   #include "qom/object.h"
> +#include "qemu/memfd.h"
>   
>   #include "console-priv.h"
>   
> @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
>   {
>   }
>   
> +#ifndef WIN32
> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> +                                   int shmfd, uint32_t offset)
> +{
> +    assert(surface->shmfd == -1);
> +
> +    surface->shmfd = shmfd;
> +    surface->shmfd_offset = offset;
> +}
> +#endif
> +
>   #ifdef WIN32
>   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>                                             HANDLE h, uint32_t offset)
> @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>       void *bits = NULL;
>   #ifdef WIN32
>       HANDLE handle = NULL;
> +#else
> +    int shmfd = -1;
>   #endif
>   
>       trace_displaysurface_create(width, height);
>   
>   #ifdef WIN32
>       bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
> +#else
> +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
>   #endif
>   
>       surface = qemu_create_displaysurface_from(
> @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>   
>   #ifdef WIN32
>       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> -    pixman_image_set_destroy_function(surface->image,
> -                                      qemu_pixman_shared_image_destroy, handle);
> +    void *data = handle;
> +#else
> +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
> +    void *data = GINT_TO_POINTER(shmfd);
>   #endif
> +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
> +
>       return surface;
>   }
>   
> @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>       DisplaySurface *surface = g_new0(DisplaySurface, 1);
>   
>       trace_displaysurface_create_from(surface, width, height, format);
> +#ifndef WIN32
> +    surface->shmfd = -1;
> +#endif
>       surface->image = pixman_image_create_bits(format,
>                                                 width, height,
>                                                 (void *)data, linesize);
> @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
>       DisplaySurface *surface = g_new0(DisplaySurface, 1);
>   
>       trace_displaysurface_create_pixman(surface);
> +#ifndef WIN32
> +    surface->shmfd = -1;
> +#endif
>       surface->image = pixman_image_ref(image);
>   
>       return surface;
Marc-André Lureau Oct. 7, 2024, 11:47 a.m. UTC | #2 
On Sat, Oct 5, 2024 at 12:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Use qemu_memfd_alloc() to allocate the display surface memory, which
> > will fallback on tmpfile/mmap() on systems without memfd, and allow to
> > share the display with other processes.
> >
> > This is similar to how display memory is allocated on win32 since commit
> > 09b4c198 ("console/win32: allocate shareable display surface").
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/ui/surface.h |  8 ++++++++
> >   ui/console.c         | 30 ++++++++++++++++++++++++++++--
> >   2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/ui/surface.h b/include/ui/surface.h
> > index 345b19169d..dacf12ffe2 100644
> > --- a/include/ui/surface.h
> > +++ b/include/ui/surface.h
> > @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
> >       GLenum gltype;
> >       GLuint texture;
> >   #endif
> > +#ifndef WIN32
> > +    int shmfd;
> > +    uint32_t shmfd_offset;
> > +#endif
> >   #ifdef WIN32
> >       HANDLE handle;
>
> What about defining a new struct that contains either of shmfd or
> handle? We can then have a unified set of functions that uses the struct
> to allocate/free a shared pixman image and to set one to DisplaySurface.

Well, that structure is pretty much DisplaySurface. I am not sure if
it's valuable to introduce another abstraction.

>
> >       uint32_t handle_offset;
> > @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
> >   DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
> >   DisplaySurface *qemu_create_placeholder_surface(int w, int h,
> >                                                   const char *msg);
> > +#ifndef WIN32
> > +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> > +                                   int shmfd, uint32_t offset);
> > +#endif
> >   #ifdef WIN32
> >   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
> >                                             HANDLE h, uint32_t offset);
> > diff --git a/ui/console.c b/ui/console.c
> > index fdd76c2be4..56f2462c3d 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -37,6 +37,7 @@
> >   #include "trace.h"
> >   #include "exec/memory.h"
> >   #include "qom/object.h"
> > +#include "qemu/memfd.h"
> >
> >   #include "console-priv.h"
> >
> > @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
> >   {
> >   }
> >
> > +#ifndef WIN32
> > +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> > +                                   int shmfd, uint32_t offset)
> > +{
> > +    assert(surface->shmfd == -1);
> > +
> > +    surface->shmfd = shmfd;
> > +    surface->shmfd_offset = offset;
> > +}
> > +#endif
> > +
> >   #ifdef WIN32
> >   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
> >                                             HANDLE h, uint32_t offset)
> > @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >       void *bits = NULL;
> >   #ifdef WIN32
> >       HANDLE handle = NULL;
> > +#else
> > +    int shmfd = -1;
> >   #endif
> >
> >       trace_displaysurface_create(width, height);
> >
> >   #ifdef WIN32
> >       bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
> > +#else
> > +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
> >   #endif
> >
> >       surface = qemu_create_displaysurface_from(
> > @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >
> >   #ifdef WIN32
> >       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> > -    pixman_image_set_destroy_function(surface->image,
> > -                                      qemu_pixman_shared_image_destroy, handle);
> > +    void *data = handle;
> > +#else
> > +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
> > +    void *data = GINT_TO_POINTER(shmfd);
> >   #endif
> > +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
> > +
> >       return surface;
> >   }
> >
> > @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
> >       DisplaySurface *surface = g_new0(DisplaySurface, 1);
> >
> >       trace_displaysurface_create_from(surface, width, height, format);
> > +#ifndef WIN32
> > +    surface->shmfd = -1;
> > +#endif
> >       surface->image = pixman_image_create_bits(format,
> >                                                 width, height,
> >                                                 (void *)data, linesize);
> > @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
> >       DisplaySurface *surface = g_new0(DisplaySurface, 1);
> >
> >       trace_displaysurface_create_pixman(surface);
> > +#ifndef WIN32
> > +    surface->shmfd = -1;
> > +#endif
> >       surface->image = pixman_image_ref(image);
> >
> >       return surface;
>
Akihiko Odaki Oct. 7, 2024, 12:01 p.m. UTC | #3 
On 2024/10/07 20:47, Marc-André Lureau wrote:
> On Sat, Oct 5, 2024 at 12:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Use qemu_memfd_alloc() to allocate the display surface memory, which
>>> will fallback on tmpfile/mmap() on systems without memfd, and allow to
>>> share the display with other processes.
>>>
>>> This is similar to how display memory is allocated on win32 since commit
>>> 09b4c198 ("console/win32: allocate shareable display surface").
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    include/ui/surface.h |  8 ++++++++
>>>    ui/console.c         | 30 ++++++++++++++++++++++++++++--
>>>    2 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/ui/surface.h b/include/ui/surface.h
>>> index 345b19169d..dacf12ffe2 100644
>>> --- a/include/ui/surface.h
>>> +++ b/include/ui/surface.h
>>> @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
>>>        GLenum gltype;
>>>        GLuint texture;
>>>    #endif
>>> +#ifndef WIN32
>>> +    int shmfd;
>>> +    uint32_t shmfd_offset;
>>> +#endif
>>>    #ifdef WIN32
>>>        HANDLE handle;
>>
>> What about defining a new struct that contains either of shmfd or
>> handle? We can then have a unified set of functions that uses the struct
>> to allocate/free a shared pixman image and to set one to DisplaySurface.
> 
> Well, that structure is pretty much DisplaySurface. I am not sure if
> it's valuable to introduce another abstraction.

I was thinking using the abstraction for virtio-gpu to save ifdefs. It 
already has resource_set_image_destroy to abstract away the resource 
deallocation so ui/console.c can absorb it.

> 
>>
>>>        uint32_t handle_offset;
>>> @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>>>    DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
>>>    DisplaySurface *qemu_create_placeholder_surface(int w, int h,
>>>                                                    const char *msg);
>>> +#ifndef WIN32
>>> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
>>> +                                   int shmfd, uint32_t offset);
>>> +#endif
>>>    #ifdef WIN32
>>>    void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>>>                                              HANDLE h, uint32_t offset);
>>> diff --git a/ui/console.c b/ui/console.c
>>> index fdd76c2be4..56f2462c3d 100644
>>> --- a/ui/console.c
>>> +++ b/ui/console.c
>>> @@ -37,6 +37,7 @@
>>>    #include "trace.h"
>>>    #include "exec/memory.h"
>>>    #include "qom/object.h"
>>> +#include "qemu/memfd.h"
>>>
>>>    #include "console-priv.h"
>>>
>>> @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
>>>    {
>>>    }
>>>
>>> +#ifndef WIN32
>>> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
>>> +                                   int shmfd, uint32_t offset)
>>> +{
>>> +    assert(surface->shmfd == -1);
>>> +
>>> +    surface->shmfd = shmfd;
>>> +    surface->shmfd_offset = offset;
>>> +}
>>> +#endif
>>> +
>>>    #ifdef WIN32
>>>    void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>>>                                              HANDLE h, uint32_t offset)
>>> @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>>>        void *bits = NULL;
>>>    #ifdef WIN32
>>>        HANDLE handle = NULL;
>>> +#else
>>> +    int shmfd = -1;
>>>    #endif
>>>
>>>        trace_displaysurface_create(width, height);
>>>
>>>    #ifdef WIN32
>>>        bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
>>> +#else
>>> +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
>>>    #endif
>>>
>>>        surface = qemu_create_displaysurface_from(
>>> @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>>>
>>>    #ifdef WIN32
>>>        qemu_displaysurface_win32_set_handle(surface, handle, 0);
>>> -    pixman_image_set_destroy_function(surface->image,
>>> -                                      qemu_pixman_shared_image_destroy, handle);
>>> +    void *data = handle;
>>> +#else
>>> +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
>>> +    void *data = GINT_TO_POINTER(shmfd);
>>>    #endif
>>> +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
>>> +
>>>        return surface;
>>>    }
>>>
>>> @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>>>        DisplaySurface *surface = g_new0(DisplaySurface, 1);
>>>
>>>        trace_displaysurface_create_from(surface, width, height, format);
>>> +#ifndef WIN32
>>> +    surface->shmfd = -1;
>>> +#endif
>>>        surface->image = pixman_image_create_bits(format,
>>>                                                  width, height,
>>>                                                  (void *)data, linesize);
>>> @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
>>>        DisplaySurface *surface = g_new0(DisplaySurface, 1);
>>>
>>>        trace_displaysurface_create_pixman(surface);
>>> +#ifndef WIN32
>>> +    surface->shmfd = -1;
>>> +#endif
>>>        surface->image = pixman_image_ref(image);
>>>
>>>        return surface;
>>
>
diff mbox series

Patch

diff --git a/include/ui/surface.h b/include/ui/surface.h
index 345b19169d..dacf12ffe2 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -23,6 +23,10 @@  typedef struct DisplaySurface {
     GLenum gltype;
     GLuint texture;
 #endif
+#ifndef WIN32
+    int shmfd;
+    uint32_t shmfd_offset;
+#endif
 #ifdef WIN32
     HANDLE handle;
     uint32_t handle_offset;
@@ -37,6 +41,10 @@  DisplaySurface *qemu_create_displaysurface_from(int width, int height,
 DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
 DisplaySurface *qemu_create_placeholder_surface(int w, int h,
                                                 const char *msg);
+#ifndef WIN32
+void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
+                                   int shmfd, uint32_t offset);
+#endif
 #ifdef WIN32
 void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
                                           HANDLE h, uint32_t offset);
diff --git a/ui/console.c b/ui/console.c
index fdd76c2be4..56f2462c3d 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -37,6 +37,7 @@ 
 #include "trace.h"
 #include "exec/memory.h"
 #include "qom/object.h"
+#include "qemu/memfd.h"
 
 #include "console-priv.h"
 
@@ -452,6 +453,17 @@  qemu_graphic_console_init(Object *obj)
 {
 }
 
+#ifndef WIN32
+void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
+                                   int shmfd, uint32_t offset)
+{
+    assert(surface->shmfd == -1);
+
+    surface->shmfd = shmfd;
+    surface->shmfd_offset = offset;
+}
+#endif
+
 #ifdef WIN32
 void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
                                           HANDLE h, uint32_t offset)
@@ -469,12 +481,16 @@  DisplaySurface *qemu_create_displaysurface(int width, int height)
     void *bits = NULL;
 #ifdef WIN32
     HANDLE handle = NULL;
+#else
+    int shmfd = -1;
 #endif
 
     trace_displaysurface_create(width, height);
 
 #ifdef WIN32
     bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
+#else
+    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
 #endif
 
     surface = qemu_create_displaysurface_from(
@@ -486,9 +502,13 @@  DisplaySurface *qemu_create_displaysurface(int width, int height)
 
 #ifdef WIN32
     qemu_displaysurface_win32_set_handle(surface, handle, 0);
-    pixman_image_set_destroy_function(surface->image,
-                                      qemu_pixman_shared_image_destroy, handle);
+    void *data = handle;
+#else
+    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
+    void *data = GINT_TO_POINTER(shmfd);
 #endif
+    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
+
     return surface;
 }
 
@@ -499,6 +519,9 @@  DisplaySurface *qemu_create_displaysurface_from(int width, int height,
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
     trace_displaysurface_create_from(surface, width, height, format);
+#ifndef WIN32
+    surface->shmfd = -1;
+#endif
     surface->image = pixman_image_create_bits(format,
                                               width, height,
                                               (void *)data, linesize);
@@ -512,6 +535,9 @@  DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
     trace_displaysurface_create_pixman(surface);
+#ifndef WIN32
+    surface->shmfd = -1;
+#endif
     surface->image = pixman_image_ref(image);
 
     return surface;