amdgpu rendernode somewhat working but no fork support yet#3016
Open
tursulin wants to merge 13 commits into
Open
amdgpu rendernode somewhat working but no fork support yet#3016tursulin wants to merge 13 commits into
tursulin wants to merge 13 commits into
Conversation
Member
|
added 13 commits
May 20, 2026 10:51
If the protobuf image allocation fails we can simply return immediately since there aren't any other things to clean up at this point. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
The DRM minor local variable is shadowed between two blocks inside amdgpu_plugin_drm_dump_file(). If at the top level we access the minor via the copy stored in the protobuf image, we can simply drop this copy and so avoid the buffer object dump loop shadowing it. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
… loop We can save a few lines of code and end up with a potentially more readable function if we replace the open-coded try-retry of getting the list of buffer objects with a loop. While refactoring we also remove the needless copy of the entry as the dump loop iterates the handles, and also stop returning a false success with a made up zero buffer objects saved in cases when the kernel does not support the DRM_IOCTL_AMDGPU_GEM_LIST_HANDLES ioctl. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
…mage name amdgpu_plugin_drm_dump_file() already defines a large temporary stack buffer for the purpose of generating the file level protobuf image name. Instead of the buffer object dumping loop defining a separate temporary stack buffer for the buffer content protobuf images we can simply use the top level one. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
When saving buffer object content the current code constantly allocates and frees aligned memory where the GPU will copy each buffer one by one. We can optimise this path by keeping the buffer around and only grow it if is too small for the current object. While doing this we also fix a theoretically incorrect freeing of memory allocated via posix_memalign with xfree. An alternative solution could be to probe for the maximum size before hand by adding a pre-scan loop. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Instead of initializing and de-initializing libdrm for every buffer object we dump (each of which consists of a few system calls), we can simply initialize it once and by doing so simplify a bunch of the error handling blocks. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Simplify a bunch of error handling blocks by freeing the vm_info_entries array as soon as we are done accessing it. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
There is no need to use the fd libdrm duplicated from the original fd. Just use the original one. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
It is currently not possible to restore a process which did not have a KFD device open. We can work around that by assuming the restore is being done on the same system with the same GPU id. This can fail over reboots when the GPU id changes, but that also could be worked around with perhaps something like a "--ignore-gpu-id" command line switch. In the long term we probably need to think of adding the relevant GPU discovery APIs to the amdgpu kernel driver, which would enable this to work more smoothly. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Add a new boolean environment variable AMDGPU_IGNORE_SINGLE_GPUID which can be used to force restoring of a single GPU image on a single GPU machine. This is useful for images which had no kfd open and if the gpu id had changed for example after re-booting to a different kernel version. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Outside the KFD world the CPU virtual memory address of a mmapp-ed buffer object is not guaranteed to be same as the GPU virtual address. Therefore the address check in VMA restore will prevent any restore of a render node process. For now we relax the criteria in case of render node to match only on the offset. In the future this will need to be made more robust so strange partial mmaps can be handled as well. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Move GPU VA mapping operation to after the buffer object content is restored in order to avoid libdrm VA range manager conflicting with the already mapped buffers and so avoid failing the restore process. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Re-open the DRM device when dumping buffer object content in order to avoid the libdrm VA range manager conflicting with the client being check-pointed. Otherwise buffer content cannot be saved and the process aborts with a failure. The current approach of initializing libdrm of the current fd is insufficient because libdrm, despite libdrm "opening" a different fd it does that by dup() which does not create a new DRM client, so no new GPU VA address space gets created. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
tursulin
force-pushed
the
rendernode-somewhat-working-but-no-fork-support-yet
branch
from
66ec3c6 to
4f0299a
Compare
Author
Thanks for spotting this! I have uploaded a fixed version. One thing to note however is that I am in the process of cleaning up some further patches on top of this series. Those implement more missing functionality like userptr support, context support, reserved vm id and vm always valid objects support. With that work in progress I can checkpoint and restore a client in a busy submit loop. Once I am reasonably happy with that I will extend the RFC to include all that. FYI @fdavid-amd One more thing is that for the above I added some kernel side UAPI too so will need to coordinate posting of IGT + Kernel + CRIU branches for full picture. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cc @fdavid-amd
This is a RFC style PR with 5 interesting patches on top of the cleanups I've sent as separate PRs. The interesting patches are these:
With this I can get all but the last the test from https://cgit.freedesktop.org/~tursulin/intel-gpu-tools/commit/?h=amd-criu&id=24487611e08c753e73b3fc650ca85793bf053f4d to pass. The last one includes forking and for that problem I will kick off a separate discussion.
But the most complicated test case from the above:
Can be checkpointed and restored:
It would be good to test this series with some kfd and kfd+amdgpu combined workload to check I created no regressions.