Fix successWithUnOwnedNativeHandle tests #2182
Conversation
omarahmed1111
force-pushed
the
fix-success-with-unowned-native-handle-tests
branch
from
65dd776
to
4d327f7
Compare
omarahmed1111
changed the title
omarahmed1111
force-pushed
the
fix-success-with-unowned-native-handle-tests
branch
3 times, most recently
from
df2112b
to
d9b94bd
Compare
| @@ -70,7 +70,7 @@ TEST_P(urContextCreateWithNativeHandleTest, SuccessWithUnOwnedNativeHandle) { | |||
| uint32_t ref_count = 0; | |||
| ASSERT_SUCCESS(urContextGetInfo(ctx, UR_CONTEXT_INFO_REFERENCE_COUNT, | |||
| sizeof(uint32_t), &ref_count, nullptr)); | |||
| ASSERT_EQ(ref_count, 2); | |||
| ASSERT_EQ(ref_count, 1); | |||
There was a problem hiding this comment.
for now I think it's correct that cl increments the reference count here, I'm working on what the spec should look like for isNativeHandleOwned but until we figure it out I don't think this change is right
There was a problem hiding this comment.
the answer might be just don't check the reference counts at all and leave it all implementation defined
There was a problem hiding this comment.
I was just thinking what cl does is right but for the native handle itself. But, this query is querying the UR reference count. and UR reference should not be altered whether it owns the native handle or not but rather the reference count of the native handle itself is the one that should be changed. So, it feels more to test this feature should be a backend specific feature rather than a CTS test here. This would be a test for only that creating a UR object from owned/unowned handles is a success.
I don't have strong impression about this still as I think it is still to be more properly defined so, if you think we should wait for this to define isNativeHandleOwned more properly, I am okay with that.
There was a problem hiding this comment.
the answer might be just don't check the reference counts at all and leave it all implementation defined
I think that make sense and that was my intention to make this more generic for now.
omarahmed1111
force-pushed
the
fix-success-with-unowned-native-handle-tests
branch
from
d9b94bd
to
5497860
Compare
omarahmed1111
force-pushed
the
fix-success-with-unowned-native-handle-tests
branch
from
5497860
to
a8b87e7
Compare
There was a problem hiding this comment.
The conclusion I'm coming to is that CL's cast implementation isn't really compatible with how isNativeHandleOwned was meant to be used. Removing the ref count checks here is correct, and your handle PR should bring the adapter behaviour more in line with level zero
This would fix the successWithUnOwnedNativeHandle tests as the reference count returned should be related to the UR object not the native handle so it should be tested to be
1not2.