-
Notifications
You must be signed in to change notification settings - Fork 127
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
External memory FD #1913
External memory FD #1913
Conversation
CI gfxreconstruct build queued with queue ID 322697. |
CI gfxreconstruct build # 5541 running. |
CI gfxreconstruct build # 5541 failed. |
95eda51
to
eb82b6b
Compare
CI gfxreconstruct build queued with queue ID 323029. |
CI gfxreconstruct build # 5546 running. |
CI gfxreconstruct build # 5546 failed. |
CI gfxreconstruct build queued with queue ID 323117. |
71d4473
to
967717c
Compare
CI gfxreconstruct build queued with queue ID 323118. |
CI gfxreconstruct build # 5551 running. |
CI gfxreconstruct build # 5551 failed. |
CI gfxreconstruct build queued with queue ID 325414. |
CI gfxreconstruct build # 5563 running. |
CI gfxreconstruct build # 5563 failed. |
CI gfxreconstruct build queued with queue ID 326755. |
CI gfxreconstruct build # 5581 running. |
CI gfxreconstruct build # 5581 failed. |
7fbfacb
to
7ddf396
Compare
CI gfxreconstruct build queued with queue ID 327692. |
7ddf396
to
934034a
Compare
CI gfxreconstruct build queued with queue ID 327694. |
934034a
to
b473716
Compare
CI gfxreconstruct build queued with queue ID 327696. |
b473716
to
abf54c2
Compare
CI gfxreconstruct build queued with queue ID 327761. |
CI gfxreconstruct build queued with queue ID 350996. |
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.
looks good!
CI gfxreconstruct build # 350996 lost. |
1 similar comment
CI gfxreconstruct build # 350996 lost. |
f373e50
to
a52947d
Compare
CI gfxreconstruct build queued with queue ID 352671. |
CI gfxreconstruct build # 5895 running. |
CI gfxreconstruct build # 5895 passed. |
device_table_->DestroySampler(device_, draw_sampler_, nullptr); | ||
device_table_->DestroyDescriptorPool(device_, draw_pool_, nullptr); | ||
device_table_->DestroyDescriptorSetLayout(device_, draw_set_layout_, nullptr); | ||
if (draw_sampler_ != VK_NULL_HANDLE) |
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.
Are these guards around Destroys part of the fix for external memory fd?
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.
Yes, I noticed that sampler, pool, and set_layout where not initialized in the path I introduced for initializing a buffer with external memory, so I added these guards to prevent destruction of null handles.
framework/encode/capture_manager.h
Outdated
@@ -52,7 +55,7 @@ GFXRECON_BEGIN_NAMESPACE(encode) | |||
class ApiCaptureManager; | |||
|
|||
// The CommonCaptureManager provides common functionality referenced API specific capture managers | |||
class CommonCaptureManager | |||
class CommonCaptureManager : public OutputStreamWriter |
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.
I would think CommonCaptureManager is better thought to "have" an OutputStreamWriter, not to be a subclass of one. Can the OutputStreadWriter be a member instead or does that make the use of too awkward?
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.
yes, that's a valid point
/** | ||
* @brief Common class with utilities to write to an output stream. | ||
*/ | ||
class OutputStreamWriter |
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.
I think this replaces the previous use of CombineAndWrite, is that correct? This feels like a bigger abstraction than warranted if it just allows not passing in ThreadData
and allows polymorphism for OutputStreamWrite
. It's not much of a burden to pass in GetThreadData()
as a parameter to CombineAndWrite
. Do you anticipate in the future we may have capture managers with different implementations of OutputStreamWrite
? Can you tell me more about the reasoning to apply?
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.
Thank you for your comments, Brad. This one made me think a bit, and I realized the main reason for this was the capture manager doing something special before and after writing to the output stream, so I made a decorator class for the file stream in CommonCaptureManager
to make it clear. This approach is still flexible enough to allow using CommandWriter
from both CommonCaptureManager
and VulkanStateWriter
.
state_tracker_->TrackBufferMemoryBinding(device, buffer, memory, memoryOffset); | ||
} | ||
|
||
if (IsCaptureModeWrite()) |
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.
Is the logic in this PR applicable to AHB and other external memory backing? Not as part of this PR but that maybe a useful thing to bring up in our AHB conversations.
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.
While not familiar with the various external memory types, I would say: yeah, possibly.
a52947d
to
4215ad2
Compare
CI gfxreconstruct build queued with queue ID 356738. |
CI gfxreconstruct build # 5934 running. |
CI gfxreconstruct build # 5934 failed. |
4215ad2
to
7066c7f
Compare
CI gfxreconstruct build queued with queue ID 357017. |
CI gfxreconstruct build # 5938 running. |
7066c7f
to
7e8da58
Compare
CI gfxreconstruct build queued with queue ID 357113. |
CI gfxreconstruct build # 5940 running. |
CI gfxreconstruct build # 5940 passed. |
Move CombineAndWrite helper function to OutputStream and introduce two new classes to extract some common code. - The capture manager performs some operations before and after writing to a file output stream. Make this explicit by wrapping the file output stream in a decorator class. - CommandWriter is an encode utility for writing commands to an output stream and it's used by both the capture manager and the state tracker.
7e8da58
to
f74a09c
Compare
CI gfxreconstruct build queued with queue ID 360731. |
Initial support for VK_KHR_external_memory_fd.
Capture: once a device memory imported from external FD is bound to a buffer, an InitBuffer command is generated with the current content of the memory.
Replay: the import memory FD structure is removed from the pNext chain of
VkMemoryAllocateInfo
, and the initial content of the buffer is initialized through the InitBuffer command.It supports images as well!