Cycles: Improve error reporting of large textures on HIP #118239

Alaska · 2024-02-14T08:11:51+01:00

Alaska commented

2024-02-14 08:11:51 +01:00

HIP fails to allocate textures, typically when they are too large.
This commit lets the user know what might be causing the issue
rather than providing a confusing internal error message.

Thank you to Brecht for helping out with this.

HIP fails to allocate textures, typically when they are too large. This commit lets the user know what might be causing the issue rather than providing a confusing internal error message. Thank you to Brecht for helping out with this.

Alaska added the

Module

Render & Cycles

label 2024-02-14 08:11:51 +01:00

Alaska requested review from Brecht Van Lommel 2024-02-14 08:33:30 +01:00

Alaska requested review from Brian Savery (AMD) 2024-02-14 08:35:03 +01:00

Alaska commented

2024-02-14 09:17:48 +01:00

This is a desired feature as mentioned in #91571

Image texture size is limited, could detect this and resize or show user better error

This pull request handles the better error reporting. I am unsure how to handle the automatic resizing, and it's out of the scope of this pull request and not worth discussing here.

This is a desired feature as mentioned in #91571 > Image texture size is limited, could detect this and resize or show user better error This pull request handles the better error reporting. I am unsure how to handle the automatic resizing, and it's out of the scope of this pull request and not worth discussing here.

Brecht Van Lommel removed review request for Brian Savery (AMD) 2024-02-14 20:54:18 +01:00

Brecht Van Lommel requested changes 2024-02-14 20:59:29 +01:00

Dismissed

intern/cycles/device/hip/device_impl.cpp Outdated

						
				@ -651,1 +651,4 @@

				  /* hipDeviceAttributeMaxTexture2D... reports a supported texture size 1 larger than what's

				   * actually supported. */

				  int max_width = get_device_default_attribute(hipDeviceAttributeMaxTexture2DWidth, 16384) - 1;

Brecht Van Lommel commented

2024-02-14 20:59:05 +01:00

Did you find this out just by testing, is it documented somewhere, .. ?

I think it's not ideal to just subtract one:

If we hardcode this assumption, then if the driver gets fixed Cycles would reject valid texture resolutions.
It's not obvious that this is true across different platforms and driver versions now.
Subtracting 1 from the default value 16384 seems wrong.

Did you find this out just by testing, is it documented somewhere, .. ? I think it's not ideal to just subtract one: * If we hardcode this assumption, then if the driver gets fixed Cycles would reject valid texture resolutions. * It's not obvious that this is true across different platforms and driver versions now. * Subtracting 1 from the default value 16384 seems wrong.

Alaska commented

2024-02-14 23:53:29 +01:00

I found it out by testing. I'm not sure if it's documented somewhere.

The issue of being off by one has been around for a while (atleast 3 years). In this comment, it is pointed out that HIP has a texture size limit of 16384, yet when the user tried a texture of size 16384, they still had errors. This is my experience as well.

