Skip to content

cellAtracXdec: replace shared_mutexes/cond_variables with lv2 mutexes/conds#16549

Merged
elad335 merged 3 commits into
RPCS3:masterfrom
capriots:cellAtracXdec
Jan 14, 2025
Merged

cellAtracXdec: replace shared_mutexes/cond_variables with lv2 mutexes/conds#16549
elad335 merged 3 commits into
RPCS3:masterfrom
capriots:cellAtracXdec

Conversation

@capriots

Copy link
Copy Markdown
Contributor

Other PPU threads were apparently being blocked from executing while waiting on shared_mutexes/cond_variables.

Should fix #16538

switch (savestate)
{
case atracxdec_state::waiting_for_cmd_mutex_lock: break;
case atracxdec_state::waiting_for_cmd_cond_wait: goto wait_for_cmd;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use goto in rpcs3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve it in this case,

I suggest mentioning where the label literally is:

Suggested change
case atracxdec_state::waiting_for_cmd_cond_wait: goto wait_for_cmd;
case atracxdec_state::waiting_for_cmd_cond_wait: goto wait_for_cmd_line_305;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because you approve it personally doesn't make it ok

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He would have to replace it with a huge switch with fallthroughs for each case otherwise, this is not a refactoring pr but a bugfix so I rather let it be fixed first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion gotos is the most intuitive way of handling savestates here. We're simply jumping back to where the thread was interrupted, there aren't any crazy control flow gymnastics.
But if no gotos is a hard requirement, then I'll replace them with a large switch. I'd have to replace the command type switch with an if-else chain though since you can't put cases of one switch in the middle of another.
I disagree on putting the line number on the labels, however. You would have to update them every time the line number changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not a line number, but I do want unique distinguishing on labels. Perhaps:

Suggested change
case atracxdec_state::waiting_for_cmd_cond_wait: goto wait_for_cmd;
case atracxdec_state::waiting_for_cmd_cond_wait: goto label2_wait_for_cmd_stage;

Putting the prefix "label" and relative order number between other labels in the function.
I do want to standardize it for the use of savestates.

@elad335
elad335 merged commit ae670c3 into RPCS3:master Jan 14, 2025
@elad335 elad335 added the Bugfix label Jan 14, 2025
@capriots
capriots deleted the cellAtracXdec branch January 14, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mapping Start in PS move (mouse) while combo is active causes game to freeze

3 participants