Tales of Zestria and Tales of Berseria have broken colors with nouveau
orbea opened this issue · 21 comments
OS: Slackware64-current
mesa-2018.02.10_831fb29_master-x86_64-1_git
wine-2.21-x86_64-1_SBo (With staging + d3d9-nine patches)
linux-4.4.18
xorg-server-1.19.6-x86_64-2 (Using DRI3 + modesetting)
When playing the games Tales of Zestiria and Tales of Berseria which share the same engine with wine-d3d9 and nouveau the colors are really wrong. The issue is not reproducible when using just opengl and no d3d9-nine. This includes when replaying the trace made with the msvc version of apitrace and wine-d3d9-nine using wine opengl.
I am having difficulties getting apitrace to play nice with Tales of Berseria, so this issue for now is only focusing on Tales of Zestiria which does not have this issue.
OpenGL
D3D9-nine
Windows trace - http://ks392457.kimsufi.com/orbea/stuff/trace/Tales%20of%20Zestiria.trace.xz
Edt: new link - http://slackless.raccoons.tech/trace/Tales%20of%20Zestiria.trace.xz
Replaying the trace with wine-d3d9-nine and the llvmpipe still shows the issue. Neither game will boot with the llvmpipe though.
I found that older mesa versions were unaffected and bisected this issue.
4d6fab245eec3880e2a59424a579851f44857ce8 is the first bad commit
commit 4d6fab245eec3880e2a59424a579851f44857ce8
Author: Marek Olšák <marek.olsak@amd.com>
Date: Sat Jun 10 23:12:25 2017 +0200
cso: don't track the number of sampler states bound
This removes 2 loops from hot codepaths and adds 1 loop to a rare codepath
(restore_sampler_states), and makes sanitize_hash() slightly worse.
Sampler states, when bound, are not unbound for draw calls that don't need
them. That's OK, because bound sampler states don't add any overhead.
This results in lower CPU overhead in most cases.
Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
:040000 040000 e6a884cf356a84d36ef39710ce3a894292dab562 4ea5cbc8867e4df022b9d555f1ca6ccc43ec7333 M src
https://github.com/mesa3d/mesa/commit/4d6fab245eec3880e2a59424a579851f44857ce8
Additionally this likely hits many more games as I tried a third game, Atelier Sophie and it was broken in a more subtle way. Some textures viewed from certain angles would appear black. All three games I tried regressed in the same commit.
Reverting that commit and then resolving the one conflict seems to work around the issues.
diff --git a/src/gallium/auxiliary/cso_cache/cso_context.c b/src/gallium/auxiliary/cso_cache/cso_context.c
index 1b5d4b5598..57bf7c8fab 100644
--- a/src/gallium/auxiliary/cso_cache/cso_context.c
+++ b/src/gallium/auxiliary/cso_cache/cso_context.c
@@ -57,6 +57,7 @@ struct sampler_info
{
struct cso_sampler *cso_samplers[PIPE_MAX_SAMPLERS];
void *samplers[PIPE_MAX_SAMPLERS];
+ unsigned nr_samplers;
};
@@ -82,11 +83,6 @@ struct cso_context {
struct sampler_info fragment_samplers_saved;
struct sampler_info samplers[PIPE_SHADER_TYPES];
- /* Temporary number until cso_single_sampler_done is called.
- * It tracks the highest sampler seen in cso_single_sampler.
- */
- int max_sampler_seen;
-
struct pipe_vertex_buffer aux_vertex_buffer_current;
struct pipe_vertex_buffer aux_vertex_buffer_saved;
unsigned aux_vertex_buffer_index;
@@ -248,7 +244,7 @@ sanitize_hash(struct cso_hash *hash, enum cso_cache_type type,
* table, to prevent them from being deleted
*/
for (i = 0; i < PIPE_SHADER_TYPES; i++) {
- for (j = 0; j < PIPE_MAX_SAMPLERS; j++) {
+ for (j = 0; j < ctx->samplers[i].nr_samplers; j++) {
struct cso_sampler *sampler = ctx->samplers[i].cso_samplers[j];
if (sampler && cso_hash_take(hash, sampler->hash_key))
@@ -342,7 +338,6 @@ cso_create_context(struct pipe_context *pipe, unsigned u_vbuf_flags)
ctx->has_streamout = TRUE;
}
- ctx->max_sampler_seen = -1;
return ctx;
out:
@@ -1244,7 +1239,9 @@ cso_single_sampler(struct cso_context *ctx, enum pipe_shader_type shader_stage,
ctx->samplers[shader_stage].cso_samplers[idx] = cso;
ctx->samplers[shader_stage].samplers[idx] = cso->data;
- ctx->max_sampler_seen = MAX2(ctx->max_sampler_seen, (int)idx);
+ } else {
+ ctx->samplers[shader_stage].cso_samplers[idx] = NULL;
+ ctx->samplers[shader_stage].samplers[idx] = NULL;
}
}
@@ -1257,14 +1254,19 @@ cso_single_sampler_done(struct cso_context *ctx,
enum pipe_shader_type shader_stage)
{
struct sampler_info *info = &ctx->samplers[shader_stage];
+ const unsigned old_nr_samplers = info->nr_samplers;
+ unsigned i;
- if (ctx->max_sampler_seen == -1)
- return;
+ /* find highest non-null sampler */
+ for (i = PIPE_MAX_SAMPLERS; i > 0; i--) {
+ if (info->samplers[i - 1] != NULL)
+ break;
+ }
+ info->nr_samplers = i;
ctx->pipe->bind_sampler_states(ctx->pipe, shader_stage, 0,
- ctx->max_sampler_seen + 1,
+ MAX2(old_nr_samplers, info->nr_samplers),
info->samplers);
- ctx->max_sampler_seen = -1;
}
@@ -1291,9 +1293,11 @@ cso_save_fragment_samplers(struct cso_context *ctx)
struct sampler_info *info = &ctx->samplers[PIPE_SHADER_FRAGMENT];
struct sampler_info *saved = &ctx->fragment_samplers_saved;
- memcpy(saved->cso_samplers, info->cso_samplers,
- sizeof(info->cso_samplers));
- memcpy(saved->samplers, info->samplers, sizeof(info->samplers));
+ saved->nr_samplers = info->nr_samplers;
+ memcpy(saved->cso_samplers, info->cso_samplers, info->nr_samplers *
+ sizeof(*info->cso_samplers));
+ memcpy(saved->samplers, info->samplers, info->nr_samplers *
+ sizeof(*info->samplers));
}
@@ -1302,16 +1306,18 @@ cso_restore_fragment_samplers(struct cso_context *ctx)
{
struct sampler_info *info = &ctx->samplers[PIPE_SHADER_FRAGMENT];
struct sampler_info *saved = &ctx->fragment_samplers_saved;
+ int delta = (int)info->nr_samplers - saved->nr_samplers;
memcpy(info->cso_samplers, saved->cso_samplers,
- sizeof(info->cso_samplers));
- memcpy(info->samplers, saved->samplers, sizeof(info->samplers));
-
- for (int i = PIPE_MAX_SAMPLERS - 1; i >= 0; i--) {
- if (info->samplers[i]) {
- ctx->max_sampler_seen = i;
- break;
- }
+ saved->nr_samplers * sizeof(*info->cso_samplers));
+ memcpy(info->samplers, saved->samplers,
+ saved->nr_samplers * sizeof(*info->samplers));
+
+ if (delta > 0) {
+ memset(&info->cso_samplers[saved->nr_samplers], 0,
+ delta * sizeof(*info->cso_samplers));
+ memset(&info->samplers[saved->nr_samplers], 0,
+ delta * sizeof(*info->samplers));
}
cso_single_sampler_done(ctx, PIPE_SHADER_FRAGMENT);
The trace works fine on radeonsi. Likely that nouveau needs special care.
Yea, seems more than one person confirmed that the traces are fine with amd and its only nouveau. That said I am highly interested in seeing this fixed properly so if there is any more information I can provide to make it easier to fix please let me know. Unfortunately my coding ability is far too immature to really understand mesa's code base enough to fix this myself...
Just want to confirm that there are no issues with R600 driver and Mesa3D 7.3.0 .
The commit in question seems to be present in mesa 7.2.0 so my version has it.
@iiv3 sarnex reported that radeonsi and r600 are not affected in #dri-devel @ freenode when I asked if someone else could try the trace. :)
So I think two people have confirmed its not present with radeonsi and one with r600.
Fixes the problem for me:
diff --git a/src/gallium/auxiliary/cso_cache/cso_context.c b/src/gallium/auxiliary/cso_cache/cso_context.c
index dd9821e828..47944df1cd 100644
--- a/src/gallium/auxiliary/cso_cache/cso_context.c
+++ b/src/gallium/auxiliary/cso_cache/cso_context.c
@@ -1261,6 +1261,12 @@ cso_single_sampler_done(struct cso_context *ctx,
if (ctx->max_sampler_seen == -1)
return;
+ for (int i = PIPE_MAX_SAMPLERS - 1; i >= 0; i--) {
+ if (info->samplers[i]) {
+ ctx->max_sampler_seen = i;
+ break;
+ }
+ }
ctx->pipe->bind_sampler_states(ctx->pipe, shader_stage, 0,
ctx->max_sampler_seen + 1,
info->samplers);
That means nouveau accesses samplers that aren't bound, but have been bound in the past. As the other drivers to don't have this issue, my guess is that the shader compiler is broken ?
Fwiw that patch also works here.
Related conversation from #dri-devel.
16:31 <imirkin_> mareko: https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nvc0/nvc0_state.c#n445
16:31 <imirkin_> do you agree with the second loop which wipes out old samplers, or not?
16:48 <mareko> imirkin_: it's wrong
16:49 <mareko> imirkin_: the function should only update slots as specified by the start/count parameters
16:50 <mareko> so, the nvc0->num_samplers variable is BS
16:51 <imirkin_> ok. that's not how it used to work. sigh.
16:51 <mareko> set_sampler_views is wrong too
16:51 <imirkin_> you sure are redefining a lot of gallium api's lately :(
16:52 <mareko> lately? :)
16:52 <mareko> it's been a while
16:52 <imirkin_> well, people are noticing now.
16:52 <bnieuwenhuizen> because of the 18.0 rc's?
16:53 <imirkin_> wonder how many other drivers are subtly broken...
16:54 <imirkin_> looks like freedreno's ok
16:55 <mareko> bind_sampler_states has had start_slot since 755d788fe24732127af0997f26e2fb61e5aa6173 Brian Paul <brianp@vmware.com>, Thu Oct 3 14:05:26 2013
16:55 <mareko> so not "lately"
16:56 <mareko> it was right for me to make the assumption about drivers that I made last year
16:56 <imirkin_> looks like swr is also broken by this
16:56 <imirkin_> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/swr/swr_state.cpp#n287
16:57 <imirkin_> mareko: just like it'd be right for me to revert that series until it's reworked to not break things...
16:57 <imirkin_> but i probably won't do that
16:59 <mareko> I can't agree with you, and if swr was broken, I'm sure people would notice, assuming swr devs run piglit
17:00 <imirkin_> ok.
17:03 <mareko> stuff gets accidentally broken all the time, so what can we do correct that? I guess we could just revert and keep the mesa history at year 2010 to make sure nothing is broken and nothing will ever break
17:03 <imirkin_> i don't mind stuff getting broken - like you say, it happens, and quite a bit
17:05 <imirkin_> but it seems like there's not a lot of checking going on when changing APIs around with what other drivers do
17:05 <imirkin_> esp for something as undocumented and untested as messing about with start and nr with set_sampler_states & co
17:05 <imirkin_> and then the attitude becomes "well, that driver is wrong", not "my interpretation of the api was wrong"
Are they going to fix it ? It would help if you open a ticket one the official freedesktop issue tracker as it's not a nine bug. As you already have bisected it and have a "fix" it should be easy to find a proper solution. You can also reference this ticket and the apitrace that shows the problem.
I'm not sure. I suspect marek doesn't plan to fix it and imirkin suggested he might fix it, but I haven't heard anything since and as busy as he is I'm wouldn't be surprised if other things came up.
If no one is going to fix it, maybe marek's patches should be reverted until they no longer break things...
Do you have this issue filled as Mesa3D bug on their freedesktop tracker?
Be sure to do it, otherwise they can slip by...
Put all relevant info you've discovered so far, attach your revert, give a link to this issue and post the bugzilla url back here.
Also, somebody should fix the softpipe render too, it's not nouveau only bug.
What is the status of this issue?
Last I heard the other day it was on imirkin's list to fix, but he doesn't have much time so it could take some time...
One more month has passed.
Has this bug been fixed already?
I checked and llvmpipe seems to play it correctly. I do not see any commits in Mesa3D that might be relevant to it. If the bug is still present, it is nouveau only.
If it hasn't been fixed yet, I compel you to fill a report at bugs.freedesktop.org .
If you do not want to fill bugreport, I will do it instead. It's not optimal as I could not test a fix by myself, but the problem is known and it has been overlooked for way too long.
I can no longer test this with the llvmpipe...
I am not sure why, but Tales of Zestiria no longer starts here and apitrace gives me this with the old trace.
error: unsupported trace format version 6
Tales of Berseria is still broken with nouveau, but I can not trace this game due apitrace issue apitrace/apitrace#557 and the game will not boot with the llvmpipe.
To be blunt, I don't really have the energy to make another issue report on the mesa issue tracker which will just be ignored. If you want to remind the mesa devs I would suggest doing it in irc @ #dri-devel.
Actually, I was using the wrong wine install, but regardless I can't get apitrace to create a new trace...
There is already bugreport at the mesa tracker: https://bugs.freedesktop.org/show_bug.cgi?id=106577
As I have written there, llvmpipe has been fixed and the trace plays perfectly. The only buggy driver is Nouveau. The Nouveau developers are aware of the issue but so far have not even tried to fix it.
@orbea, the quoted apitrace error simply means that you are using older apitrace. Get the latest msvc binary build from https://apitrace.github.io/#download .
Thanks, I was mistakenly using the mingw apitrace version. I can now replay the trace with nouveau, but trying to do so with the llvmpipe just floods stdout with shader and apitrace output without rendering anything. I guess it doesn't really matter since the games won't boot with llvmpipe and would be far too slow with it anyways...