Added sub-case for basic test where the provided pitches are zero #2440
Conversation
bashbaug
left a comment
bashbaug
left a comment
There was a problem hiding this comment.
This adds additional coverage for clEnqueueCopyBufferRect.
Should we add additional coverage for clEnqueueReadBufferRect and clEnqueueWriteBufferRect also? If this is something we want, it's fine to add it in a separate PR.
| if ((err = copy_region(src, soffset, sregion, dst, doffset, | ||
| dregion))) |
There was a problem hiding this comment.
I don't understand this part of the change. Shouldn't this continue to call test_functions.copy instead of copy_region directly?
Why are extra {curly braces} needed?
| int copy_region(size_t src, size_t soffset[3], size_t sregion[3], size_t dst, | ||
| size_t doffset[3], size_t dregion[3]) | ||
| { | ||
| bool comp_pitch = (genrand_int32(mt) % 2) != 0 ? true : false; |
There was a problem hiding this comment.
Nitpick: I don't know what comp_pitch is supposed to mean. Could we spell "comp" out completely to add clarity?
Additionally, with this changes we either always seem to compute all four pitches, or we pass zero for all four pitches. Should we test different combinations of zero and non-zero pitches?
Should I create separate issue or just comment to #2279 |
I've created new PR #2515 without new issue |
|
We're running into some failures with this and would like some more time to check the verification code. |
| (unsigned)src, (unsigned)dst); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The calculation for spitch, sslice, dpitch and dslice in lines 209 to 213 has to be changed as well. The specs says:
If src_row_pitch is 0, src_row_pitch is computed as region[0].
If src_slice_pitch is 0, src_slice_pitch is computed as region[1] × src_row_pitch.
If dst_row_pitch is 0, dst_row_pitch is computed as region[0].
If dst_slice_pitch is 0, dst_slice_pitch is computed as region[1] × dst_row_pitch.
So when the row or slice pitches are 0, the region values should be used.
| size_t spitch = (src_row_pitch == 0) ? sregion[0] : src_row_pitch; | |
| size_t sslice = (src_slice_pitch == 0) ? sregion[1] * spitch : src_slice_pitch; | |
| size_t dpitch = (dst_row_pitch == 0) ? dregion[0] : dst_row_pitch; | |
| size_t dslice = (dst_slice_pitch == 0) ? dregion[1] * dpitch : dst_slice_pitch; |
There was a problem hiding this comment.
So when the row or slice pitches are 0, the region values should be used.
To me this sounds like a specification issue. If anything other than the actual source/destination buffer width/height is used for the row/slice pitches, we will no longer be copying a rectangular (or cubic) region. Instead, the copy will become skewed because each row will start at an incorrect offset. Am I missing something ?
4 participants
Related to #2279