On Mon, 14 Oct 2024 at 14:41, <[email protected]> wrote:
>
> From: Marc-André Lureau <[email protected]>
>
> Use a common shareable type for win32 & unix, and helper functions.
> This simplify the code as it avoids a lot of #ifdef'ery.
>
> Note: if it helps review, commits could be reordered to introduce the
> common type before introducing shareable memory for unix.
>
> Suggested-by: Akihiko Odaki <[email protected]>
> Signed-off-by: Marc-André Lureau <[email protected]>
> Reviewed-by: Akihiko Odaki <[email protected]>
> Message-ID: <[email protected]>
Hi; this is a rather old commit (now 1ff788db9781615b in git),
but I have just noticed that it seems to break the usage of
qemu_create_displaysurface_from() by hw/display/xlnx_dp.c.
> @@ -517,14 +482,25 @@ 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);
Before this refactoring, qemu_create_displaysurface_from() always
calls pixman_image_create_bits(). That pixman function allows
you to pass NULL to mean "allocate me the memory for the data".
If you do that, pixman will always compute the stride for the
data (by looking at the pixel format and the width), so the
input linesize here is ignored. The xlnx_dp.c code takes
advantage of this in calls like:
s->g_plane.surface
= qemu_create_displaysurface_from(width, height,
s->g_plane.format, 0, NULL);
> - assert(surface->image != NULL);
> + surface->share_handle = SHAREABLE_NONE;
>
> + if (data) {
> + surface->image = pixman_image_create_bits(format,
> + width, height,
> + (void *)data, linesize);
> + } else {
> + qemu_pixman_image_new_shareable(&surface->image,
> + &surface->share_handle,
> + "displaysurface",
> + format,
> + width,
> + height,
> + linesize,
> + &error_abort);
> + surface->flags = QEMU_ALLOCATED_FLAG;
> + }
In the refactored version, although non-NULL data still
results in a call to pixman_image_create_bits(), now a NULL
data pointer means we call qemu_pixman_image_new_shareable().
> +bool
> +qemu_pixman_image_new_shareable(pixman_image_t **image,
> + qemu_pixman_shareable *handle,
> + const char *name,
> + pixman_format_code_t format,
> + int width,
> + int height,
> + int rowstride_bytes,
> + Error **errp)
> +{
> + ERRP_GUARD();
> + size_t size = height * rowstride_bytes;
This function assumes that rowstride_bytes is not zero,
so the previously-working "pass 0, NULL" now results in
a size value of 0 here and an attempt to allocate 0 bytes
in qemu_pixman_shareable_alloc(), which winds up triggering
an abort in qemu_memfd_alloc().
Presumably:
(1) qemu_pixman_image_new_shareable() should calculate the
rowstride_bytes as pixman does, rather than requiring it
to be passed in
(2) qemu_memfd_alloc() ought not to abort on zero size
For a repro case:
$ cat /tmp/xlnx-dp.txt
writel 0xfd4a0194 0x00000780
writel 0xfd4a0198 0x00000438
$ ./build/arm/qemu-system-aarch64 -accel qtest -M xlnx-zcu102
-display none -chardev
file,id=repro,path=/dev/null,input-path=/tmp/xlnx-dp.txt -qtest
chardev:repro
[I 0.000000] OPENED
pulseaudio: set_sink_input_volume() failed Reason: Invalid argument
pulseaudio: set_sink_input_mute() failed Reason: Invalid argument
[R +0.121066] writel 0xfd4a0194 0x00000780
[S +0.121092] OK
[R +0.121096] writel 0xfd4a0198 0x00000438
Unexpected error in qemu_memfd_alloc() at ../../util/memfd.c:143:
qemu-system-aarch64: failed to allocate shared memory: Invalid argument
Aborted (core dumped)
thanks
-- PMM
