GPU: Make SDL_SetGPUBufferName and SDL_SetGPUTextureName thread safe

[thatcosmonaut]

Resolves #11927

[kg]

For consistency with how these naming APIs work in some other places you probably want to strdup the name, since it could be a temporary stack buffer being passed in. Or document that it needs to stay alive indefinitely, but that seems worse.

You should be able to free the name once you actually pass it through to vulkan though.

[thatcosmonaut]

Good call.

[slouken]

This code looks fine, but are we guaranteed that the main thread is a safe thread to call this on? Is there a possibility of interacting with other graphics calls being made off the main thread?

[thatcosmonaut]

Skimmed the Vulkan spec again and saw:

Host access to pNameInfo->objectHandle must be externally synchronized

This might be more problematic than I thought. I'm not sure that we have any way to guarantee this for the client.

[slouken]

Skimmed the Vulkan spec again and saw:

Host access to pNameInfo->objectHandle must be externally synchronized

This might be more problematic than I thought. I'm not sure that we have any way to guarantee this for the client.

What does externally synchronized mean? Just that it can only be done by one thread at a time?

[slouken]

@danginsburg, do you have any insight here?

[slime73]

Is the thread safety of these mainly hard to reason about because they can be called at any point after creating the resource? Would making resource names be part of buffer and texture creation SDL_Properties instead solve that? Although I suppose it might be too late to do that now..

[thatcosmonaut]

What does externally synchronized mean? Just that it can only be done by one thread at a time?

It means the handle should only be accessed by one thread at a time.

[thatcosmonaut]

Is the thread safety of these mainly hard to reason about because they can be called at any point after creating the resource? Would making resource names be part of buffer and texture creation SDL_Properties instead solve that? Although I suppose it might be too late to do that now..

Yeah I think that would be a good solution. As long as the thread safety issue on the existing functions is clearly documented (which it currently is) we could just add this as a creation property and sidestep the safety issues that way.

[slouken]

Is the thread safety of these mainly hard to reason about because they can be called at any point after creating the resource? Would making resource names be part of buffer and texture creation SDL_Properties instead solve that? Although I suppose it might be too late to do that now..

Yeah I think that would be a good solution. As long as the thread safety issue on the existing functions is clearly documented (which it currently is) we could just add this as a creation property and sidestep the safety issues that way.

Agreed. Having the name as a creation property makes a lot of sense.

[kg]

Does passing the name at creation time actually fix this? That's what I was doing when I encountered this bug, because I wasn't guarding the name setting operation behind a lock.

It's safe to create textures from any thread (based on my reading of the vulkan and d3d12 docs) but in vulkan it's not safe to assign a name to the texture even if it's done immediately after from the same thread. It seems like there's some internal command buffer these naming operations go into and you have to externally synchronize around it.

EDIT: That is, the code looked like this:

And guarding it with the same lock I guard rendering operations with made the validator warnings go away, so it seems to dislike names being set while the device is otherwise in use.

[thatcosmonaut]

Does passing the name at creation time actually fix this? That's what I was doing when I encountered this bug, because I wasn't guarding the name setting operation behind a lock.

It should, because if we call the Vulkan debug name function before returning from the creation function there's no way for the client to even have a handle that they would need to synchronize.

[kg]

In my repro case the texture wasn't externally visible yet or being used. The synchronization hazard is on the device I think.

[thatcosmonaut]

Closing this in favor of #11946