lineage-rockchip/android_external_minigbm
1
0
Fork 0
Code Activity Actions

virtgpu_cross_domain: Hold a lock for all of bo_create

Browse source 
In crosvm, the MinigbmDevice implementation of the Gralloc interface
assumes that a call to get_image_memory_requirements will be immediately
followed by a matching call to allocate_memory, as the implementation
stashes a buffer allocated by the first call to be returned by the
second.

However on Android multiple processes/threads can be making calls to
allocate memory via CrosGralloc. In particular if the device is
configured to use the cross-domain back-end, and two threads are trying
to allocate memory, the two requests to get the memory requirements for
an allocation can be submitted to crosvm followed by the two requests to
actually allocate memory.

When the assumption is violated, the crosvm code was raising an error
for the SECOND memory requirement request. However the error does not
propagate back to the virtgpu_cross_domain code here. The result is that
the request is understood to have succeeded, and the code here then
reads the size metadata from the response buffer, but that contains the
values from the FIRST memory requirement request. Those values may be

There is a fix for crosvm in review (https://crrev.com/c/6260975) to
not raise an error if the calls are not made in the expected order,
while still maintaining a single stashed buffer. However this means the
memory allocated for one of the two requests must be released, and would
have to be reallocated, at some extra runtime cost as allocating
graphics memory requires allocating physically contiguous memory.

The existing cross-domain code here acquired a simple mutex lock to
maintain a metadata cache, which was held for the duration of the first
call. This change extends the duration of the lock to the entire
allocation request so that the guest always makes the two requests in
the expected order.

BUG=b:395748805,b:355060470
TEST=ARCVM on Corsola starts up under high CPU stress

Change-Id: I6429ea28141ef440345a3eb442066173b3e04802
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/6288246
Commit-Queue: Lloyd Pique <lpique@google.com>
Reviewed-by: Yiwei Zhang <zzyiwei@chromium.org>
Tested-by: Lloyd Pique <lpique@google.com>
This commit is contained in:
Lloyd Pique 2025-02-20 11:42:42 -08:00 • committed by Chromeos LUCI
parent e37e16203f
commit aec6536741
1 changed files with 31 additions and 12 deletions
Download patch file Download diff file Expand all files Collapse all files

43
virtgpu_cross_domain.c
View file

@ -35,7 +35,7 @@ struct cross_domain_private {
uint32_t ring_handle;
void *ring_addr;
struct drv_array *metadata_cache;
pthread_mutex_t metadata_cache_lock;
pthread_mutex_t bo_create_lock;
bool mt8183_camera_quirk_;
};
@ -61,7 +61,7 @@ static void cross_domain_release_private(struct driver *drv)
if (priv->metadata_cache)
drv_array_destroy(priv->metadata_cache);
pthread_mutex_destroy(&priv->metadata_cache_lock);
pthread_mutex_destroy(&priv->bo_create_lock);
free(priv);
}
@ -155,14 +155,13 @@ static int cross_domain_metadata_query(struct driver *drv, struct bo_metadata *m
uint32_t plane;
memset(&cmd_get_reqs, 0, sizeof(cmd_get_reqs));
pthread_mutex_lock(&priv->metadata_cache_lock);
for (uint32_t i = 0; i < drv_array_size(priv->metadata_cache); i++) {
cached_data = (struct bo_metadata *)drv_array_at_idx(priv->metadata_cache, i);
if (!metadata_equal(metadata, cached_data))
continue;
memcpy(metadata, cached_data, sizeof(*cached_data));
goto out_unlock;
return 0;
}
cmd_get_reqs.hdr.cmd = CROSS_DOMAIN_CMD_GET_IMAGE_REQUIREMENTS;
@ -192,7 +191,7 @@ static int cross_domain_metadata_query(struct driver *drv, struct bo_metadata *m
ret = cross_domain_submit_cmd(drv, (uint32_t *)&cmd_get_reqs, cmd_get_reqs.hdr.cmd_size,
true);
if (ret < 0)
goto out_unlock;
return ret;
memcpy(&metadata->strides, &addr[0], 4 * sizeof(uint32_t));
memcpy(&metadata->offsets, &addr[4], 4 * sizeof(uint32_t));
@ -211,10 +210,7 @@ static int cross_domain_metadata_query(struct driver *drv, struct bo_metadata *m
metadata->sizes[plane - 1] = metadata->total_size - metadata->offsets[plane - 1];
drv_array_append(priv->metadata_cache, metadata);
out_unlock:
pthread_mutex_unlock(&priv->metadata_cache_lock);
return ret;
return 0;
}
/* Fill out metadata for guest buffers, used only for CPU access: */
@ -264,7 +260,7 @@ static int cross_domain_init(struct driver *drv)
if (!priv)
return -ENOMEM;
ret = pthread_mutex_init(&priv->metadata_cache_lock, NULL);
ret = pthread_mutex_init(&priv->bo_create_lock, NULL);
if (ret) {
free(priv);
return ret;
@ -367,8 +363,8 @@ static void cross_domain_close(struct driver *drv)
cross_domain_release_private(drv);
}
static int cross_domain_bo_create(struct bo *bo, uint32_t width, uint32_t height, uint32_t format,
uint64_t use_flags)
static int cross_domain_bo_create_locked(struct bo *bo, uint32_t width, uint32_t height,
uint32_t format, uint64_t use_flags)
{
int ret;
uint32_t blob_flags = VIRTGPU_BLOB_FLAG_USE_SHAREABLE;
@ -418,6 +414,29 @@ static int cross_domain_bo_create(struct bo *bo, uint32_t width, uint32_t height
return 0;
}
static int cross_domain_bo_create(struct bo *bo, uint32_t width, uint32_t height, uint32_t format,
uint64_t use_flags)
{
int ret = 0;
struct cross_domain_private *priv = bo->drv->priv;
// HACK(b/395748805): Any host GET_IMAGE_REQUIREMENTS request must be immediately followed
// by the matching CREATE_BLOB request, as the current implementation in crosvm stashes a
// single buffer allocation for the first to be returned by the second. We ensure the two
// requests are made back to back by using a mutex lock, where the lock is acquired for the
// duration of the allocation requests.
//
// This forces all guest allocations to be made in serial order, and allows the host buffer
// stash to be an optimization.
pthread_mutex_lock(&priv->bo_create_lock);
ret = cross_domain_bo_create_locked(bo, width, height, format, use_flags);
pthread_mutex_unlock(&priv->bo_create_lock);
return ret;
}
static void *cross_domain_bo_map(struct bo *bo, struct vma *vma, uint32_t map_flags)
{
int ret;