Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly write-protect segments #673

Closed
wants to merge 1 commit into from
Closed

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Aug 28, 2024

Extracted from #606. Before this, we would still render even if the segments buffer was being overflowed.

I believe that this was missed in #537

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 28, 2024

@94bryanr, does this resolve the issue you were seeing?

@94bryanr
Copy link

94bryanr commented Aug 29, 2024

Unfortunately, I still see the visual artifact of nothing rendered below a certain line when the resolution is increased. This is with the new config DebugLayers set to none and MemoryHints set to Performance.

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 29, 2024

We probably need code which reproduces this issue to be able to help

@dominikh
Copy link

dominikh commented Aug 29, 2024

Just editing the simple example to fill a shape of a large size, say 8000x8000, will demonstrate the problem when rendering to a window roughly larger than 4096x4096 (it really depends on the area). At some point, new tiles stop being rendered. Though if you're unlucky, you get a compute shader timeout instead.

If I had to guess I'd say that the limit is 256 x 256 tiles.

@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 2, 2024

Just done some experiments, and have been able to reproduce, and isolated this to us only supporting 256 bins. I've opened #680 to track this.

However, the issue @94bryanr seems to be a separate issue again, interestingly.
We need a minimal reproduction for that issue as well. I have explained why it looks like a different issue in #680.

@DJMcNab DJMcNab added this to the Vello 0.3 release milestone Sep 23, 2024
@DJMcNab DJMcNab requested a review from raphlinus September 23, 2024 15:24
@raphlinus
Copy link
Contributor

There is a reason we don't have a check in path_tiling, though it's possible I missed something. Essentially, the number of tiles written in path_tiling should always be less than or equal to the count determined in the path_count stage. That, in turn, is checked against the buffer size in coarse, and then downstream of that, path_tiling_setup zeroes out the dispatch of path_tiling, so effectively it doesn't run.

If my analysis is correct, then we probably should close this without merging, but I am very open to an argument that something might still go wrong that this check would mitigate.

@DJMcNab DJMcNab removed this from the Vello 0.3 release milestone Sep 25, 2024
@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 30, 2024

This was discussed in #gpu > Segments and seg_counts buffer sizes. We decided that follow-up work is to make the segments and seg_counts buffer use the same capacity (even though they have different bumps).

I'll do that in a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants