Skip to content

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Sep 1, 2025

urBindlessImagesImageCopyExp documents its argument that describe copy region as using bytes-pixel-pixel format, while we were mistakenly passing pixel-pixel-pixel format in there.

This PR adjusts that. It is an NFC change for CUDA & HIP adapters, because for them it only changes the place where pixel to byte conversion happens, but the change is tricker for L0 adapter. In L0, there are APIs which expect pixel-pixel-pixel format and there are APIs which accept bytes-pixel-pixel format, so some extra care is needed there.

// ur_rect_offset_t and ur_rect_region_t describe their first component as
// bytes, whilst ze_image_region_t uses pixels.
//
// However, the getImageRegionHelper above is used for both bindless and regular
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for UR reviewers: this inconsistency in the spec was reported as URT-969

@AlexeySachkov AlexeySachkov marked this pull request as ready for review September 3, 2025 20:24
@AlexeySachkov AlexeySachkov requested review from a team as code owners September 3, 2025 20:24
@cperkinsintel
Copy link
Contributor

Was this not covered by any test? Could we add one now?

@AlexeySachkov
Copy link
Contributor Author

AlexeySachkov commented Sep 4, 2025

Was this not covered by any test?

Yes and no. We do have tests for copies, but a lot of them REQUIRES: cuda and have never been run anywhere else. I reported HIP failures in #19957 and I'm trying to enable those tests on L0 in #19819. However, the latter requires more fixes and this PR is split out of #19819 to reduce the diff there.

UPD: to be precise, out of 6 bindless image copy tests, 4 have REQUIRES: cuda, 1 other is XFAILed on L0 due to missing L0 functionality and the last one which is running on L0 (it has very low coverage comparing to other tests) still passes after the change.

The change itself is NFC for CUDA & HIP, because I only moved the calculation from one place to another. It is less NFC for L0, because there something did change, but at the same time we won't see the positive effect until #19819 comes in.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Great catch! 🚀

@AlexeySachkov
Copy link
Contributor Author

@intel/unified-runtime-reviewers-level-zero, could you please take a look?

I'm mostly looking for feedback about the new helper function that I'm introducing. I can't modify existing one, because there are other APIs which use it, but they expect the "old" pixel-pixel-pixel format contrary to the "new" bytes-pixel-pixel that I'm enforcing.

The way of how the helper will be used isn't final, because bindlessImagesHandleCopyFlags will be significantly reworked later (see #19819)

Co-authored-by: Steffen Larsen <[email protected]>
@AlexeySachkov AlexeySachkov merged commit 3d6a85d into intel:sycl Sep 16, 2025
49 of 50 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/use-correct-units-in-bindless-image-copies branch September 16, 2025 08:00
YixingZhang007 pushed a commit to YixingZhang007/llvm that referenced this pull request Sep 22, 2025
…el#19936)

`urBindlessImagesImageCopyExp` documents its argument that describe copy
region as using `bytes-pixel-pixel` format, while we were mistakenly
passing `pixel-pixel-pixel` format in there.

This PR adjusts that. It is an `NFC` change for CUDA & HIP adapters,
because for them it only changes the place where pixel to byte
conversion happens, but the change is tricker for L0 adapter. In L0,
there are APIs which expect `pixel-pixel-pixel` format and there are
APIs which accept `bytes-pixel-pixel` format, so some extra care is
needed there.

---------

Co-authored-by: Steffen Larsen <[email protected]>
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.

5 participants