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

[jvle]

Second of the pieces @gurchetansingh asked for on the ARSP review — the hand-coded
half. The regenerated vk_decoder.cpp and the mappable-only-blob change follow as
separate PRs.

What goes wrong

Intel ANV leaves an AHB-external image's layout unresolved until vkBindImageMemory
and reports size=0, alignment=0 until then. That is spec-valid, but guests take it
literally:

MESA: error: Failed to allocate coherent memory: failed to allocate on the host: -1.
MESA: error: zink: couldn't allocate memory: heap=0 size=0
weston.service: Main process exited, code=killed, status=11/SEGV

The guest allocates and maps host-visible memory in one step inside
vkAllocateMemory (ResourceTracker::getCoherentMemory), so a zero-sized request
fails at the host allocation with VK_ERROR_OUT_OF_HOST_MEMORY — the -1 above —
before anything is mapped.

Why the image is AHB-external at all

Not the guest's doing. A Linux guest asks for DMA_BUF_BIT_EXT, and
transformImpl_VkImageCreateInfo_tohost ORs in the host's default handle type, which
is AHB on an Android host. That is deliberate — it is what lets the guest's
window-system image alias an AHB-backed ColorBuffer — so the requirements have to be
answered rather than the handle type avoided.

I did try the other direction (dmabuf end to end via VulkanExternalMemoryMode:OpaqueFd)
and it does remove this failure, but scanout then breaks: the Android display path needs
AHB for zero-copy, and the composited frame never reaches the surface. So this seemed
like the right place to fix it.

Approach

Allocate the AHB up front for an image the driver refuses to describe, take the real
size from vkGetAndroidHardwareBufferPropertiesANDROID, substitute it, and import the
same AHB on the guest's dedicated allocation so the driver resolves the layout on bind.

Placement is load-bearing: after updateImageMemorySizeLocked (which rewrites the
struct for compressed images) and before transformToGuestMemoryRequirements, because
ahbProps.memoryTypeBits is in host indices and that transform is what maps them to
guest indices.

Note on scope

on_vkGetImageSubresourceLayout is added here but is not reached until the decoder
routes that call through VkDecoderGlobalState — that is
Mesa MR 43969 plus the
regenerated vk_decoder.cpp, which I will send once the Mesa side lands.

Testing

Intel PTL Android host: Weston reaches GL renderer: zink Vulkan 1.4 (Intel(R) Graphics (PTL)), the host substitutes size=3768320 for the 1280x720 scanout image, and
glmark2-wayland runs accelerated on zink/Intel PTL.

[gurchetansingh]

Can you separate the codegen changes to commit (1) [with a no-op impl] and the actual implemntation to commit (2)? Thanks!

[gurchetansingh]

Group these into:

    struct DeferredLayoutInfo {
        std::shared_ptr<AHardwareBuffer> ahb;
        VkDeviceSize size = 0;
        VkDeviceSize alignment = 0;
        uint32_t memoryTypeBits = 0;
        VkDeviceSize rowPitch = 0;
    };

[gurchetansingh]

Umm, looks like LLM-speak :-), I suggest rewording this or just deleting this.

[gurchetansingh]

This can be consolidated into a helper function:

    void updateImageMemoryRequirementsLocked(VkDevice device, VkImage image,                                                                           
                                             VkMemoryRequirements* pMemoryRequirements) REQUIRES(mMutex) {                                             
        auto* imageInfo = gfxstream::base::find(mImageInfo, image);                                                                                    
        if (!imageInfo) return;                                                                                                                        
                                                                                                                                                       
        if (imageInfo->compressInfo) {                                                                                                                 
            *pMemoryRequirements = imageInfo->compressInfo->getMemoryRequirements();                                                                   
            return;                                                                                                                                    
        }                                                                                                                                              
    #ifdef __ANDROID__                                                                                                                                 
        if (pMemoryRequirements && pMemoryRequirements->size == 0 && imageInfo->deferredLayoutSize > 0) {                                              
            pMemoryRequirements->size = imageInfo->deferredLayoutSize;                                                                                 
            pMemoryRequirements->alignment = imageInfo->deferredLayoutAlignment;                                                                       
            pMemoryRequirements->memoryTypeBits = imageInfo->deferredLayoutMemoryTypeBits;                                                             
        }                                                                                                                                              
    #endif                                                                                                                                             
    }                                                                                                                                                  

[gurchetansingh]

Hold a shared_ptr<> since you drop the mutex:

   std::shared_ptr<AHardwareBuffer> deferredAhbHold;                                                                                                  
    if (dedicatedAllocInfoPtr && dedicatedAllocInfoPtr->image != VK_NULL_HANDLE) {                                                                     
        std::lock_guard<std::mutex> dlLock(mMutex);                                                                                                    
        auto* dlInfo = gfxstream::base::find(mImageInfo, dedicatedAllocInfoPtr->image);                                                                
        if (dlInfo && dlInfo->deferredLayoutAhb) {                                                                                                     
            deferredAhbHold = dlInfo->deferredLayoutAhb;                                                                                               
            importDeferredLayoutAhb.buffer = deferredAhbHold.get();                                                                                    

[jvle]

Thanks — all addressed, now two commits.

• (1) codegen routing + a pass-through impl, no behaviour change. (2) the implementation.
• DeferredLayoutInfo struct as suggested.
• Trimmed the comments — you were right, that was too much prose.
• Both requirements handlers now go through updateImageMemoryRequirementsLocked, which also
  absorbs the old updateImageMemorySizeLocked.
• shared_ptr held across the lock drop — good catch, that was a real use-after-free.

Retested on Intel PTL: Weston on its GL renderer, substitution and stride both applied, no
transfer or blob errors.

One note on (1): vk_decoder.cpp carries only the delta MR 43969 produces, not a full regen — a
plain regeneration also rewrites ~87 unrelated lines across three generated files, because the
checked-in output has drifted from the generator. Happy to send that separately if you want it
cleaned up.

[gurchetansingh]

Looks better, few questions though

[gurchetansingh]

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?

[gurchetansingh]

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.

[jvle]

Both real, thanks for catching these — pushed as a third commit.

memoryTypeBits: confirmed — deferredLayout.memoryTypeBits was a verbatim copy of ahbProps.memoryTypeBits, and nothing stripped HOST_VISIBLE. Now masking down to the host's non-host-visible types (falling back to the full mask only if the AHB has none), before transformToGuestMemoryRequirements runs.

Duplicate AHB import: also confirmed reachable — vkAllocateMemory appends importDeferredLayoutAhb unconditionally whenever the dedicated-alloc image has a deferred layout, and separately appends its own VkImportAndroidHardwareBufferInfoANDROID whenever VkImportColorBufferGOOGLE is present and resolves to the Android branch — both can be true for the same allocation. Now skipping the deferred-layout import whenever a ColorBuffer import is present; that path supplies its own.

Retested on Intel PTL/fatcat: Weston GL renderer, desktop and app windows composite correctly, no GFXSTREAM errors.

[gurchetansingh]

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]

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]

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.

[gurchetansingh]

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).