Show panic alerts if the CP configuration doesn't match the XF configuration #10672

Pokechu22 · 2022-05-17T21:16:42Z

This is somewhat similar to #8717. At least in some cases, on real hardware this mismatched configuration can result in a hang, but Dolphin was previously allowing it without any warnings (I ran into this when experimenting with embossing).

I've also added a check that the CP and XF matrix index registers match, since it's weird that they are duplicated (we currently always use the CP one with the hardware backends, and the XF one for the software renderer), and the XF input vertex spec register doesn't include a count for matrix indices (so if per-vertex matrix indices are enabled, it's not clear how XF knows which ones are in use).

Ways this could be triggered

As far as I can tell, this kind of bug can only easily be caused with normals, since the normal count is based on both the vertex descriptor and the vertex attribute table while colors and texture coordinates are based on the vertex descriptor only. I ran into this by using GX_SetVtxAttrFmt(GX_VTXFMT0, GX_VA_NBT, ...) but also GX_SetVtxDesc(GX_VA_NRM, ...); you need to use either GX_VA_NBT or GX_VA_NRM in both places. As far as I can tell, that applies to both libogc and the official SDK.

Libogc also seems to have a bug where using GX_SetVtxDesc(GX_VA_NRM, GX_NONE) will result in 1 normal being used for the XF configuration, and GX_SetVtxDesc(GX_VA_NBT, GX_NONE) will result in 3 normals being used (the only way to set it to 0 is to use GX_ClearVtxDesc()). The official SDK does not have this issue.

Libogc GX_SetVtxAttrFmt

if (vtxattr==GX_VA_NRM && comptype==GX_NRM_XYZ
	&& (compsize==GX_S8 || compsize==GX_S16 || compsize==GX_F32))
{
	__gx->VAT0reg[vat].NormalElements = NormalComponentCount::N;
	__gx->VAT0reg[vat].NormalFormat = compsize;
	__gx->VAT0reg[vat].NormalIndex3 = false;
}
else if (vtxattr==GX_VA_NBT && (comptype==GX_NRM_NBT || comptype==GX_NRM_NBT3)
	&& (compsize==GX_S8 || compsize==GX_S16 || compsize==GX_F32))
{
	__gx->VAT0reg[vat].NormalElements = NormalComponentCount::NTB;
	__gx->VAT0reg[vat].NormalFormat = compsize;
	if (comptype==GX_NRM_NBT3)
		__gx->VAT0reg[vat].NormalIndex3 = true;
}

SDK (from Super Mario Sunshine) GXSetVtxAttrFmt

if (vtxattr==GX_VA_NRM || vtxattr == GX_VA_NBT)
{
	__gx->VAT0reg[vat].NormalFormat = compsize;
	if (comptype == GX_NRM_NBT3)
	{
		__gx->VAT0reg[vat].NormalElements = NormalComponentCount::NTB;
		__gx->VAT0reg[vat].NormalIndex3 = true;
	}
	else
	{
		if (comptype == GX_NRM_NBT)
			__gx->VAT0reg[vat].NormalElements = NormalComponentCount::NTB;
		else
			__gx->VAT0reg[vat].NormalElements = NormalComponentCount::N;
		__gx->VAT0reg[vat].NormalIndex3 = false;
	}
	break;
}

Libogc GX_SetVtxDesc

// vcdNrms is only set to 0 by GX_ClearVtxDesc; it's not set if type is GX_NONE
if (vtxattr==GX_VA_NRM)
{
	__gx->vcdLo.Normal = type;
	__gx->vcdNrms = 1;
}
else if (vtxattr==GX_VA_NBT)
{
	__gx->vcdLo.Normal = type;
	__gx->vcdNrms = 2;
}

SDK (from Super Mario Sunshine) GXSetVtxDesc

if (vtxattr==GX_VA_NRM)
{
	if (type == GX_NONE)
	{
		__gx->HasNormal = false;  // gx + 0x41c
	}
	else
	{
		__gx->HasNormal = true;  // gx + 0x41c
		__gx->HasNTB = false;  // gx + 0x41d
		__gx->NormalFmt = type;  // gx + 0x418
	}
}
else if (vtxattr==GX_VA_NBT)
{
	if (type == GX_NONE)
	{
		__gx->HasNTB = false;  // gx + 0x41c
	}
	else
	{
		__gx->HasNTB = true;  // gx + 0x41c
		__gx->HasNormal = false;  // gx + 0x41d
		__gx->NormalFmt = type;  // gx + 0x418
	}
}
if ((gx->HasNormal == false) && (gx->HasNTB == false))
	__gx->vcdLo.Normal = GX_NONE;
else
	__gx->vcdLo.Normal = gx->NormalFmt;

Libogc __GX_XfVtxSpecs

u32 nrms = 0;
if (__gx->vcdNrms==1) nrms = 1;
else if (__gx->vcdNrms==2) nrms = 2;

SDK (from Super Mario Sunshine) __GXXfVtxSpecs

u32 nrms;
if (gx->HasNTB == false)
{
	if (gx->HasNormal == false)
	{
		nrms = 0;
	}
	else
	{
		nrms = 1;
	}
}
else
{
	nrms = 2;
}

