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
[mtl 2D texel buffer view #2340
Conversation
//Note: we rely on SPIRV-Cross to use the proper 2D texel indexing here | ||
let texel_count = (end_rough - start) * 8 / format_desc.bits as u64; | ||
let col_count = cmp::min(texel_count, self.private_caps.max_texture_size); | ||
let row_count = cmp::max(1, texel_count / self.private_caps.max_texture_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised row_count
doesn't depend on col_count
at all here, how does this work when either dimension has been clamped to max_texture_size
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the stride account for this somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if col_count < max_texture_size
then row_count
is guaranteed to be 1
anyway. Otherwise, it is equal to max_texture_size
and thus we don't need it in the row_count
computation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks! Should we assert that row_count
is less than self.private_caps.max_texture_size
too then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely could, but it's a part of a bigger picture: do we assert/verify/validate on all unexpected state? We already report the limits for the users to obey, and we have Metal validation layer underneath, so it's not clear as to what should be out tactic here.
Anyhow, the PR doesn't need to be blocked on that :)
bors r=grovesNL |
2340: [mtl 2D texel buffer view r=grovesNL a=kvark Makes our buffer views bigger, relying on KhronosGroup/SPIRV-Cross#664 PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
2341: [mtl] hotfix for the buffer alignment r=kvark a=kvark follow-up to #2340 PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: - [ ] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Makes our buffer views bigger, relying on KhronosGroup/SPIRV-Cross#664
PR checklist:
make
succeeds (on *nix)make reftests
succeedsrustfmt
run on changed code