Skip to content

gfxstream: answer a deferred AHB image layout from the AHB itself #176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jvle wants to merge 8 commits into
base: main
Choose a base branch
from
Open

gfxstream: answer a deferred AHB image layout from the AHB itself #176

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
956c684
gfxstream: route vkGetImageSubresourceLayout through the global state
jvle Aug 25, 2026
e181255
gfxstream: answer a deferred AHB image layout from the AHB itself
jvle Aug 25, 2026
c1a1ee7
Merge branch 'main' into deferred-ahb-image-layout
jvle Aug 27, 2026
131d217
gfxstream: keep the deferred AHB layout from leaking host-visible types
jvle Aug 27, 2026
02cd169
gfxstream: release the deferred-layout AHB once vkAllocateMemory reso…
Aug 28, 2026
a290b16
gfxstream: reuse deferred-layout AHB for ColorBuffer
Sep 3, 2026
e55963e
gfxstream: drop the deferred-layout memoryTypeBits filtering
Sep 4, 2026
8f3eaf5
Merge branch 'main' into deferred-ahb-image-layout
gurchetansingh Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion host/vulkan/vk_common_operations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2064,7 +2064,7 @@ MTLResource_id VkEmulation::getMtlResourceFromVkDeviceMemory(VulkanDispatch* vk,
#ifdef __ANDROID__
// Allocate an AHardwareBuffer matching the given image's format, extent and usage.
// Returns nullptr on failure (caller falls back to the non-AHB allocation path).
static AHardwareBuffer* allocAhb(const VkImageCreateInfo* imageCreateInfo) {
AHardwareBuffer* allocAhb(const VkImageCreateInfo* imageCreateInfo) {
// Map VkFormat to the corresponding AHB format — must match to avoid tiling mismatch.
uint32_t ahbFormat;
switch (imageCreateInfo->format) {
Expand Down
10 changes: 10 additions & 0 deletions host/vulkan/vk_common_operations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
// limitations under the License.
#pragma once

#ifdef __ANDROID__
#include <android/hardware_buffer.h>
#endif

#include <GLES2/gl2.h>
#include <vulkan/vulkan.h>

Expand Down Expand Up @@ -205,6 +209,7 @@ class VkEmulation {
void onVkDeviceLost();

VkExternalMemoryHandleTypeFlagBits getDefaultExternalMemoryHandleType();

void appendExternalMemoryModeDeviceExtensions(std::vector<const char*>& outDeviceExtensions);
ExternalMemory::Mode getExternalMemoryMode() const;
bool supportsExternalMemory() {
Expand Down Expand Up @@ -740,6 +745,11 @@ class VkEmulation {
std::unique_ptr<UdmabufCreator> mUdmabufCreator;
};

#ifdef __ANDROID__
// Allocates an AHardwareBuffer matching an image's create info.
AHardwareBuffer* allocAhb(const VkImageCreateInfo* imageCreateInfo);
#endif

} // namespace vk
} // namespace host
} // namespace gfxstream
7 changes: 3 additions & 4 deletions host/vulkan/vk_decoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3399,14 +3399,12 @@ size_t VkDecoder::Impl::decode(void* buf, size_t len, IOStream* ioStream,
VkImage image;
const VkImageSubresource* pSubresource;
VkSubresourceLayout* pLayout;
// Begin non wrapped dispatchable handle unboxing for device;
// Begin global wrapped dispatchable handle unboxing for device;
uint64_t cgen_var_0;
memcpy((uint64_t*)&cgen_var_0, *readStreamPtrPtr, 1 * 8);
*readStreamPtrPtr += 1 * 8;
*(VkDevice*)&device = (VkDevice)(VkDevice)((VkDevice)(*&cgen_var_0));
auto unboxed_device = unbox_VkDevice(device);
auto vk = dispatch_VkDevice(device);
// End manual dispatchable handle unboxing for device;
uint64_t cgen_var_1;
memcpy((uint64_t*)&cgen_var_1, *readStreamPtrPtr, 1 * 8);
*readStreamPtrPtr += 1 * 8;
Expand Down Expand Up @@ -3435,7 +3433,8 @@ size_t VkDecoder::Impl::decode(void* buf, size_t len, IOStream* ioStream,
(unsigned long long)pSubresource, (unsigned long long)pLayout);
}
if (CC_LIKELY(vk)) {
vk->vkGetImageSubresourceLayout(unboxed_device, image, pSubresource, pLayout);
m_state->on_vkGetImageSubresourceLayout(&m_pool, snapshotApiCallHandle, device,
image, pSubresource, pLayout);
}
vkStream->unsetHandleMapping();
if (pLayout) {
Expand Down
174 changes: 166 additions & 8 deletions host/vulkan/vk_decoder_global_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3164,6 +3164,66 @@ class VkDecoderGlobalState::Impl {
imageInfo.imageCreateInfoShallow = vk_make_orphan_copy(*pCreateInfo);
imageInfo.layout = pCreateInfo->initialLayout;
imageInfo.anbInfo = std::move(anbInfo);
if (const auto* extMemCreateInfo =
vk_find_struct<VkExternalMemoryImageCreateInfo>(pCreateInfo)) {
imageInfo.externalHandleTypes = extMemCreateInfo->handleTypes;
}
#ifdef __ANDROID__
// Deferred image layout, see ImageInfo::DeferredLayoutInfo. Only for AHB-external
// images we are not already backing another way: the ANB path owns its own buffer, and for
// compressed images updateImageMemoryRequirementsLocked() overwrites them anyway.
if ((imageInfo.externalHandleTypes &
VK_EXTERNAL_MEMORY_HANDLE_TYPE_ANDROID_HARDWARE_BUFFER_BIT_ANDROID) &&
!imageInfo.anbInfo && !imageInfo.compressInfo) {
VkMemoryRequirements probeReqs = {};
vk->vkGetImageMemoryRequirements(device, *pImage, &probeReqs);
// Only intervene where the driver actually refused to answer. A driver that reports a
// real size needs no help from us, and substituting there would be a regression.
if (probeReqs.size == 0) {
AHardwareBuffer* rawAhb = allocAhb(pCreateInfo);
if (rawAhb) {
VkAndroidHardwareBufferPropertiesANDROID ahbProps = {
.sType = VK_STRUCTURE_TYPE_ANDROID_HARDWARE_BUFFER_PROPERTIES_ANDROID,
.pNext = nullptr,
};
VkResult propsRes =
vk->vkGetAndroidHardwareBufferPropertiesANDROID(device, rawAhb, &ahbProps);
if (propsRes == VK_SUCCESS && ahbProps.allocationSize > 0) {
imageInfo.deferredLayout.ahb =
std::shared_ptr<AHardwareBuffer>(rawAhb, [](AHardwareBuffer* b) {
if (b) AHardwareBuffer_release(b);
});
imageInfo.deferredLayout.size = ahbProps.allocationSize;
// The driver reported no alignment either; the AHB satisfies its own.
imageInfo.deferredLayout.alignment =
probeReqs.alignment ? probeReqs.alignment : 1;
imageInfo.deferredLayout.memoryTypeBits = ahbProps.memoryTypeBits;
// The driver will also refuse to report rowPitch for this image; the AHB
// knows its own stride (in pixels), so derive the byte pitch from it.
AHardwareBuffer_Desc ahbDesc = {};
AHardwareBuffer_describe(rawAhb, &ahbDesc);
uint32_t bytesPerPixel = 4; // allocAhb only ever picks 32-bit RGBA/BGRA
imageInfo.deferredLayout.rowPitch =
static_cast<VkDeviceSize>(ahbDesc.stride) * bytesPerPixel;
GFXSTREAM_INFO(
"DL-AHB tracked image=%p size=%llu typeBits=0x%x rowPitch=%llu "
"ahbStridePx=%u (driver said size=0)",
(void*)*pImage, (unsigned long long)imageInfo.deferredLayout.size,
imageInfo.deferredLayout.memoryTypeBits,
(unsigned long long)imageInfo.deferredLayout.rowPitch, ahbDesc.stride);
} else {
GFXSTREAM_ERROR(
"DL-AHB properties query failed (res=%d size=%llu); leaving "
"requirements untouched",
(int)propsRes, (unsigned long long)ahbProps.allocationSize);
AHardwareBuffer_release(rawAhb);
}
} else {
GFXSTREAM_ERROR("DL-AHB allocAhb failed for image=%p", (void*)*pImage);
}
}
}
#endif

if (boxImage) {
*pImage = new_boxed_non_dispatchable_VkImage(*pImage);
Expand Down Expand Up @@ -5454,14 +5514,18 @@ class VkDecoderGlobalState::Impl {
}
}

// An AHB-backed image does not need to be CPU-mappable, and it

@gurchetansingh gurchetansingh Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, memoryTypeBits is assigned directly from ahbProps.memoryTypeBits without stripping HOST_VISIBLE bits?

On Intel ANV (which uses unified memory where types often report HOST_VISIBLE), the guest may
pick a host-visible memory type, causing crosvm resource_map_blob() to fail and mmap64 to return EINVAL—the exact failure mode described in the comment.

Or am I missing something?

// must not be: if the guest picks a HOST_VISIBLE memory type, gfxstream has to expose the
// allocation as a mappable blob, and crosvm's resource_map_blob() only accepts Mesa handles --
// an AHB-backed blob fails with "invalid Mesa handle" and the guest's mmap64 returns EINVAL.
// So hand back only the device-local, non-host-visible subset when one exists.
void on_vkGetImageMemoryRequirements(gfxstream::base::BumpPool* pool, VkSnapshotApiCallHandle,
VkDevice boxed_device, VkImage image,
VkMemoryRequirements* pMemoryRequirements) {
auto device = unbox_VkDevice(boxed_device);
auto vk = dispatch_VkDevice(boxed_device);
vk->vkGetImageMemoryRequirements(device, image, pMemoryRequirements);
std::lock_guard<std::mutex> lock(mMutex);
updateImageMemorySizeLocked(device, image, pMemoryRequirements);

auto* deviceInfo = gfxstream::base::find(mDeviceInfo, device);
if (!deviceInfo) {
Expand All @@ -5477,9 +5541,33 @@ class VkDecoderGlobalState::Impl {
}

auto& physicalDeviceMemHelper = physicalDeviceInfo->memoryPropertiesHelper;
updateImageMemoryRequirementsLocked(device, image, pMemoryRequirements,
physicalDeviceMemHelper.get());
physicalDeviceMemHelper->transformToGuestMemoryRequirements(pMemoryRequirements);
}

// A driver that defers the layout also reports rowPitch=0; answer with the AHB's stride.
void on_vkGetImageSubresourceLayout(gfxstream::base::BumpPool*, VkSnapshotApiCallHandle,
VkDevice boxed_device, VkImage image,
const VkImageSubresource* pSubresource,
VkSubresourceLayout* pLayout) {
auto device = unbox_VkDevice(boxed_device);
auto vk = dispatch_VkDevice(boxed_device);
vk->vkGetImageSubresourceLayout(device, image, pSubresource, pLayout);
#ifdef __ANDROID__
if (pLayout && pLayout->rowPitch == 0) {
std::lock_guard<std::mutex> lock(mMutex);
auto* dlInfo = gfxstream::base::find(mImageInfo, image);
if (dlInfo && dlInfo->deferredLayout.rowPitch > 0) {
pLayout->rowPitch = dlInfo->deferredLayout.rowPitch;
if (pLayout->size == 0) pLayout->size = dlInfo->deferredLayout.size;
GFXSTREAM_INFO("DL-AHB stride image=%p rowPitch=%llu", (void*)image,
(unsigned long long)pLayout->rowPitch);
}
}
#endif
}

void on_vkGetImageMemoryRequirements2(gfxstream::base::BumpPool* pool, VkSnapshotApiCallHandle,
VkDevice boxed_device,
const VkImageMemoryRequirementsInfo2* pInfo,
Expand Down Expand Up @@ -5518,9 +5606,10 @@ class VkDecoderGlobalState::Impl {
&pMemoryRequirements->memoryRequirements);
}

updateImageMemorySizeLocked(device, pInfo->image, &pMemoryRequirements->memoryRequirements);

auto& physicalDeviceMemHelper = physicalDeviceInfo->memoryPropertiesHelper;
updateImageMemoryRequirementsLocked(device, pInfo->image,
&pMemoryRequirements->memoryRequirements,
physicalDeviceMemHelper.get());
physicalDeviceMemHelper->transformToGuestMemoryRequirements(
&pMemoryRequirements->memoryRequirements);
}
Expand Down Expand Up @@ -6311,6 +6400,35 @@ class VkDecoderGlobalState::Impl {
if (dedicatedAllocInfoPtr) {
localDedicatedAllocInfo = vk_make_orphan_copy(*dedicatedAllocInfoPtr);
}
#ifdef __ANDROID__
// The driver only resolves the layout if the bound memory carries an AHB, so import the
// image's AHB on its dedicated allocation. Function scope: vk_append_struct() only stores
// a pointer and the chain is consumed at vkAllocateMemory below.
VkImportAndroidHardwareBufferInfoANDROID importDeferredLayoutAhb = {

@gurchetansingh gurchetansingh Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add logic to make sure the importDeferredLayoutAhb never conflicts from the import add by importCbInfoPtr: I.e, we don't have duplication AHB extension struts ever.

.sType = VK_STRUCTURE_TYPE_IMPORT_ANDROID_HARDWARE_BUFFER_INFO_ANDROID,
.pNext = nullptr,
.buffer = nullptr,
};
// Keeps the AHB alive past the lock: the chain is not consumed until vkAllocateMemory
// below, by which point the image may have been destroyed.
std::shared_ptr<AHardwareBuffer> deferredAhbHold;
// A ColorBuffer import below appends its own AHB import for this same allocation --
// skip ours so the chain never carries two VkImportAndroidHardwareBufferInfoANDROID.
if (dedicatedAllocInfoPtr && dedicatedAllocInfoPtr->image != VK_NULL_HANDLE &&
!vk_find_struct<VkImportColorBufferGOOGLE>(pAllocateInfo)) {
std::lock_guard<std::mutex> dlLock(mMutex);

@gurchetansingh gurchetansingh Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, I think we can possibly eliminate this entire block on on_VkAllocateMemory all-together.

The reason is because the guest has:

                bufferBlob = instance->createBlob(createBlob);
                if (!bufferBlob) return VK_ERROR_OUT_OF_DEVICE_MEMORY;

and

    if (bufferBlob) {
        if (hasDedicatedBuffer) {
            importBufferInfo.buffer = bufferBlob->getResourceHandle();
            vk_append_struct(&structChainIter, &importBufferInfo);
        } else {
            importCbInfo.colorBuffer = bufferBlob->getResourceHandle();
            vk_append_struct(&structChainIter, &importCbInfo);
        }
    }

so importCbInfo is always appended when using an AHB. Therefore, I think we should move ownership of the AHB. So rather than AHardwareBuffer* allocAhb(const VkImageCreateInfo* imageCreateInfo) exposed to the vkDecobder, it should be std::optional<AhbInfo> VkEmulation::allocate_ahb(const VkImageCreateInfo):

struct AhbInfo {
        VkDeviceSize size = 0;
        VkDeviceSize alignment = 0;
        uint32_t memoryTypeBits = 0;
        VkDeviceSize rowPitch = 0;
    };

You would save the Ahb itself inside the VkEmulation, and then move when the createBlob requests. Essentially, you would have a table of pending Ahb, and if their properties match the create blob request the move occurs.

Right now, I think we might be double allocating? The deferred layout AHB and the createBlob AHB.

@jvle jvle Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right double allocating happened — I don't think the literal "match a pending AHB at createBlob time" version works, for a specific reason:

The issue: VkImportColorBufferGOOGLE only ever appears in VkMemoryAllocateInfo (at vkAllocateMemory) — it's never present in VkImageCreateInfo (at vkCreateImage), so there's no signal at image-creation time that a ColorBuffer import is coming. And on the other side, on_vkAllocateMemory's ColorBuffer handling only ever looks up an existing ColorBuffer by handle (getColorBufferAllocationInfo) — it never creates one. VkEmulation::createVkColorBufferLocked creates the ColorBuffer's own image + AHB independently, and normally before the guest ever calls vkCreateImage for the image that will later import it (confirmed this is consistent with the existing AddPendingBlob/TakePendingBlob mechanism in virtio_gpu_context.cpp, which correlates CREATE_3D metadata with a later CREATE_BLOB call — a similar but distinct pending mechanism, at the virtio-gpu resource level rather than the Vulkan level).

So by the time the deferred-layout probe fires in on_vkCreateImage, a ColorBuffer that will later be imported already exists with its own AHB — there's no "pending" probe AHB from an earlier point in time that a later ColorBuffer creation could adopt. Matching them at createBlob time as described would mean handing the same physical AHB to two unrelated images, which risks aliasing their memory.

What I would do instead: moved the probe into VkEmulation::getDeferredLayoutProbe(), memoized by AHB shape (width/height/format/usage) for the life of VkEmulation. It never keeps the AHB around — queries vkGetAndroidHardwareBufferPropertiesANDROID once per distinct shape and releases it immediately, caching only the 4 scalars (size/alignment/memoryTypeBits/rowPitch). on_vkAllocateMemory now allocates a real, non-shared AHB lazily, only when actually needed as backing memory (no ColorBuffer import present) — same cost as before for that case, but after the first image of a given shape, every other image sharing that shape costs zero extra AHardwareBuffer_allocate() calls for its probe. That should cover the common case you flagged (e.g. every same-sized window surface) without the aliasing risk.

Please let me know if that address your concern so I can send another commit for review

@gurchetansingh gurchetansingh Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by the time the deferred-layout probe fires in on_vkCreateImage, a ColorBuffer that will later be imported already exists with its own AHB — there's no "pending" probe AHB from an earlier point in time that a later ColorBuffer creation could adopt.

That does not match my mental model of how it's supposed to work. The flow I have in mind is:

  • Guest requests vkCreateImage with VkExternalMemoryImageCreateInfo and the dmabuf handle type. That is translated vkCreateImage to External Memory + AHB on the host. This is where we want to put the probe.
  • After getting the image size, the guest calls CreateBlob with the following path:
        if (hasDedicatedImage) {
            VkImageCreateInfo imageCreateInfo;
            {
                std::lock_guard<std::recursive_mutex> lock(mLock);

                auto it = info_VkImage.find(dedicatedAllocInfoPtr->image);
                if (it == info_VkImage.end()) return VK_ERROR_INITIALIZATION_FAILED;
                const auto& imageInfo = it->second;

                imageCreateInfo = imageInfo.createInfo;
            }

            // Need to query the stride of the underyling image resource
            // (VkSubresourceLayout::rowPitch) In most cases, the application will have created the
            // VkImage w/ VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT, in which case the aspectMask to
            // query is the PLANE_0_BIT resource. Otherwise, query the more generic COLOR_BIT.
            // Note: For VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT, the image may actually be emulated
            // with VK_IMAGE_TILING_LINEAR.
            const VkImageSubresource imageSubresource = {
                .aspectMask = (imageCreateInfo.tiling == VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT)
                                  ? VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT
                                  : VK_IMAGE_ASPECT_COLOR_BIT,
                .mipLevel = 0,
                .arrayLayer = 0,
            };
            VkSubresourceLayout subResourceLayout;
            enc->vkGetImageSubresourceLayout(device, dedicatedAllocInfoPtr->image,
                                             &imageSubresource, &subResourceLayout,
                                             true /* do lock */);

But if you don't see DedicatedImageCreateInfo, I suppose we can miss that path entirely? Let's try to confirm the path taken before deciding on what to do.

@jvle jvle Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — traced it in the guest (ResourceTracker.cpp, on_vkAllocateMemory, LINUX_GUEST_BUILD/exportDmabuf):

hasDedicatedImage is set from dedicatedAllocInfoPtr->image (line 3941-3942), and yes, that branch calls enc->vkGetImageSubresourceLayout() (line 3972) before createBlob — exactly the flow you described.
That createBlob uses kBlobFlagShareable | kBlobFlagCrossDevice, not kBlobFlagMappable (line 4006).
bufferBlob (from that createBlob) always gets appended as importCbInfo in the same vkAllocateMemory call, since hasDedicatedBuffer is false on this path (lines 4103-4111) — so on_vkAllocateMemory always sees VkImportColorBufferGOOGLE set here.
On why I don't think the AHB itself can be handed over at createBlob time: that resource is built through VirtioGpuContext::AddPendingBlob/TakePendingBlob (virtio_gpu_frontend.cpp:440,850), which only carries rc3d (width/height/format) — no reference to the Vulkan-level probe AHB from on_vkCreateImage. So the two live in different layers with no existing hook between them; wiring one up would mean threading the probe AHB (or a handle to it) through the virtio-gpu resource layer into VirtioGpuResource::Create, which is more invasive than either of us has proposed so far.

Let me know if that matches what you had in mind, or if I'm missing the hook you were picturing.

@gurchetansingh gurchetansingh Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the two live in different layers with no existing hook between them; wiring one up would mean threading the probe AHB (or a handle to it) through the virtio-gpu resource layer into VirtioGpuResource::Create, which is more invasive than either of us has proposed so far.

You are correct they live in two different threads, but they are serialized by the global VkEmulation and the various locks on that.

So if you:

  1. Intercept at vkCreateImage (create AHB there) and store in `VkEmulation``
  2. Wait for blob resource create to come down from guest vkAllocateMemory: which goes to VkEmulation::createVkColorBufferLocked.

the threading should work out

@jvle jvle Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the VkEmulation handoff you suggested. Deferred-layout AHBs are stashed by shape at vkCreateImage and adopted by matching ColorBuffer creation. If the source image imports its own AHB, it removes that entry first, so an in-use AHB cannot be reused.

I kept the memoryTypeBits filtering for now as a guard against exposing AHB-backed memory as mappable, but I can drop it if you prefer the smaller change.

Retested on fatcat/PTL with SystemBlob enabled and udmabuf disabled: Weston reaches the GL renderer with no error or SIGSEGV.

@gurchetansingh gurchetansingh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the memoryTypeBits filtering for now as a guard against exposing AHB-backed memory as mappable, but I can drop it if you prefer the smaller change.

Let's drop it. If it works without the memory-type filtering, we should avoid the impression that a piece of code is necessary for correctness.

Not requiring the memoryTypeBits filtering is actually good news, meaning that the guest never tries to map the AHB. This will enable future optimizations down the road.

@jvle jvle Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memoryTypeBits filtering has been dropped. Everything is still good

auto* dlInfo = gfxstream::base::find(mImageInfo, dedicatedAllocInfoPtr->image);
if (dlInfo && dlInfo->deferredLayout.ahb) {
deferredAhbHold = dlInfo->deferredLayout.ahb;
importDeferredLayoutAhb.buffer = deferredAhbHold.get();
vk_append_struct(&structChainIter, &importDeferredLayoutAhb);
GFXSTREAM_INFO("DL-AHB import image=%p ahb=%p size=%llu",
(void*)dedicatedAllocInfoPtr->image,
(void*)importDeferredLayoutAhb.buffer,
(unsigned long long)localAllocInfo.allocationSize);
}
}
#endif
if (!usingDirectMapping()) {
// We copy bytes 1 page at a time from the guest to the host
// if we are not using direct mapping. This means we can end up
Expand Down Expand Up @@ -10345,14 +10463,47 @@ class VkDecoderGlobalState::Impl {
return false;
}

void updateImageMemorySizeLocked(VkDevice device, VkImage image,
VkMemoryRequirements* pMemoryRequirements) REQUIRES(mMutex) {
void updateImageMemoryRequirementsLocked(
VkDevice device, VkImage image, VkMemoryRequirements* pMemoryRequirements,
const EmulatedPhysicalDeviceMemoryProperties* memHelper = nullptr) REQUIRES(mMutex) {
auto* imageInfo = gfxstream::base::find(mImageInfo, image);
if (!imageInfo || !imageInfo->compressInfo) {
if (!imageInfo) return;

if (imageInfo->compressInfo) {
*pMemoryRequirements = imageInfo->compressInfo->getMemoryRequirements();
return;
}

*pMemoryRequirements = imageInfo->compressInfo->getMemoryRequirements();
#ifdef __ANDROID__
// A driver that defers an AHB-external image's layout answers size=0 until bind. Answer
// with what the image's own AHB needs instead. Host memory type indices here; the caller
// must still run transformToGuestMemoryRequirements afterwards.
if (pMemoryRequirements && pMemoryRequirements->size == 0 &&
imageInfo->deferredLayout.size > 0) {
pMemoryRequirements->size = imageInfo->deferredLayout.size;
pMemoryRequirements->alignment = imageInfo->deferredLayout.alignment;

uint32_t typeBits = imageInfo->deferredLayout.memoryTypeBits;

@gurchetansingh gurchetansingh Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking further, I was thinking we don't need to worry about updating the memory types anymore. The reason is when creating color buffers, kBlobFlagMappable is not set, but kBlobFlagShareable | kBlobFlagCrossDevice is.

Since your prior commit did not mutate the memory types and it worked, there a good chance we get lucky again and can avoid the complexity for now (we'll probably aim for a more robust solution long-term, but that might require some new Android graphics APIs).

// The AHB must not be exposed as a host-visible/mappable type: crosvm's
// resource_map_blob() rejects AHB-backed blobs, and on a unified-memory host
// (e.g. Intel ANV) most or all of ahbProps.memoryTypeBits can be HOST_VISIBLE.
// Prefer the non-host-visible subset; fall back to the full mask if the AHB has
// no such type, since some allocation beats failing this call outright.
if (memHelper) {
const auto& hostProps = memHelper->getHostMemoryProperties();
uint32_t deviceLocalOnlyBits = 0;
for (uint32_t i = 0; i < hostProps.memoryTypeCount; i++) {
if ((typeBits & (1u << i)) && !(hostProps.memoryTypes[i].propertyFlags &
VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT)) {
deviceLocalOnlyBits |= (1u << i);
}
}
if (deviceLocalOnlyBits != 0) {
typeBits = deviceLocalOnlyBits;
}
}
pMemoryRequirements->memoryTypeBits = typeBits;
}
#endif
}

bool enableEmulatedEtc2() const { return m_vkEmulation->isEtc2EmulationEnabled(); }
Expand Down Expand Up @@ -12015,6 +12166,13 @@ void VkDecoderGlobalState::on_vkCmdCopyImageToBuffer2KHR(
mImpl->on_vkCmdCopyImageToBuffer2KHR(pool, apiCallHandle, commandBuffer, pCopyImageToBufferInfo);
}

void VkDecoderGlobalState::on_vkGetImageSubresourceLayout(
gfxstream::base::BumpPool* pool, VkSnapshotApiCallHandle apiCallHandle, VkDevice device,
VkImage image, const VkImageSubresource* pSubresource, VkSubresourceLayout* pLayout) {
mImpl->on_vkGetImageSubresourceLayout(pool, apiCallHandle, device, image, pSubresource,
pLayout);
}

void VkDecoderGlobalState::on_vkGetImageMemoryRequirements(
gfxstream::base::BumpPool* pool, VkSnapshotApiCallHandle apiCallHandle, VkDevice device,
VkImage image, VkMemoryRequirements* pMemoryRequirements) {
Expand Down
5 changes: 5 additions & 0 deletions host/vulkan/vk_decoder_global_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,11 @@ class VkDecoderGlobalState {
VkSnapshotApiCallHandle apiCallHandle, VkDevice device,
VkImage image, VkMemoryRequirements* pMemoryRequirements);

void on_vkGetImageSubresourceLayout(gfxstream::base::BumpPool* pool,
VkSnapshotApiCallHandle apiCallHandle, VkDevice device,
VkImage image, const VkImageSubresource* pSubresource,
VkSubresourceLayout* pLayout);

void on_vkGetImageMemoryRequirements2(gfxstream::base::BumpPool* pool,
VkSnapshotApiCallHandle apiCallHandle, VkDevice device,
const VkImageMemoryRequirementsInfo2* pInfo,
Expand Down
19 changes: 19 additions & 0 deletions host/vulkan/vk_decoder_internal_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@

#include <vulkan/vulkan.h>

#ifdef __ANDROID__
#include <android/hardware_buffer.h>
#endif

#ifdef _WIN32
#include <malloc.h>
#endif

#include <stdlib.h>

#include <condition_variable>
#include <memory>
#include <mutex>
#include <optional>
#include <set>
Expand Down Expand Up @@ -359,6 +364,20 @@ struct ImageInfo {
// TODO: might need to use an array of layouts to represent each sub resource
VkImageLayout layout = VK_IMAGE_LAYOUT_UNDEFINED;
VkDeviceMemory memory = VK_NULL_HANDLE;
// From VkExternalMemoryImageCreateInfo; imageCreateInfoShallow drops pNext.
VkExternalMemoryHandleTypeFlags externalHandleTypes = 0;
#ifdef __ANDROID__
// Set when a driver defers an AHB-external image's layout to vkBindImageMemory and reports
// size=0 until then. The AHB is shared_ptr-held so every mImageInfo teardown path frees it.
struct DeferredLayoutInfo {
std::shared_ptr<AHardwareBuffer> ahb;
VkDeviceSize size = 0;
VkDeviceSize alignment = 0;
uint32_t memoryTypeBits = 0;
VkDeviceSize rowPitch = 0;
};
DeferredLayoutInfo deferredLayout;
#endif
};

struct ImageViewInfo {
Expand Down