From 91cf9b4e439a9555935889a99c30aca43f3f9a8c Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Thu, 24 Apr 2025 14:12:21 -0400 Subject: [PATCH] asahi: do not use "Null" layout This is the most serious bug we've had in a long time due to a fundamental misunderstanding of the hardware (due to incomplete reverse-engineering). It caught me off guard. The texture descriptor has "mode" bits which configure two aspects of how the address pointer is interpreted: * whether it is indirected, pointing to a secondary page table for sparse * whether it writes texture access counters (for Metal's idea of sparse). ...Neither of these is a "null texture" mode. So why did I see Apple's blob using a non-normal mode for null textures, and why did I copy those settings? 1. Because the hardware texture access counters provide a cheap way to detect null texture accesses after the fact, which I think their GPU debug tools use. I'm not sure why release builds of the driver do/did that, but whatever. 2. Because I assumed Cupertino knew best and I didn't bother looking too close. We can't use them here (without doing extra memory allocations), since then the GPU will increment access counters. And since our null texture address used to just be a pointer in the command buffer, that mean the GPU will trash whatever memory happened to be 0x400 bytes after the start of the null texture descriptor. The symptom being random faults. This bug was caught when trying to use the zero-page instead, which raised a permission fault when the GPU tried to write counts. Then I remembered the sparse mechanism and had a bit of a eureka moment. Immediately followed by an "oh, f#$&" moment as I realized how many random bugs could potentially be root caused to this. The fix is two-fold: 1. Use normal layout instead. 2. Set the address to the zero-page (which is a fixed VA) and detect null textures by checking the address, instead of the mode. The latter is a good idea anyway, but both parts needs to be done atomically to maintain bisectability. Backport-to: 25.1 Signed-off-by: Alyssa Rosenzweig Part-of: (cherry picked from commit 3eb75756795ef29fd7a983ebeb0b095358aadc38) --- .pick_status.json | 2 +- src/asahi/genxml/cmdbuf.xml | 9 +++------ src/asahi/lib/agx_abi.h | 2 +- src/asahi/lib/agx_helpers.h | 5 ++--- src/asahi/libagx/texture.cl | 7 ++++--- src/asahi/vulkan/hk_device.c | 2 +- src/gallium/drivers/asahi/agx_state.c | 6 +++--- 7 files changed, 15 insertions(+), 18 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 248a6e4658e..4212b1c1f55 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -274,7 +274,7 @@ "description": "asahi: do not use \"Null\" layout", "nominated": true, "nomination_type": 4, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": null, "notes": null diff --git a/src/asahi/genxml/cmdbuf.xml b/src/asahi/genxml/cmdbuf.xml index c497b791283..605b81fb06a 100644 --- a/src/asahi/genxml/cmdbuf.xml +++ b/src/asahi/genxml/cmdbuf.xml @@ -203,12 +203,9 @@ - - - - + + + diff --git a/src/asahi/lib/agx_abi.h b/src/asahi/lib/agx_abi.h index 20b6a37fef9..22f54145a2a 100644 --- a/src/asahi/lib/agx_abi.h +++ b/src/asahi/lib/agx_abi.h @@ -24,4 +24,4 @@ * addressed with only small integers in the low/high. That lets us do some * robustness optimization even without soft fault. */ -#define AGX_ZERO_PAGE_ADDRESS (1ull << 32) +#define AGX_ZERO_PAGE_ADDRESS (((uint64_t)1) << 32) diff --git a/src/asahi/lib/agx_helpers.h b/src/asahi/lib/agx_helpers.h index 29a1aa07129..bc5859404ce 100644 --- a/src/asahi/lib/agx_helpers.h +++ b/src/asahi/lib/agx_helpers.h @@ -168,7 +168,7 @@ agx_pack_line_width(float line_width) * the texture descriptor itself. */ static void -agx_set_null_texture(struct agx_texture_packed *tex, uint64_t valid_address) +agx_set_null_texture(struct agx_texture_packed *tex) { agx_pack(tex, TEXTURE, cfg) { cfg.layout = AGX_LAYOUT_TWIDDLED; @@ -178,8 +178,7 @@ agx_set_null_texture(struct agx_texture_packed *tex, uint64_t valid_address) cfg.swizzle_g = AGX_CHANNEL_0; cfg.swizzle_b = AGX_CHANNEL_0; cfg.swizzle_a = AGX_CHANNEL_0; - cfg.address = valid_address; - cfg.mode = AGX_IMAGE_MODE_NULL; + cfg.address = AGX_ZERO_PAGE_ADDRESS; } } diff --git a/src/asahi/libagx/texture.cl b/src/asahi/libagx/texture.cl index 6bd551c472f..3f40d3fa462 100644 --- a/src/asahi/libagx/texture.cl +++ b/src/asahi/libagx/texture.cl @@ -3,6 +3,7 @@ * Copyright 2023 Valve Corporation * SPDX-License-Identifier: MIT */ +#include "asahi/lib/agx_abi.h" #include "compiler/libcl/libcl.h" #include "libagx_intrinsics.h" #include @@ -18,7 +19,7 @@ libagx_txs(constant struct agx_texture_packed *ptr, uint16_t lod, * * OpImageQuery*... return 0 if the bound descriptor is a null descriptor */ - if (d.mode == AGX_IMAGE_MODE_NULL) + if (d.address == AGX_ZERO_PAGE_ADDRESS) return 0; /* Buffer textures are lowered to 2D so the original size is irrecoverable. @@ -67,7 +68,7 @@ libagx_texture_samples(constant struct agx_texture_packed *ptr) agx_unpack(NULL, ptr, TEXTURE, d); /* As above */ - if (d.mode == AGX_IMAGE_MODE_NULL) + if (d.address == AGX_ZERO_PAGE_ADDRESS) return 0; /* We may assume the input is multisampled, so just check the samples */ @@ -79,7 +80,7 @@ libagx_texture_levels(constant struct agx_texture_packed *ptr) { agx_unpack(NULL, ptr, TEXTURE, d); - if (d.mode == AGX_IMAGE_MODE_NULL) + if (d.address == AGX_ZERO_PAGE_ADDRESS) return 0; else return (d.last_level - d.first_level) + 1; diff --git a/src/asahi/vulkan/hk_device.c b/src/asahi/vulkan/hk_device.c index 6d05e597f1d..2b9e27383cf 100644 --- a/src/asahi/vulkan/hk_device.c +++ b/src/asahi/vulkan/hk_device.c @@ -302,7 +302,7 @@ hk_upload_null_descriptors(struct hk_device *dev) struct agx_pbe_packed null_pbe; uint32_t offset_tex, offset_pbe; - agx_set_null_texture(&null_tex, dev->rodata.null_sink); + agx_set_null_texture(&null_tex); agx_set_null_pbe(&null_pbe, dev->rodata.null_sink); hk_descriptor_table_add(dev, &dev->images, &null_tex, sizeof(null_tex), diff --git a/src/gallium/drivers/asahi/agx_state.c b/src/gallium/drivers/asahi/agx_state.c index b3ae2bf9bb4..39e61873e64 100644 --- a/src/gallium/drivers/asahi/agx_state.c +++ b/src/gallium/drivers/asahi/agx_state.c @@ -2777,7 +2777,7 @@ agx_upload_textures(struct agx_batch *batch, struct agx_compiled_shader *cs, struct agx_sampler_view *tex = ctx->stage[stage].textures[i]; if (tex == NULL) { - agx_set_null_texture(&textures[i], T_tex.gpu); + agx_set_null_texture(&textures[i]); continue; } @@ -2793,7 +2793,7 @@ agx_upload_textures(struct agx_batch *batch, struct agx_compiled_shader *cs, } for (unsigned i = nr_active_textures; i < nr_textures; ++i) - agx_set_null_texture(&textures[i], T_tex.gpu); + agx_set_null_texture(&textures[i]); for (unsigned i = 0; i < nr_images; ++i) { /* Image descriptors come in pairs after the textures */ @@ -2803,7 +2803,7 @@ agx_upload_textures(struct agx_batch *batch, struct agx_compiled_shader *cs, struct agx_pbe_packed *pbe = (struct agx_pbe_packed *)(texture + 1); if (!(ctx->stage[stage].image_mask & BITFIELD_BIT(i))) { - agx_set_null_texture(texture, T_tex.gpu); + agx_set_null_texture(texture); agx_set_null_pbe(pbe, agx_pool_alloc_aligned(&batch->pool, 1, 64).gpu); continue; }