Skip to content

Commit

Permalink
egl-wayland: Fix an unbounded array growth issue
Browse files Browse the repository at this point in the history
We do not seem to shrink the dynamically allocated streamImages array
that we use for storing the resources associated with the swapchain
images. When an entry in this array is destroyed in the
destroy_stream_image() function, its fields are simply reverted to
default/invalid values. Later on, the entries in this array are tried to
be recycled in the add_surface_image() function. However, this approach
is error-prone since we keep valid and invalid entries together, which
requires us to check the validity of the entries every time we access
them. Also, when explicit sync is in use, this array just keeps growing
over time, especially during the application window resizes, due to a
bug in the entry destruction logic.

So, to solve these problems, this change converts the dynamically
allocated streamImages array into a linked list for simplified
insertions and deletions. Each entry in this linked list is removed from
the list and deallocated once they are no longer needed. So, all of the
entries in the list stay valid.

Per-entry mutexes are replaced with a single mutex that guards accesses
to the entire list to make sure that the linked list does not get
corrupted when it is accessed from multiple threads. This only happens
when explicit sync is not in use. The sizes of the critical sections
that are protected by this new mutex are very small. To test to see if
this change creates lock contention issues, weston-simple-egl
application with swap interval of 0 was run on a Wayland compositor that
does not support explicit sync to create as much lock contention as
possible. However, no measurable difference in performance was observed
after this change was applied.

As a side effect of this change, a bug in the
wlEglSurfaceCheckReleasePoints() function, where we wrongly assumed that
all the entries in the streamImages array were valid, is fixed. This bug
caused us to pass destroyed DRM syncobjs to the drmSyncobjTimelineWait()
function, which led to random application freezes as a result in some
cases since it prevented images from being released back to the EGL
stream.

Another side effect of this change is that, it makes the maximum number
of entries in this list known when explicit sync is in use, allowing us
to avoid dynamically allocating the arrays for the list of DRM syncobjs
and timeline points in wlEglSurfaceCheckReleasePoints(). This fixes a
memory leak issue that can happen if only one of these allocations
fails.
  • Loading branch information
dkorkmazturk committed Jul 26, 2024
1 parent c9e0c51 commit 45e374e
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 185 deletions.
30 changes: 16 additions & 14 deletions include/wayland-eglsurface-internal.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014-2022, NVIDIA CORPORATION. All rights reserved.
* Copyright (c) 2014-2024, NVIDIA CORPORATION. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
Expand Down Expand Up @@ -39,14 +39,6 @@ typedef struct WlEglStreamImageRec {
/* Pointer back to the parent surface for use in Wayland callbacks */
struct WlEglSurfaceRec *surface;

/*
* Use an individual mutex to guard access to each image's data. This avoids
* sharing the surface lock between the app and buffer release event
* threads, resulting in simplified lock management and smaller critical
* sections.
*/
pthread_mutex_t mutex;

EGLImageKHR eglImage;
struct wl_buffer *buffer;
EGLBoolean attached;
Expand All @@ -59,6 +51,14 @@ typedef struct WlEglStreamImageRec {
uint64_t releasePoint;
/* Cached acquire EGLSync from acquireImage */
EGLSyncKHR acquireSync;

/*
* Used for delaying the destruction of the image if we are waiting the
* buffer release thread to use it later.
*/
EGLBoolean destructionPending;

struct wl_list link;
} WlEglStreamImage;

typedef struct WlEglSurfaceCtxRec {
Expand All @@ -78,14 +78,16 @@ typedef struct WlEglSurfaceCtxRec {
EGLuint64KHR framesProcessed;

/*
* The double pointer is because of the need to allocate the data for each
* image slot separately to avoid clobbering the acquiredLink member
* whenever the streamImages arrary is resized with realloc().
* Use an individual mutex to guard access to streamImages. This helps us
* to avoid sharing the surface lock between the app and buffer release
* event threads, resulting in simplified lock management and smaller
* critical sections.
*/
WlEglStreamImage **streamImages;
pthread_mutex_t streamImagesMutex;

struct wl_list streamImages;
struct wl_list acquiredImages;
struct wl_buffer *currentBuffer;
uint32_t numStreamImages;

struct wl_list link;
} WlEglSurfaceCtx;
Expand Down
Loading

0 comments on commit 45e374e

Please sign in to comment.