Skip to content

Commit 18a36c5

Browse files
authored
Fixed false-positive image reuploads (#1557)
* Fixed false-positive image reuploads * Fixed userfaultfd path, removed dead code, simplified calculations * oopsie * track potentially dirty images and hash them * untrack only first page of the image in case of head access * rebase, initialize hash, fix bounds check * include image tail in the calculations
1 parent 3f1be5a commit 18a36c5

File tree

9 files changed

+196
-40
lines changed

9 files changed

+196
-40
lines changed

src/video_core/buffer_cache/buffer_cache.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,7 @@ bool BufferCache::SynchronizeBufferFromImage(Buffer& buffer, VAddr device_addr,
635635
"Texel buffer aliases image subresources {:x} : {:x}", device_addr,
636636
image.info.guest_address);
637637
boost::container::small_vector<vk::BufferImageCopy, 8> copies;
638-
u32 offset = buffer.Offset(image.cpu_addr);
638+
u32 offset = buffer.Offset(image.info.guest_address);
639639
const u32 num_layers = image.info.resources.layers;
640640
const u32 max_offset = offset + size;
641641
for (u32 m = 0; m < image.info.resources.levels; m++) {

src/video_core/page_manager.cpp

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ struct PageManager::Impl {
114114

115115
// Notify rasterizer about the fault.
116116
const VAddr addr = msg.arg.pagefault.address;
117-
const VAddr addr_page = Common::AlignDown(addr, PAGESIZE);
118-
rasterizer->InvalidateMemory(addr_page, PAGESIZE);
117+
const VAddr addr_page = GetPageAddr(addr);
118+
rasterizer->InvalidateMemory(addr, addr_page, PAGESIZE);
119119
}
120120
}
121121

@@ -157,8 +157,8 @@ struct PageManager::Impl {
157157
const auto addr = reinterpret_cast<VAddr>(fault_address);
158158
const bool is_write = Common::IsWriteError(context);
159159
if (is_write && owned_ranges.find(addr) != owned_ranges.end()) {
160-
const VAddr addr_aligned = Common::AlignDown(addr, PAGESIZE);
161-
rasterizer->InvalidateMemory(addr_aligned, PAGESIZE);
160+
const VAddr addr_aligned = GetPageAddr(addr);
161+
rasterizer->InvalidateMemory(addr, addr_aligned, PAGESIZE);
162162
return true;
163163
}
164164
return false;
@@ -174,6 +174,14 @@ PageManager::PageManager(Vulkan::Rasterizer* rasterizer_)
174174

175175
PageManager::~PageManager() = default;
176176

177+
VAddr PageManager::GetPageAddr(VAddr addr) {
178+
return Common::AlignDown(addr, PAGESIZE);
179+
}
180+
181+
VAddr PageManager::GetNextPageAddr(VAddr addr) {
182+
return Common::AlignUp(addr + 1, PAGESIZE);
183+
}
184+
177185
void PageManager::OnGpuMap(VAddr address, size_t size) {
178186
impl->OnMap(address, size);
179187
}

src/video_core/page_manager.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ class PageManager {
2828
/// Increase/decrease the number of surface in pages touching the specified region
2929
void UpdatePagesCachedCount(VAddr addr, u64 size, s32 delta);
3030

31+
static VAddr GetPageAddr(VAddr addr);
32+
static VAddr GetNextPageAddr(VAddr addr);
33+
3134
private:
3235
struct Impl;
3336
std::unique_ptr<Impl> impl;

src/video_core/renderer_vulkan/vk_rasterizer.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -792,9 +792,9 @@ u32 Rasterizer::ReadDataFromGds(u32 gds_offset) {
792792
return value;
793793
}
794794

795-
void Rasterizer::InvalidateMemory(VAddr addr, u64 size) {
796-
buffer_cache.InvalidateMemory(addr, size);
797-
texture_cache.InvalidateMemory(addr, size);
795+
void Rasterizer::InvalidateMemory(VAddr addr, VAddr addr_aligned, u64 size) {
796+
buffer_cache.InvalidateMemory(addr_aligned, size);
797+
texture_cache.InvalidateMemory(addr, addr_aligned, size);
798798
}
799799

800800
void Rasterizer::MapMemory(VAddr addr, u64 size) {

src/video_core/renderer_vulkan/vk_rasterizer.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class Rasterizer {
4646

4747
void InlineData(VAddr address, const void* value, u32 num_bytes, bool is_gds);
4848
u32 ReadDataFromGds(u32 gsd_offset);
49-
void InvalidateMemory(VAddr addr, u64 size);
49+
void InvalidateMemory(VAddr addr, VAddr addr_aligned, u64 size);
5050
void MapMemory(VAddr addr, u64 size);
5151
void UnmapMemory(VAddr addr, u64 size);
5252

src/video_core/texture_cache/image.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,7 @@ void UniqueImage::Create(const vk::ImageCreateInfo& image_ci) {
144144
Image::Image(const Vulkan::Instance& instance_, Vulkan::Scheduler& scheduler_,
145145
const ImageInfo& info_)
146146
: instance{&instance_}, scheduler{&scheduler_}, info{info_},
147-
image{instance->GetDevice(), instance->GetAllocator()}, cpu_addr{info.guest_address},
148-
cpu_addr_end{cpu_addr + info.guest_size_bytes} {
147+
image{instance->GetDevice(), instance->GetAllocator()} {
149148
mip_hashes.resize(info.resources.levels);
150149
ASSERT(info.pixel_format != vk::Format::eUndefined);
151150
// Here we force `eExtendedUsage` as don't know all image usage cases beforehand. In normal case

src/video_core/texture_cache/image.h

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,12 @@ VK_DEFINE_HANDLE(VmaAllocator)
2222
namespace VideoCore {
2323

2424
enum ImageFlagBits : u32 {
25-
CpuDirty = 1 << 1, ///< Contents have been modified from the CPU
25+
Empty = 0,
26+
MaybeCpuDirty = 1 << 0, ///< The page this image is in was touched before the image address
27+
CpuDirty = 1 << 1, ///< Contents have been modified from the CPU
2628
GpuDirty = 1 << 2, ///< Contents have been modified from the GPU (valid data in buffer cache)
27-
Dirty = CpuDirty | GpuDirty,
29+
Dirty = MaybeCpuDirty | CpuDirty | GpuDirty,
2830
GpuModified = 1 << 3, ///< Contents have been modified from the GPU
29-
Tracked = 1 << 4, ///< Writes and reads are being hooked from the CPU
3031
Registered = 1 << 6, ///< True when the image is registered
3132
Picked = 1 << 7, ///< Temporary flag to mark the image as picked
3233
MetaRegistered = 1 << 8, ///< True when metadata for this surface is known and registered
@@ -78,7 +79,9 @@ struct Image {
7879

7980
[[nodiscard]] bool Overlaps(VAddr overlap_cpu_addr, size_t overlap_size) const noexcept {
8081
const VAddr overlap_end = overlap_cpu_addr + overlap_size;
81-
return cpu_addr < overlap_end && overlap_cpu_addr < cpu_addr_end;
82+
const auto image_addr = info.guest_address;
83+
const auto image_end = info.guest_address + info.guest_size_bytes;
84+
return image_addr < overlap_end && overlap_cpu_addr < image_end;
8285
}
8386

8487
ImageViewId FindView(const ImageViewInfo& info) const {
@@ -99,14 +102,18 @@ struct Image {
99102
void CopyImage(const Image& image);
100103
void CopyMip(const Image& image, u32 mip);
101104

105+
bool IsTracked() {
106+
return track_addr != 0 && track_addr_end != 0;
107+
}
108+
102109
const Vulkan::Instance* instance;
103110
Vulkan::Scheduler* scheduler;
104111
ImageInfo info;
105112
UniqueImage image;
106113
vk::ImageAspectFlags aspect_mask = vk::ImageAspectFlagBits::eColor;
107114
ImageFlagBits flags = ImageFlagBits::Dirty;
108-
VAddr cpu_addr = 0;
109-
VAddr cpu_addr_end = 0;
115+
VAddr track_addr = 0;
116+
VAddr track_addr_end = 0;
110117
std::vector<ImageViewInfo> image_view_infos;
111118
std::vector<ImageViewId> image_view_ids;
112119

@@ -130,6 +137,7 @@ struct Image {
130137
std::vector<State> subresource_states{};
131138
boost::container::small_vector<u64, 14> mip_hashes{};
132139
u64 tick_accessed_last{0};
140+
u64 hash{0};
133141

134142
struct {
135143
union {

0 commit comments

Comments
 (0)