Skip to content

UTs for commandCopyImageToBuffer, commandCopyImage, commandCopyBufferToImage#24

Open
kamil-goras-mobica wants to merge 4 commits intomainfrom
uts_commandCopy
Open

UTs for commandCopyImageToBuffer, commandCopyImage, commandCopyBufferToImage#24
kamil-goras-mobica wants to merge 4 commits intomainfrom
uts_commandCopy

Conversation

@kamil-goras-mobica
Copy link
Collaborator

UTs for commandCopyImageToBuffer, commandCopyImage, commandCopyBufferToImage

Copy link
Collaborator

@shajder shajder left a comment

Choose a reason for hiding this comment

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

My remarks applies for all tests, please review all.

Comment on lines 3643 to 3644
cl::MutableCommandKhr* mutable_handle = nullptr;
const cl::CommandQueue* command_queue = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, preference to save some space and pass nullptr's without additional variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shajder correction added.

Comment on lines 3647 to 3648
ret = commandBufferKhrPool[0].commandCopyImageToBuffer(image2DPool[0], bufferPool[0], origin, region, dst_offset, &sync_points_vec,
&sync_point, mutable_handle, command_queue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could verify sync_point value set in clcommandCopyImageToBufferKHR_testcommandCopyImageToBuffer after this call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shajder correction added.

Comment on lines 3641 to 3642
cl_sync_point_khr sync_point = 0;
const cl::vector<cl_sync_point_khr> sync_points_vec = {sync_point};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will work but the code is a bit misleading. Sync points from wait list should not be the same as one returned by clCommandCopyImageToBuffer. Preference to use different values and avoid putting sync_point to the wait list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shajder correction added.

Copy link
Collaborator

@shajder shajder left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants