Robust GPU error recovery [Windows] by dvijayak · Pull Request #99 · parsec-cloud/libmatoya

dvijayak · 2024-04-02T14:06:23Z

~~This PR introduces the ability for users of libmatoya to query for errors and handle them accordingly. Currently, only GPU errors are being reported.~~

~~The proof-of-concept is done. What is pending is to clean up the implementation.~~

Improves on the way GPU errors are handled while drawing quads or UI. Matoya now handles these cases gracefully and resets the pipeline seamlessly in these scenarios.

Additionally, a few fixes were made as they were discovered while implementing error recovery.

WINDOWS-ONLY for now d3d11 and d3d12

Work In-Scope or Completed

Changes to the platform-specific gfx modules that add better recovery in the event of fatal GPU errors.
Improvement to MTY_WindowSetUITexture. Now the existing texture for the given id is only deleted if the new texture was successfully created. (Discovered this bug that was causing log-spam when GPU was reset while the app window was minimized to tray).
Updated MTY_WindowGetContextState to consider device resets as deeming the window's graphics context state as MTY_CONTEXT_STATE_NEW so that matoya clients will know to resubmit textures when GPU errors/resets take place.

… thread and not a window. Defined error codes so that matoya clients can action differently based on the error received while still masking the internal error details.

bmcnett · 2024-04-05T00:12:38Z

it occurs to me that maybe there is something much simpler that solves the problem at hand: instead of a queue of MTY_Error, you may really want a bitmask, with one bit for each possible MTY_Error. if there are 64 or fewer error types, this fits into a uint64_t. it can be tested by ANDing the uint64_t with (1ULL << error_code), etc. this is because you probably don't care exactly what order the errors occurred in, but you probably do care about which combination of 2+ errors occurred.

… matoya client is informed that textures need to be resubmitted.

…g texture for the id. It only does so if the new texture successfully was created.

dvijayak · 2024-05-08T12:00:23Z

 	if (!cmn->gfx_ui)
 		cmn->gfx_ui = GFX_UI_API[cmn->api].create(device);

-	if (!cmn->ui_textures)
+	if (!cmn->gfx_ui) {
+		MTY_HashDestroy(&cmn->ui_textures, NULL);
+
+	} else 	if (!cmn->ui_textures) {
 		cmn->ui_textures = MTY_HashCreate(0);
+	}

 	return cmn->gfx_ui != NULL;
 }


If the gfx_ui state could not be created from the given device, then we want to clean out the hash of textures.

dvijayak · 2024-05-08T12:01:33Z

 	MTY_Device *device = gfx_ctx_get_device(cmn);
+	if (!device)
+		return false;


Avoid a bunch of error scenarios by not allowing the function to proceed if the graphics context doesn't have a good device handle. This scenario occurs when the GPU resets.

dvijayak · 2024-05-08T12:20:34Z


 	if (r) {
-		void *texture = MTY_HashPopInt(cmn->ui_textures, id);
-		GFX_UI_API[cmn->api].destroy_texture(cmn->gfx_ui, &texture, device);
+		void *texture = MTY_HashGetInt(cmn->ui_textures, id);
+
+		if (rgba) {
+			void *texture_new = GFX_UI_API[cmn->api].create_texture(cmn->gfx_ui, device, rgba, width, height);

-		if (rgba)
-			texture = GFX_UI_API[cmn->api].create_texture(cmn->gfx_ui, device, rgba, width, height);
+			if (texture_new) {
+				GFX_UI_API[cmn->api].destroy_texture(cmn->gfx_ui, &texture, device);

-		if (texture)
-			MTY_HashSetInt(cmn->ui_textures, id, texture);
+				texture = texture_new;
+				MTY_HashSetInt(cmn->ui_textures, id, texture_new);
+			}
+
+		} else {
+			GFX_UI_API[cmn->api].destroy_texture(cmn->gfx_ui, &texture, device);
+			MTY_HashPopInt(cmn->ui_textures, id);
+		}

 		r = texture != NULL;
 	}


The old logic is to first destroy the existing texture for id and get rid of it from the textures hash, and then carry on with creating the new texture with the given pixel data rgba if it was provide, finally updating the hash node for this id with the new texture handle.

The problem with this is that when create_texture fails and/or returns a null texture handle, the existing perfectly-fine texture is removed. This failure can occur when the GPU resets, and because the entry is removed from the hash indiscriminately, MTY_WindowHasUITexture thereafter always returns false for the given id, which is incorrect behavior.

My fix is to replace the old/existing texture with the new one IFF the new texture was successfully created. After this change, even when the GPU resets, textures are rendered properly because the hash still maintains a reference to the original valid handle.