__GX_SetMatrixIndex and its SDK version always set both the CP and XF version to the same values, so that shouldn't have any issues.

I currently do this check in VertexLoaderManager::RunVertices, which is called for every primitive command. This might be more frequent than it should be, but I can't put it in VertexManagerBase::Flush as we need to know the vtx_attr_group (and Flush might be called after multiple different vtx_attr_group values have been used).

iwubcode · 2022-05-17T21:38:04Z

Source/Core/VideoCommon/VertexLoaderManager.cpp

@@ -265,6 +267,82 @@ int RunVertices(int vtx_attr_group, OpcodeDecoder::Primitive primitive, int coun
  if (is_preprocess)
    return size;

+  // Validate that the XF input configuration matches the CP configuration


Maybe wrap this whole block in a function?

iwubcode · 2022-05-17T21:38:23Z

Source/Core/VideoCommon/VertexLoaderManager.cpp

+    break;
+  default:
+    PanicAlertFmt("xfmem.invtxspec.numnormals is invalid: {}", xfmem.invtxspec.numnormals);
+    num_xf_normals = 888;


It's an arbitrary number that num_cp_normals cannot be (so that the second test always fails); leaving it uninitialized would result in undefined behavior, and using 0 could result in no game quirk being reported.

I know the c++ community has different views on it. But rather than magic numbers for values being undefined, I personally prefer std::optional.

If you do want to use a magic number like this, please add a constant or a comment.

I thought that std::optional would make the num_cp_normals != num_xf_normals check more complicated (instead requiring !num_xf_normals || num_cp_normals != num_xf_normals but it seems like num_cp_normals will be automatically converted to a present optional and if num_xf_normals is not present then the two are considered not equal. There isn't a fmt::formatter for std::optional though so that logic is more complicated though...

Don't know if it'd be useful to do so but we could add a formatter for optional, someone did so here: fmtlib/fmt#1367

Eh, I think manually handling it here is OK since it means we can use a specific message for the not-present case (rather than always using "NO VALUE" or the like). If it ends up being a more common problem then we can add one though.

iwubcode

Untested. LGTM

…ptions

…x config This probably isn't triggered by real games, but it's possible to accidentally do it with libogc (which results in freezes on real hardware).

… indices This almost certainly never happens, but if it does we want to know.

Pokechu22 · 2022-05-18T21:47:44Z

I made an adjustment to the message; it now includes the relevant registers (in hex), as well as the VAT index:

(I was going to do it using the formatter, but it's way too large.)

iwubcode reviewed May 17, 2022

View reviewed changes

Pokechu22 force-pushed the xf-invtxspec branch 2 times, most recently from 4394ba6 to c724691 Compare May 17, 2022 22:44

iwubcode approved these changes May 18, 2022

View reviewed changes

Pokechu22 added 4 commits May 18, 2022 14:43

XFMemory: Rename hostinfo to invtxspec

736466a

Rename CP and XF normal component count enums and update their descri…

46bcdc4

…ptions

Show a panic alert if the CP vertex config doesn't match the XF verte…

38a75f6

…x config This probably isn't triggered by real games, but it's possible to accidentally do it with libogc (which results in freezes on real hardware).

Show a panic alert if the CP matrix indices don't match the XF matrix…

8df55b4

… indices This almost certainly never happens, but if it does we want to know.

Pokechu22 force-pushed the xf-invtxspec branch from c724691 to 8df55b4 Compare May 18, 2022 21:44

Pokechu22 merged commit 2aa0ae0 into dolphin-emu:master May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show panic alerts if the CP configuration doesn't match the XF configuration #10672

Show panic alerts if the CP configuration doesn't match the XF configuration #10672

Pokechu22 commented May 17, 2022

iwubcode May 17, 2022 •

edited

Loading

Pokechu22 May 17, 2022

iwubcode May 17, 2022

Pokechu22 May 17, 2022

iwubcode May 17, 2022

JosJuice May 17, 2022

Pokechu22 May 17, 2022 •

edited

Loading

iwubcode May 17, 2022

Pokechu22 May 17, 2022 •

edited

Loading

iwubcode left a comment

Pokechu22 commented May 18, 2022

Show panic alerts if the CP configuration doesn't match the XF configuration #10672

Show panic alerts if the CP configuration doesn't match the XF configuration #10672

Conversation

Pokechu22 commented May 17, 2022

iwubcode May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Pokechu22 May 17, 2022

Choose a reason for hiding this comment

iwubcode May 17, 2022

Choose a reason for hiding this comment

Pokechu22 May 17, 2022

Choose a reason for hiding this comment

iwubcode May 17, 2022

Choose a reason for hiding this comment

JosJuice May 17, 2022

Choose a reason for hiding this comment

Pokechu22 May 17, 2022 • edited Loading

Choose a reason for hiding this comment

iwubcode May 17, 2022

Choose a reason for hiding this comment

Pokechu22 May 17, 2022 • edited Loading

Choose a reason for hiding this comment

iwubcode left a comment

Choose a reason for hiding this comment

Pokechu22 commented May 18, 2022

iwubcode May 17, 2022 •

edited

Loading

Pokechu22 May 17, 2022 •

edited

Loading

Pokechu22 May 17, 2022 •

edited

Loading