-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add setting to cache the GPUMemory mappings for pipeline upload #78
Conversation
Fixes: #72 |
Can one of the admins verify this patch? |
ok to test |
Test summary for commit 749cfb4Driver commits used in build
CTS tests (Failed: 0/227984)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
I'm in favor of this change, but am also concerned that this will grow unboundedly-- many of our systems in the wild still have limited BAR space and thus limited GPU memory that they can CPU-map at a time. Either only doing this when we don't have this limitation (no invisible memory) or having some scheme to eventually unmap the memory so we don't have this issue would resolve this in my mind. |
The mapped memory is unmapped when the internal memory manager is destroyed. But in general since the maximum pool size is large enough the later memory allocations will be serviced by already allocated pools. Also if needed to enable this only when required, I have added a new pal setting that disables it by default. |
Some AAA game titles have very large numbers of pipelines (sometimes tens of thousands). Even with large pools of memory, there could still be a large number of these pools active simultaneously. Keeping these allocations mapped essentially forever (because the internal memory mgr is not destroyed until the parent Having the setting is nice, but a better option would be to have a limit on the number of these large pools which can remain mapped. If we maintained a list of open mappings, and released the least-recently-used mapping when we exceeded our maximum, we'd be able to have most of if not all of the benefits of this change, without any of the potential drawbacks. If this maximum were controllable via a PAL setting, then we could set the maximum to zero in cases where we might need to disable this functionality. |
Makes sense. I have updated this to use a maxNumMappedPool setting to decide the maximum number of mapped pools. Pools are unmapped based on least recently used scheme. |
Test summary for commit 53cefe3Driver commits used in build
CTS tests (Failed: 336/227984)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
Test summary for commit fa620f7Driver commits used in build
CTS tests (Failed: 336/227984)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
Test summary for commit a19f205Driver commits used in build
CTS tests (Failed: 0/227984)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't conform to the PAL coding standards. Please update it so that it does. You can review the standards here: https://github.com/GPUOpen-Drivers/pal/blob/dev/doc/process/palCodingStandards.md
d107728
to
b3626e7
Compare
I have also change the max logic from number of pools to the size of mapped pools. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, just a few minor formatting nits. I realize these may seem arbitrary or unnecessary, but the rationale behind them is to enforce a consistent style throughout all of PAL. Doing so helps maintain the readability of the codebase for all developers, internal and external to AMD. It is much appreciated.
Thanks for your patience on this.
Makes perfect sense, really appreciate the detailed review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me.
@jinjianrong -- What additional testing (if any) is required before this can be marked as approved?
I 'll re-trigger the test ( retest this please) |
Test summary for commit 12c9330Driver commits used in build
CTS tests (Failed: 0/225188)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
@@ -2019,4 +2036,4 @@ | |||
"Description": "Maximum string length for a miscellaneous string setting" | |||
} | |||
] | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samikhawaja could you tell me how you made this change in your editor? I can't tell the difference between the two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just a trailing newline. No functional change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coding standard above doesn't say anything about the trailing new line. I can remove it if this is not ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok, no need to change it any more :)
retest this please |
2 similar comments
retest this please |
retest this please |
/rebase |
@samikhawaja could you rebase your branch? |
During pipeline upload the GpuMemory is allocated from the pool. This memory is mapped, pipeline is uploaded and then the memory is unmapped. In applications with lots of pipelines being created during rendering, this causes lots of map/unmap calls and slows down the pipeline upload. Add a setting that allows the cache of the GpuMemory mapping, so that it can be reused by another pipeline upload.
Sure. Rebased the branch on dev. |
Test summary for commit e129c53Driver commits used in build
CTS tests (Failed: 0/225376)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8Ubuntu 20.04, Gfx103 |
e129c53 Jenkins build error. |
e129c53 Jenkins build error. |
During pipeline upload the GpuMemory is allocated from the pool. This
memory is mapped, pipeline is uploaded and then the memory is unmapped.
In applications with lots of pipelines being created during rendering,
this causes lots of map/unmap calls and slows down the pipeline upload.
Add a setting that allows the cache of the GpuMemory mapping, so that it
can be reused by another pipeline upload.