dvijayak · 2024-05-08T12:24:35Z

 MTY_ContextState MTY_WindowGetContextState(MTY_App *app, MTY_Window window)
 {
-	return MTY_CONTEXT_STATE_NORMAL;
+	MTY_ContextState state = MTY_CONTEXT_STATE_NORMAL;
+
+	struct window_common *cmn = mty_window_get_common(app, window);
+	if (cmn && cmn->api != MTY_GFX_NONE && cmn->device_changed) {
+		state = MTY_CONTEXT_STATE_NEW;
+		cmn->device_changed = false;
+	}
+
+	return state;
 }


When the device has changed or been reset, it is a mistake not to inform clients of matoya that textures need to be resubmitted.

dvijayak · 2024-05-08T12:25:49Z

All changes in this file implement a more robust GPU error handling. This portion concerns the main graphics context state.

dvijayak · 2024-05-08T12:26:05Z

All changes in this file implement a more robust GPU error handling. This portion concerns state pertaining to rendering UI.

Kodikuu

A high-level-ish review given that I don't actually have experience dealing with DirectX

Kodikuu · 2024-09-03T14:21:26Z

+			if (texture_new) {
+				GFX_UI_API[cmn->api].destroy_texture(cmn->gfx_ui, &texture, device);


Please check if texture is NULL before destroying it. An up-front check is ideal, even if the individual APIs may check too

Kodikuu · 2024-09-03T14:23:04Z

+	if (r)
+		MTY_Log("d3d11_ctx reinit ok.");
+	else
+		MTY_Log("d3d11_ctx reinit FAILED!");


No braces, and I'm not certain we need to log two times every time

Kodikuu · 2024-09-03T14:23:19Z

+
+int32_t mty_d3d11_error_handler_default(int32_t e1, int32_t e2, void *opaque)
+{
+	e1; e2; opaque;


We usually put each of these on a different line

dvijayak added 4 commits March 13, 2024 07:46

WIP

223e2b8

More WIP

b63b8f5

Better idea. WIP

8a3b5fa

Add stubs for non-windows platforms.

33f73bf

dvijayak added the 1. Ready for Impl. The scope of the work is all planned and specced out, and this can be worked on by the implementer label Apr 2, 2024

dvijayak added 2 commits April 3, 2024 08:13

Even better idea: use a tlocal queue. Changed error to be scoped to a…

be4ad33

… thread and not a window. Defined error codes so that matoya clients can action differently based on the error received while still masking the internal error details.

Cleanup

c014061

dvijayak force-pushed the gpu-error-handler branch from 3e74bc7 to c014061 Compare April 3, 2024 12:42

bmcnett reviewed Apr 4, 2024

View reviewed changes

Comment thread src/error.c Outdated

bmcnett reviewed Apr 4, 2024

View reviewed changes

Comment thread src/thread.c Outdated

bmcnett reviewed Apr 4, 2024

View reviewed changes

Comment thread src/error.c Outdated

bmcnett assigned dvijayak Apr 8, 2024

bmcnett added Low priority Feature review can be safely postponed indefinitely. and removed 1. Ready for Impl. The scope of the work is all planned and specced out, and this can be worked on by the implementer labels Apr 30, 2024

dvijayak added the 2. Ready for Review The implementation work is done, and this can be reviewed label May 1, 2024

Total simplification....let matoya fix itself.

b4f1412

dvijayak force-pushed the gpu-error-handler branch from d2450da to b4f1412 Compare May 3, 2024 18:40

dvijayak added 5 commits May 6, 2024 08:53

Added logs so its clear when d3d11 ctx is reiniting.

d2b88a9

Upon d3d11 device and ctx reinit due to fatal errors, ensure that the…

1af162e

… matoya client is informed that textures need to be resubmitted.

TEST: logs to help figure out what's going on

c7c3d66

MTY_WindowSetUITexture no longer indiscriminately deletes the existin…

3066a96

…g texture for the id. It only does so if the new texture successfully was created.

Merge remote-tracking branch 'parsec/stable' into gpu-error-handler

6473bde

dvijayak changed the title ~~Robust Error-Handling for Matoya Clients~~ Robust GPU error recovery May 8, 2024

dvijayak changed the title ~~Robust GPU error recovery~~ Robust GPU error recovery [Windows] May 8, 2024

dvijayak commented May 8, 2024

View reviewed changes

Kodikuu suggested changes Sep 3, 2024

View reviewed changes

Kodikuu added 1. Ready for Impl. The scope of the work is all planned and specced out, and this can be worked on by the implementer and removed 2. Ready for Review The implementation work is done, and this can be reviewed labels Sep 17, 2024

		if (texture_new) {
		GFX_UI_API[cmn->api].destroy_texture(cmn->gfx_ui, &texture, device);

Uh oh!

Conversation

dvijayak commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work In-Scope or Completed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmcnett commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kodikuu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dvijayak commented Apr 2, 2024 •

edited

Loading