Skip to content

Commit

Permalink
syncval: Encapsulate sync event state in context
Browse files Browse the repository at this point in the history 
Encapsulate sync event tracking with it's own context object to better
support replay operations of events recorded in one context being
replayed in another.

Change-Id: I8ff43baaaad974bd486e8b10315ba83497a48569
  • Loading branch information
@jzulauf-lunarg @jeremyg-lunarg
jzulauf-lunarg authored and jeremyg-lunarg committed Feb 2, 2021
1 parent d511570 commit 669dfd5
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 87 deletions.
110 changes: 43 additions & 67 deletions layers/synchronization_validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1779,7 +1779,10 @@ void AccessContext::RecordLayoutTransitions(const RENDER_PASS_STATE &rp_state, u

void CommandBufferAccessContext::ApplyGlobalBarriersToEvents(const SyncExecScope &src, const SyncExecScope &dst) {
const bool all_commands_bit = 0 != (src.mask_param & VK_PIPELINE_STAGE_ALL_COMMANDS_BIT);
for (auto &event_pair : event_state_) {

auto *events_context = GetCurrentEventsContext();
assert(events_context);
for (auto &event_pair : *events_context) {
assert(event_pair.second); // Shouldn't be storing empty
auto &sync_event = *event_pair.second;
// Events don't happen at a stage, so we need to check and store the unexpanded ALL_COMMANDS if set for inter-event-calls
Expand Down Expand Up @@ -2215,7 +2218,10 @@ bool CommandBufferAccessContext::ValidateSetEvent(VkCommandBuffer commandBuffer,
// I'll put this here just in case we need to pass this in for future extension support
const auto cmd = CMD_SETEVENT;
bool skip = false;
const auto *sync_event = GetEventState(event);
const auto *event_state = sync_state_->Get<EVENT_STATE>(event);
if (!event_state) return skip;

const auto *sync_event = GetCurrentEventsContext()->Get(event_state);
if (!sync_event) return false; // Core, Lifetimes, or Param check needs to catch invalid events.

const char *const reset_set =
Expand Down Expand Up @@ -2262,7 +2268,10 @@ bool CommandBufferAccessContext::ValidateSetEvent(VkCommandBuffer commandBuffer,

void CommandBufferAccessContext::RecordSetEvent(VkCommandBuffer commandBuffer, VkEvent event, VkPipelineStageFlags stageMask,
const ResourceUsageTag &tag) {
auto *sync_event = GetEventState(event);
auto event_state_shared = sync_state_->GetShared<EVENT_STATE>(event);
if (!event_state_shared.get()) return; // Core, Lifetimes, or Param check needs to catch invalid events.

auto *sync_event = GetCurrentEventsContext()->GetFromShared(event_state_shared);
if (!sync_event) return; // Core, Lifetimes, or Param check needs to catch invalid events.

// NOTE: We're going to simply record the sync scope here, as anything else would be implementation defined/undefined
Expand Down Expand Up @@ -2304,7 +2313,10 @@ bool CommandBufferAccessContext::ValidateResetEvent(VkCommandBuffer commandBuffe
bool skip = false;
// TODO: EVENTS:
// What is it we need to check... that we've had a reset since a set? Set/Set seems ill formed...
const auto *sync_event = GetEventState(event);
auto event_state = sync_state_->Get<EVENT_STATE>(event);
if (!event_state) return skip; // Core, Lifetimes, or Param check needs to catch invalid events.

const auto *sync_event = GetCurrentEventsContext()->Get(event_state);
if (!sync_event) return false; // Core, Lifetimes, or Param check needs to catch invalid events.

const char *const set_wait =
Expand Down Expand Up @@ -2340,7 +2352,10 @@ bool CommandBufferAccessContext::ValidateResetEvent(VkCommandBuffer commandBuffe

void CommandBufferAccessContext::RecordResetEvent(VkCommandBuffer commandBuffer, VkEvent event, VkPipelineStageFlags stageMask) {
const auto cmd = CMD_RESETEVENT;
auto *sync_event = GetEventState(event);
auto event_state_shared = sync_state_->GetShared<EVENT_STATE>(event);
if (!event_state_shared.get()) return; // Core, Lifetimes, or Param check needs to catch invalid events.

auto *sync_event = GetCurrentEventsContext()->GetFromShared(event_state_shared);
if (!sync_event) return;

// Clear out the first sync scope, any races vs. wait or set are reported, so we'll keep the bookkeeping simple assuming
Expand All @@ -2358,56 +2373,10 @@ void CommandBufferAccessContext::RecordResetEvent(VkCommandBuffer commandBuffer,

void CommandBufferAccessContext::RecordDestroyEvent(VkEvent event) {
// Erase is okay with the key not being
auto event_it = event_state_.find(event);
if (event_it != event_state_.end()) {
SyncEventStateShared &sync_event = event_it->second;
if (sync_event) {
sync_event->destroyed = true;
}
event_state_.erase(event_it);
}
}

SyncEventStateShared &CommandBufferAccessContext::GetEventStateShared(VkEvent event) {
SyncEventStateShared &event_state = event_state_[event];
if (!event_state) {
auto event_atate = sync_state_->GetShared<EVENT_STATE>(event);
event_state.reset(new SyncEventState(event_atate));
}
return event_state;
}

const SyncEventStateShared CommandBufferAccessContext::GetEventStateConstCastShared(VkEvent event) const {
auto event_it = event_state_.find(event);
if (event_it == event_state_.cend()) {
return SyncEventStateShared();
}
return event_it->second;
}

const SyncEventStateConstShared CommandBufferAccessContext::GetEventStateShared(VkEvent event) const {
auto event_it = event_state_.find(event);
if (event_it == event_state_.cend()) {
return SyncEventStateConstShared();
}
return event_it->second;
}

SyncEventState *CommandBufferAccessContext::GetEventState(VkEvent event) {
auto &event_up = event_state_[event];
if (!event_up) {
auto event_atate = sync_state_->GetShared<EVENT_STATE>(event);
event_up.reset(new SyncEventState(event_atate));
}
return event_up.get();
}

const SyncEventState *CommandBufferAccessContext::GetEventState(VkEvent event) const {
auto event_it = event_state_.find(event);
if (event_it == event_state_.cend()) {
return nullptr;
const auto *event_state = sync_state_->Get<EVENT_STATE>(event);
if (event_state) {
GetCurrentEventsContext()->Destroy(event_state);
}
return event_it->second.get();
}

bool RenderPassAccessContext::ValidateDrawSubpassAttachment(const CommandBufferAccessContext &cb_context,
Expand Down Expand Up @@ -5082,7 +5051,7 @@ bool SyncValidator::PreCallValidateCmdWaitEvents(VkCommandBuffer commandBuffer,
assert(cb_context);
if (!cb_context) return skip;

SyncOpWaitEvents wait_events_op(*cb_context, cb_context->GetQueueFlags(), eventCount, pEvents, srcStageMask, dstStageMask,
SyncOpWaitEvents wait_events_op(*this, cb_context->GetQueueFlags(), eventCount, pEvents, srcStageMask, dstStageMask,
memoryBarrierCount, pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers,
imageMemoryBarrierCount, pImageMemoryBarriers);
return wait_events_op.Validate(*cb_context);
Expand All @@ -5104,7 +5073,7 @@ void SyncValidator::PostCallRecordCmdWaitEvents(VkCommandBuffer commandBuffer, u
if (!cb_context) return;

const auto tag = cb_context->NextCommandTag(CMD_WAITEVENTS);
SyncOpWaitEvents wait_events_op(*cb_context, cb_context->GetQueueFlags(), eventCount, pEvents, srcStageMask, dstStageMask,
SyncOpWaitEvents wait_events_op(*this, cb_context->GetQueueFlags(), eventCount, pEvents, srcStageMask, dstStageMask,
memoryBarrierCount, pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers,
imageMemoryBarrierCount, pImageMemoryBarriers);
return wait_events_op.Record(cb_context, tag);
Expand Down Expand Up @@ -5313,15 +5282,15 @@ void SyncOpBarriers::MakeImageMemoryBarriers(const SyncValidator &sync_state, co
}
}

SyncOpWaitEvents::SyncOpWaitEvents(const CommandBufferAccessContext &cb_context, VkQueueFlags queue_flags, uint32_t eventCount,
SyncOpWaitEvents::SyncOpWaitEvents(const SyncValidator &sync_state, VkQueueFlags queue_flags, uint32_t eventCount,
const VkEvent *pEvents, VkPipelineStageFlags srcStageMask, VkPipelineStageFlags dstStageMask,
uint32_t memoryBarrierCount, const VkMemoryBarrier *pMemoryBarriers,
uint32_t bufferMemoryBarrierCount, const VkBufferMemoryBarrier *pBufferMemoryBarriers,
uint32_t imageMemoryBarrierCount, const VkImageMemoryBarrier *pImageMemoryBarriers)
: SyncOpBarriers(cb_context.GetSyncState(), queue_flags, srcStageMask, dstStageMask, VkDependencyFlags(0U), memoryBarrierCount,
: SyncOpBarriers(sync_state, queue_flags, srcStageMask, dstStageMask, VkDependencyFlags(0U), memoryBarrierCount,
pMemoryBarriers, bufferMemoryBarrierCount, pBufferMemoryBarriers, imageMemoryBarrierCount,
pImageMemoryBarriers) {
MakeEventsList(cb_context, eventCount, pEvents);
MakeEventsList(sync_state, eventCount, pEvents);
}

bool SyncOpWaitEvents::Validate(const CommandBufferAccessContext &cb_context) const {
Expand All @@ -5341,11 +5310,15 @@ bool SyncOpWaitEvents::Validate(const CommandBufferAccessContext &cb_context) co

VkPipelineStageFlags event_stage_masks = 0U;
bool events_not_found = false;
for (const auto &sync_event_shared : events_) {
const auto sync_event = sync_event_shared.get();
const auto *events_context = cb_context.GetCurrentEventsContext();
assert(events_context);
for (const auto &sync_event_pair : *events_context) {
const auto *sync_event = sync_event_pair.second.get();
if (!sync_event) {
// NOTE PHASE2: This is where we'll need queue submit time validation to come back and check the srcStageMask bits
events_not_found = true; // Demote "extra_stage_bits" error to warning, to avoid false positives.
// or solve this with replay creating the SyncEventState in the queue context... also this will be a
// new validation error... wait without previously submitted set event...
events_not_found = true; // Demote "extra_stage_bits" error to warning, to avoid false positives at *record time*

continue; // Core, Lifetimes, or Param check needs to catch invalid events.
}
Expand Down Expand Up @@ -5494,6 +5467,9 @@ void SyncOpWaitEvents::Record(CommandBufferAccessContext *cb_context, const Reso
auto *access_context = cb_context->GetCurrentAccessContext();
assert(access_context);
if (!access_context) return;
auto *events_context = cb_context->GetCurrentEventsContext();
assert(events_context);
if (!events_context) return;

// Unlike PipelineBarrier, WaitEvent is *not* limited to accesses within the current subpass (if any) and thus needs to import
// all accesses. Can instead import for all first_scopes, or a union of them, if this becomes a performance/memory issue,
Expand All @@ -5503,9 +5479,9 @@ void SyncOpWaitEvents::Record(CommandBufferAccessContext *cb_context, const Reso
const auto &dst = dst_exec_scope_;
// TODO... this needs change the SyncEventContext it's using depending on whether this is replay... the recorded
// sync_event will be in the recorded context, but we need to update the sync_events in the current context....
for (auto &sync_event_shared : events_) {
const auto sync_event = sync_event_shared.get();
if (!sync_event) continue;
for (auto &event_shared : events_) {
if (!event_shared.get()) continue;
auto *sync_event = events_context->GetFromShared(event_shared);

sync_event->last_command = CMD_WAITEVENTS;

Expand Down Expand Up @@ -5533,9 +5509,9 @@ void SyncOpWaitEvents::Record(CommandBufferAccessContext *cb_context, const Reso
access_context->ApplyToContext(apply_pending_action);
}

void SyncOpWaitEvents::MakeEventsList(const CommandBufferAccessContext &cb_context, uint32_t event_count, const VkEvent *events) {
void SyncOpWaitEvents::MakeEventsList(const SyncValidator &sync_state, uint32_t event_count, const VkEvent *events) {
events_.reserve(event_count);
for (uint32_t event_index = 0; event_index < event_count; event_index++) {
events_.emplace_back(cb_context.GetEventStateConstCastShared(events[event_index]));
events_.emplace_back(sync_state.GetShared<EVENT_STATE>(events[event_index]));
}
}
77 changes: 57 additions & 20 deletions layers/synchronization_validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ enum class AccessAddressType : uint32_t { kLinear = 0, kIdealized = 1, kMaxType

struct SyncEventState {
enum IgnoreReason { NotIgnored = 0, ResetWaitRace, SetRace, MissingStageBits };
using EventPointer = std::shared_ptr<EVENT_STATE>;
using EventPointer = std::shared_ptr<const EVENT_STATE>;
using ScopeMap = sparse_container::range_map<VkDeviceSize, bool>;
EventPointer event;
CMD_TYPE last_command; // Only Event commands are valid here.
Expand All @@ -166,8 +166,9 @@ struct SyncEventState {
ResourceUsageTag first_scope_tag;
bool destroyed;
std::array<ScopeMap, static_cast<size_t>(AccessAddressType::kTypeCount)> first_scope;
SyncEventState(const EventPointer &event_state)
: event(event_state),
template <typename EventPointerType>
SyncEventState(EventPointerType &&event_state)
: event(std::forward<EventPointerType>(event_state)),
last_command(CMD_NONE),
unsynchronized_set(CMD_NONE),
barriers(0U),
Expand All @@ -182,7 +183,49 @@ struct SyncEventState {
};
using SyncEventStateShared = std::shared_ptr<SyncEventState>;
using SyncEventStateConstShared = std::shared_ptr<const SyncEventState>;
using SyncEventsContext = std::unordered_map<VkEvent, SyncEventStateShared>;
class SyncEventsContext {
public:
using Map = std::unordered_map<const EVENT_STATE *, SyncEventStateShared>;
using iterator = Map::iterator;
using const_iterator = Map::const_iterator;

SyncEventState *GetFromShared(const SyncEventState::EventPointer &event_state) {
const auto find_it = map_.find(event_state.get());
if (find_it == map_.end()) {
const auto *event_plain_ptr = event_state.get();
auto sync_state = SyncEventStateShared(new SyncEventState(event_state));
auto insert_pair = map_.insert(std::make_pair(event_plain_ptr, std::move(sync_state)));
return insert_pair.first->second.get();
}
return find_it->second.get();
}

const SyncEventState *Get(const EVENT_STATE *event_state) const {
const auto find_it = map_.find(event_state);
if (find_it == map_.end()) {
return nullptr;
}
return find_it->second.get();
}

// stl style naming for range-for support
inline iterator begin() { return map_.begin(); }
inline const_iterator begin() const { return map_.begin(); }
inline iterator end() { return map_.end(); }
inline const_iterator end() const { return map_.end(); }

void Destroy(const EVENT_STATE *event_state) {
auto sync_it = map_.find(event_state);
if (sync_it != map_.end()) {
sync_it->second->destroyed = true;
map_.erase(sync_it);
}
}
void Clear() { map_.clear(); }

private:
Map map_;
};

// To represent ordering guarantees such as rasterization and store

Expand Down Expand Up @@ -468,9 +511,9 @@ class SyncOpPipelineBarrier : public SyncOpBarriers {

class SyncOpWaitEvents : public SyncOpBarriers {
public:
SyncOpWaitEvents(const CommandBufferAccessContext &cb_context, VkQueueFlags queue_flags, uint32_t eventCount,
const VkEvent *pEvents, VkPipelineStageFlags srcStageMask, VkPipelineStageFlags dstStageMask,
uint32_t memoryBarrierCount, const VkMemoryBarrier *pMemoryBarriers, uint32_t bufferMemoryBarrierCount,
SyncOpWaitEvents(const SyncValidator &sync_state, VkQueueFlags queue_flags, uint32_t eventCount, const VkEvent *pEvents,
VkPipelineStageFlags srcStageMask, VkPipelineStageFlags dstStageMask, uint32_t memoryBarrierCount,
const VkMemoryBarrier *pMemoryBarriers, uint32_t bufferMemoryBarrierCount,
const VkBufferMemoryBarrier *pBufferMemoryBarriers, uint32_t imageMemoryBarrierCount,
const VkImageMemoryBarrier *pImageMemoryBarriers);
bool Validate(const CommandBufferAccessContext &cb_context) const override;
Expand All @@ -479,8 +522,8 @@ class SyncOpWaitEvents : public SyncOpBarriers {
protected:
// TODO PHASE2 This is the wrong thing to use for "replay".. as the event state will have moved on since the record
// TODO PHASE2 May need to capture by value w.r.t. "first use" or build up in calling/enqueue context through replay.
std::vector<SyncEventStateShared> events_;
void MakeEventsList(const CommandBufferAccessContext &cb_context, uint32_t event_count, const VkEvent *events);
std::vector<std::shared_ptr<const EVENT_STATE>> events_;
void MakeEventsList(const SyncValidator &sync_state, uint32_t event_count, const VkEvent *events);
};

class AccessContext {
Expand Down Expand Up @@ -708,7 +751,7 @@ class CommandBufferAccessContext {
current_renderpass_context_(),
cb_state_(),
queue_flags_(),
event_state_(),
events_context_(),
destroyed_(false) {}
CommandBufferAccessContext(SyncValidator &sync_validator, std::shared_ptr<CMD_BUFFER_STATE> &cb_state, VkQueueFlags queue_flags)
: CommandBufferAccessContext() {
Expand All @@ -726,15 +769,15 @@ class CommandBufferAccessContext {
render_pass_contexts_.clear();
current_context_ = &cb_access_context_;
current_renderpass_context_ = nullptr;
event_state_.clear();
events_context_.Clear();
}
void MarkDestroyed() { destroyed_ = true; }
bool IsDestroyed() const { return destroyed_; }

std::string FormatUsage(const HazardResult &hazard) const;
AccessContext *GetCurrentAccessContext() { return current_context_; }
SyncEventsContext *GetCurrectEventsContext() { return &event_state_; }
const SyncEventsContext *GetCurrectEventsContext() const { return &event_state_; }
SyncEventsContext *GetCurrentEventsContext() { return &events_context_; }
const SyncEventsContext *GetCurrentEventsContext() const { return &events_context_; }
const AccessContext *GetCurrentAccessContext() const { return current_context_; }
void RecordBeginRenderPass(const ResourceUsageTag &tag);
void ApplyGlobalBarriersToEvents(const SyncExecScope &src, const SyncExecScope &dst);
Expand Down Expand Up @@ -796,13 +839,7 @@ class CommandBufferAccessContext {
return *sync_state_;
}

const SyncEventStateShared GetEventStateConstCastShared(VkEvent) const;

private:
SyncEventStateShared &GetEventStateShared(VkEvent);
const SyncEventStateConstShared GetEventStateShared(VkEvent) const;
SyncEventState *GetEventState(VkEvent);
const SyncEventState *GetEventState(VkEvent) const;
ResourceUsageTag::TagIndex access_index_;
uint32_t command_number_;
uint32_t subcommand_number_;
Expand All @@ -815,7 +852,7 @@ class CommandBufferAccessContext {
SyncValidator *sync_state_;

VkQueueFlags queue_flags_;
SyncEventsContext event_state_;
SyncEventsContext events_context_;
bool destroyed_;
};

Expand Down

0 comments on commit 669dfd5

Please sign in to comment.