I found it out by testing. I'm not sure if it's documented somewhere. The issue of being off by one has been around for a while (atleast 3 years). In [this comment](https://projects.blender.org/blender/blender/issues/93109#issuecomment-175926), it is pointed out that HIP has a texture size limit of 16384, yet when the user [tried a texture of size 16384](https://projects.blender.org/blender/blender/issues/93109#issuecomment-175929), they still had errors. This is my experience as well.

Brecht Van Lommel commented

2024-02-15 00:20:04 +01:00

In that comment, it says that width 16384 failed while 8192 worked. So it's off by one in the power, where it's 2^13 = 8192 instead of 2^14 = 16384. Not 16383 instead of 16384.

What I suspect is that it's like OpenGL, and there isn't actually a single maximum because it depends on the precision, channels and dimensions. It might support a 1 channel byte image at that resolution, but not a 4 channel float image.

In that comment, it says that width 16384 failed while 8192 worked. So it's off by one in the power, where it's 2^13 = 8192 instead of 2^14 = 16384. Not 16383 instead of 16384. What I suspect is that it's like OpenGL, and there isn't actually a single maximum because it depends on the precision, channels and dimensions. It might support a 1 channel byte image at that resolution, but not a 4 channel float image.

Brecht Van Lommel commented

2024-02-15 00:23:30 +01:00

I think a better solution here would be to check for errors returned by hipTexObjectCreate, hipDrvMemcpy2DUnaligned, hipDrvMemcpy3D and hipArray3DCreate. Maybe they already tell us that the resolution is not supported, or maybe it's a more vague error where we can hint to the user what the possible cause is.

That way we don't stop a texture from being created when it is supported.

I think a better solution here would be to check for errors returned by `hipTexObjectCreate`, `hipDrvMemcpy2DUnaligned`, `hipDrvMemcpy3D` and `hipArray3DCreate`. Maybe they already tell us that the resolution is not supported, or maybe it's a more vague error where we can hint to the user what the possible cause is. That way we don't stop a texture from being created when it is supported.

Alaska commented

2024-02-15 02:42:16 +01:00

Looking at some API docs, it doesn't seem like we get much about the error from these functions. Take for example, hipTexObjectCreate. It either returns hipSuccess = 0 (Texture creation was success) or hipErrorInvalidVale = 1 (One of the values is unacceptable)

Some of the other functions (E.G. hipDrvMemcpy2DUnaligned, hipDrvMemcpy3D) can return different values, but nothing particularly useful other than success, or some sort of error due to an invalid value.

At the moment I've added an error message for hipTexObjectCreate. I just wanted to check with you if this is the kind of thing you wanted before I continued on with the other parts. Are there any changes you would like to wording? Possibly include reports on the resolution being used or the limits?

It seems many different texture types are handled in tex_alloc. 1D, 2D, and 3D. I just wanted to check, where do all these shapes come from? This is just so I create scenes that have errors and test them.

I'm unsure where 1D comes from. LUTs from Cycles code? Can a user create a 1D texture?
2D is primarily textures.
3D seems to be volumes. But some/all NANOVDB stuff seems to be handled a bit differently. Clarification on this would be useful.

Looking at some API docs, it doesn't seem like we get much about the error from these functions. Take for example, [hipTexObjectCreate](https://cgmb-hip.readthedocs.io/en/sphinx/api/index.html#c.hipTexObjectCreate). It either returns `hipSuccess = 0 (Texture creation was success)` or `hipErrorInvalidVale = 1 (One of the values is unacceptable)` Some of the other functions (E.G. `hipDrvMemcpy2DUnaligned`, `hipDrvMemcpy3D`) can return different values, but nothing particularly useful other than success, or some sort of error due to an invalid value. --- At the moment I've added an error message for `hipTexObjectCreate`. I just wanted to check with you if this is the kind of thing you wanted before I continued on with the other parts. Are there any changes you would like to wording? Possibly include reports on the resolution being used or the limits? It seems many different texture types are handled in `tex_alloc`. 1D, 2D, and 3D. I just wanted to check, where do all these shapes come from? This is just so I create scenes that have errors and test them. - I'm unsure where 1D comes from. LUTs from Cycles code? Can a user create a 1D texture? - 2D is primarily textures. - 3D seems to be volumes. But some/all NANOVDB stuff seems to be handled a bit differently. Clarification on this would be useful.

Alaska changed title from ~~Cycles: Improve error reporting of large textures on HIP~~ to WIP: Cycles: Improve error reporting of large textures on HIP

2024-02-15 02:42:29 +01:00

Brecht Van Lommel requested changes 2024-02-26 15:52:27 +01:00

Dismissed

Brecht Van Lommel left a comment

I don't think 1D textures are used currently, but if we do use them I think it would be fine to just call them "texture" still. For 3D you could use "volume texture".

intern/cycles/device/hip/device_impl.cpp Outdated

						
				@ -857,2 +857,3 @@

				    hip_assert(hipTexObjectCreate(&cmem->texobject, &resDesc, &texDesc, NULL));

				      /* Returns 0 if successful. Returns other values depending on the error. */

				    if (hipTexObjectCreate(&cmem->texobject, &resDesc, &texDesc, NULL)) {

Brecht Van Lommel commented

2024-02-26 15:50:33 +01:00

This type of change looks fine. Would prefer != 0 for clarity, and string_printf is not needed if it's just a plain string.

I would suggest for the message.

Failed to create texture. Maximum GPU texture size or available GPU memory was likely exceeded.

This type of change looks fine. Would prefer != 0 for clarity, and `string_printf` is not needed if it's just a plain string. I would suggest for the message. > Failed to create texture. Maximum GPU texture size or available GPU memory was likely exceeded.

Alaska marked this conversation as resolved

Alaska force-pushed better-hip-texture-warning from 81138cecfa to fe8e27ad17

2024-02-27 03:13:41 +01:00

Compare

Alaska added 1 commit 2024-02-27 03:19:47 +01:00

68c3906018 Update error message handling

Alaska commented

2024-02-27 03:25:44 +01:00

At the moment I only have an error message for hipTexObjectCreate because in my testing, this is the only place that reports errors when the texture is too large.

We could try to implement a more descriptive error message with a code change like this: c4ef916438

What are your thoughts?

At the moment I only have an error message for `hipTexObjectCreate` because in my testing, this is the only place that reports errors when the texture is too large. We could try to implement a more descriptive error message with a code change like this: https://projects.blender.org/Alaska/blender/commit/c4ef916438e67393047ade2fc95007452fa4d421 What are your thoughts?

Alaska changed title from ~~WIP: Cycles: Improve error reporting of large textures on HIP~~ to Cycles: Improve error reporting of large textures on HIP

2024-02-27 03:26:11 +01:00

Brecht Van Lommel commented

2024-02-27 10:07:23 +01:00

I prefer to keep it simple and not do that, especially since the information is unreliable.