From aec6536741c08e070ae0413746d9436ef1eaa136 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Thu, 20 Feb 2025 11:42:42 -0800 Subject: [PATCH 1/2] virtgpu_cross_domain: Hold a lock for all of bo_create 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 Reviewed-by: Yiwei Zhang Tested-by: Lloyd Pique --- virtgpu_cross_domain.c | 43 ++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/virtgpu_cross_domain.c b/virtgpu_cross_domain.c index 1bba350..02e5f13 100644 --- a/virtgpu_cross_domain.c +++ b/virtgpu_cross_domain.c @@ -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; From 3dd534cca22443d6ed2c7c0e2929a0a47c2a524c Mon Sep 17 00:00:00 2001 From: Andrew Wolfers Date: Wed, 12 Mar 2025 13:30:44 +0000 Subject: [PATCH 2/2] Add padding for i915 cursor buffers This change adds behavior to pad the requested buffer size when the BO_USE_CURSOR flag is set. This change is required due to restrictions on cursor planes with the i915 driver, where a buffer must be of a particular size in order to be committed to a cursor plane. The exact requirements can be queried from the DRM_CAP_CURSOR_{WIDTH|HEIGHT} properties, which if provided, will specify a known acceptable cursor buffer size. After this change, if the BO_USE_CURSOR flag is set and the requested size is less than indicated capability, the resulting buffer will be padded according to the difference. Bug: b/378461707 , b/388014686 Change-Id: I451421cc784d1e3fdc83eb4e9762d6f0f4caea7c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/6282179 Tested-by: Andrew Wolfers Reviewed-by: Lina Versace Commit-Queue: Andrew Wolfers --- cros_gralloc/cros_gralloc_helpers.cc | 2 +- i915.c | 25 +++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/cros_gralloc/cros_gralloc_helpers.cc b/cros_gralloc/cros_gralloc_helpers.cc index 945552c..9495778 100644 --- a/cros_gralloc/cros_gralloc_helpers.cc +++ b/cros_gralloc/cros_gralloc_helpers.cc @@ -128,7 +128,7 @@ uint64_t cros_gralloc_convert_usage(uint64_t usage) handle_usage(&usage, GRALLOC_USAGE_EXTERNAL_DISP, &use_flags, BO_USE_NONE); /* Map PROTECTED to linear until real HW protection is available on Android. */ handle_usage(&usage, GRALLOC_USAGE_PROTECTED, &use_flags, BO_USE_LINEAR); - handle_usage(&usage, GRALLOC_USAGE_CURSOR, &use_flags, BO_USE_NONE); + handle_usage(&usage, GRALLOC_USAGE_CURSOR, &use_flags, BO_USE_CURSOR); /* HACK: See b/30054495 for BO_USE_SW_READ_OFTEN. */ handle_usage(&usage, GRALLOC_USAGE_HW_VIDEO_ENCODER, &use_flags, BO_USE_HW_VIDEO_ENCODER | BO_USE_SW_READ_OFTEN); diff --git a/i915.c b/i915.c index f1c4eb7..adfd8c5 100644 --- a/i915.c +++ b/i915.c @@ -564,10 +564,9 @@ static size_t i915_num_planes_from_modifier(struct driver *drv, uint32_t format, static int i915_bo_compute_metadata(struct bo *bo, uint32_t width, uint32_t height, uint32_t format, uint64_t use_flags, const uint64_t *modifiers, uint32_t count) { - uint64_t modifier; struct i915_device *i915 = bo->drv->priv; - bool huge_bo = (i915->graphics_version < 11) && (width > 4096); + uint64_t modifier; if (modifiers) { modifier = drv_pick_modifier(modifiers, count, i915->modifier.order, i915->modifier.count); @@ -578,10 +577,32 @@ static int i915_bo_compute_metadata(struct bo *bo, uint32_t width, uint32_t heig modifier = combo->metadata.modifier; } + /* + * For cursor buffer, add padding as needed to reach a known cursor-plane-supported + * buffer size, as reported by the cursor capability properties. + * + * If the requested dimensions exceed either of the reported capabilities, or if the + * capabilities couldn't be read, silently fallback by continuing without additional + * padding. The buffer can still be used normally, and be committed to non-cursor + * planes. + */ + if (use_flags & BO_USE_CURSOR) { + uint64_t cursor_width = 0; + uint64_t cursor_height = 0; + bool err = drmGetCap(bo->drv->fd, DRM_CAP_CURSOR_WIDTH, &cursor_width) || + drmGetCap(bo->drv->fd, DRM_CAP_CURSOR_HEIGHT, &cursor_height); + + if (!err && width <= cursor_width && height <= cursor_height) { + width = cursor_width; + height = cursor_height; + } + } + /* * i915 only supports linear/x-tiled above 4096 wide on Gen9/Gen10 GPU. * VAAPI decode in NV12 Y tiled format so skip modifier change for NV12/P010 huge bo. */ + bool huge_bo = (i915->graphics_version < 11) && (width > 4096); if (huge_bo && format != DRM_FORMAT_NV12 && format != DRM_FORMAT_P010 && modifier != I915_FORMAT_MOD_X_TILED && modifier != DRM_FORMAT_MOD_LINEAR) { uint32_t i;