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 <alyssa@rosenzweig.io> Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34703> (cherry picked from commit 3eb75756795ef29fd7a983ebeb0b095358aadc38)
This commit is contained in:

committed by
Eric Engestrom

parent
e1f06788f5
commit
91cf9b4e43
@@ -274,7 +274,7 @@
|
|||||||
"description": "asahi: do not use \"Null\" layout",
|
"description": "asahi: do not use \"Null\" layout",
|
||||||
"nominated": true,
|
"nominated": true,
|
||||||
"nomination_type": 4,
|
"nomination_type": 4,
|
||||||
"resolution": 0,
|
"resolution": 1,
|
||||||
"main_sha": null,
|
"main_sha": null,
|
||||||
"because_sha": null,
|
"because_sha": null,
|
||||||
"notes": null
|
"notes": null
|
||||||
|
@@ -203,12 +203,9 @@
|
|||||||
|
|
||||||
<enum name="Image Mode">
|
<enum name="Image Mode">
|
||||||
<value name="Normal" value="0"/>
|
<value name="Normal" value="0"/>
|
||||||
<!-- Needs investigation, only seen with null textures so far -->
|
<!-- This is r/e, I can name things however I'd like. -->
|
||||||
<value name="Null" value="1"/>
|
<value name="Not sparse but probably with counters" value="1"/>
|
||||||
<!-- This is r/e, I can name things however I'd like.
|
<value name="Sparse but probably no counters" value="2"/>
|
||||||
Might be a legacy mode of some kind, we should investigate.
|
|
||||||
-->
|
|
||||||
<value name="Sparse but it does not seem to work" value="2"/>
|
|
||||||
<value name="Sparse" value="3"/>
|
<value name="Sparse" value="3"/>
|
||||||
</enum>
|
</enum>
|
||||||
|
|
||||||
|
@@ -24,4 +24,4 @@
|
|||||||
* addressed with only small integers in the low/high. That lets us do some
|
* addressed with only small integers in the low/high. That lets us do some
|
||||||
* robustness optimization even without soft fault.
|
* robustness optimization even without soft fault.
|
||||||
*/
|
*/
|
||||||
#define AGX_ZERO_PAGE_ADDRESS (1ull << 32)
|
#define AGX_ZERO_PAGE_ADDRESS (((uint64_t)1) << 32)
|
||||||
|
@@ -168,7 +168,7 @@ agx_pack_line_width(float line_width)
|
|||||||
* the texture descriptor itself.
|
* the texture descriptor itself.
|
||||||
*/
|
*/
|
||||||
static void
|
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) {
|
agx_pack(tex, TEXTURE, cfg) {
|
||||||
cfg.layout = AGX_LAYOUT_TWIDDLED;
|
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_g = AGX_CHANNEL_0;
|
||||||
cfg.swizzle_b = AGX_CHANNEL_0;
|
cfg.swizzle_b = AGX_CHANNEL_0;
|
||||||
cfg.swizzle_a = AGX_CHANNEL_0;
|
cfg.swizzle_a = AGX_CHANNEL_0;
|
||||||
cfg.address = valid_address;
|
cfg.address = AGX_ZERO_PAGE_ADDRESS;
|
||||||
cfg.mode = AGX_IMAGE_MODE_NULL;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@@ -3,6 +3,7 @@
|
|||||||
* Copyright 2023 Valve Corporation
|
* Copyright 2023 Valve Corporation
|
||||||
* SPDX-License-Identifier: MIT
|
* SPDX-License-Identifier: MIT
|
||||||
*/
|
*/
|
||||||
|
#include "asahi/lib/agx_abi.h"
|
||||||
#include "compiler/libcl/libcl.h"
|
#include "compiler/libcl/libcl.h"
|
||||||
#include "libagx_intrinsics.h"
|
#include "libagx_intrinsics.h"
|
||||||
#include <agx_pack.h>
|
#include <agx_pack.h>
|
||||||
@@ -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
|
* 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;
|
return 0;
|
||||||
|
|
||||||
/* Buffer textures are lowered to 2D so the original size is irrecoverable.
|
/* 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);
|
agx_unpack(NULL, ptr, TEXTURE, d);
|
||||||
|
|
||||||
/* As above */
|
/* As above */
|
||||||
if (d.mode == AGX_IMAGE_MODE_NULL)
|
if (d.address == AGX_ZERO_PAGE_ADDRESS)
|
||||||
return 0;
|
return 0;
|
||||||
|
|
||||||
/* We may assume the input is multisampled, so just check the samples */
|
/* 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);
|
agx_unpack(NULL, ptr, TEXTURE, d);
|
||||||
|
|
||||||
if (d.mode == AGX_IMAGE_MODE_NULL)
|
if (d.address == AGX_ZERO_PAGE_ADDRESS)
|
||||||
return 0;
|
return 0;
|
||||||
else
|
else
|
||||||
return (d.last_level - d.first_level) + 1;
|
return (d.last_level - d.first_level) + 1;
|
||||||
|
@@ -302,7 +302,7 @@ hk_upload_null_descriptors(struct hk_device *dev)
|
|||||||
struct agx_pbe_packed null_pbe;
|
struct agx_pbe_packed null_pbe;
|
||||||
uint32_t offset_tex, offset_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);
|
agx_set_null_pbe(&null_pbe, dev->rodata.null_sink);
|
||||||
|
|
||||||
hk_descriptor_table_add(dev, &dev->images, &null_tex, sizeof(null_tex),
|
hk_descriptor_table_add(dev, &dev->images, &null_tex, sizeof(null_tex),
|
||||||
|
@@ -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];
|
struct agx_sampler_view *tex = ctx->stage[stage].textures[i];
|
||||||
|
|
||||||
if (tex == NULL) {
|
if (tex == NULL) {
|
||||||
agx_set_null_texture(&textures[i], T_tex.gpu);
|
agx_set_null_texture(&textures[i]);
|
||||||
continue;
|
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)
|
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) {
|
for (unsigned i = 0; i < nr_images; ++i) {
|
||||||
/* Image descriptors come in pairs after the textures */
|
/* 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);
|
struct agx_pbe_packed *pbe = (struct agx_pbe_packed *)(texture + 1);
|
||||||
|
|
||||||
if (!(ctx->stage[stage].image_mask & BITFIELD_BIT(i))) {
|
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);
|
agx_set_null_pbe(pbe, agx_pool_alloc_aligned(&batch->pool, 1, 64).gpu);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user