Fix gpu driver crash caused by pixel history #3729
Conversation
baldurk
left a comment
baldurk
left a comment
There was a problem hiding this comment.
I'm not entirely sure I follow the exact problem - I understand there might be a language barrier, but are you able to explain in more detail what happens? Or could you share a reproducible test case that shows the problem happening?
I think something should be fixed, but I've left one comment and it would be good to understand better to make sure the fix isn't better placed elsewhere perhaps.
In general both subpasses and secondary command buffers are mostly rarely used and there are a lot of problems with getting them to work in pixel history, so results likely will be poor, but we should not crash.
|
|
||
| if(m_ActionCallback && m_ActionCallback->ForceLoadRPs()) | ||
| { | ||
| unwrappedInheritInfo.subpass = 0; |
There was a problem hiding this comment.
If I understand right I think this should happen below, after this value is used to look up the right element in loadFBs and loadRPs?
Without knowing exactly what goes wrong it's hard to say.
There was a problem hiding this comment.
when calling Pixel History, m_ActionCallback is called too in vkBeginCommandBuffer; however, in the case when vkCmdExecuteCommands is executed in 1-indexed subpass, the subpass passed to driver is 1, but the actual subpass
is 0 (because renderdoc change original rp to force load rp); this is the real case in the game ‘Star Resonance’ and causes
crash in our driver(innosilicon gpu).
There was a problem hiding this comment.
API call goes like this:
vkCmdBeginRenderpass
......
vkCmdExecuteCommands
......
vkCmdNextSubpass
vkCmdExecuteCommands (crash)
......
driver may need the right subpass info
There was a problem hiding this comment.
OK if I understand right then, the change I mentioned should be made to ensure the right load RP is selected but otherwise this sounds good.
There was a problem hiding this comment.
OK if I understand right then, the change I mentioned should be made to ensure the right load RP is selected but otherwise this sounds good.
Yes, choose the subpass of the load pr instead of the subpass of the original pr.
There was a problem hiding this comment.
I realise there may be a language barrier here, so I will repeat one final time what I said above - if this feedback is not addressed and the code isn't fixed then this PR will be closed without being merged.
There was a problem hiding this comment.
I realise there may be a language barrier here, so I will repeat one final time what I said above - if this feedback is not addressed and the code isn't fixed then this PR will be closed without being merged.
Sorry, I misunderstood your meaning. I'll upload the modified code later.
There was a problem hiding this comment.
I have uploaded the new patch
There was a problem hiding this comment.
Thank you for making that fix, and for tidying up the branch ready to merge.
Force load renderpass contains only one subpass, if not set subpass, the subpass passed to gpu driver mismatch the actual subpass, and then causes crash.
nwpuCfy
force-pushed
the
fix/pixel-history-crash
branch
from
f5244c6 to
3068463
Compare
2 participants
Force load renderpass contains only one subpass, if not set subpass, the subpass passed to gpu driver mismatch the actual subpass, and then causes crash.