Skip to content

Commit 17ff30f

Browse files
Erfan-AhmadiErfan-Ahmadi
committed
moved checking validity of regions to the beginning of the image upload function
1 parent 24527cb commit 17ff30f

File tree

2 files changed

+29
-53
lines changed

2 files changed

+29
-53
lines changed

‎include/nbl/video/IGPUImage.h

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,19 @@ class NBL_API IGPUImage : public asset::IImage, public IDeviceMemoryBacked, publ
4343
E_OBJECT_TYPE getObjectType() const override { return EOT_IMAGE; }
4444

4545
//!
46-
virtual bool validateCopies(const core::SRange<const SBufferCopy>& regions, const IGPUBuffer* src, const asset::VkExtent3D& minImageTransferGranularity = { 1,1,1 }) const
46+
virtual bool validateCopies(const SBufferCopy* pRegionsBegin, const SBufferCopy* pRegionsEnd, const IGPUBuffer* src, const asset::VkExtent3D& minImageTransferGranularity = { 1,1,1 }) const
4747
{
48-
if (!validateCopies_template(regions.begin(), regions.end(), src))
48+
if (!validateCopies_template(pRegionsBegin, pRegionsEnd, src))
4949
return false;
5050

5151
const auto srcParams = src->getCreationParams();
5252
if (!srcParams.usage.hasFlags(asset::IBuffer::E_USAGE_FLAGS::EUF_TRANSFER_SRC_BIT))
5353
return false;
5454

55-
for (const auto region : regions)
55+
for (auto region = pRegionsBegin; region != pRegionsEnd; region++)
5656
{
57-
auto subresourceSize = getMipSize(region.imageSubresource.mipLevel);
58-
if (!validateCopyOffsetAndExtent(region.imageExtent, region.imageOffset, subresourceSize, minImageTransferGranularity))
57+
auto subresourceSize = getMipSize(region->imageSubresource.mipLevel);
58+
if (!validateCopyOffsetAndExtent(region->imageExtent, region->imageOffset, subresourceSize, minImageTransferGranularity))
5959
return false;
6060
}
6161

@@ -67,22 +67,22 @@ class NBL_API IGPUImage : public asset::IImage, public IDeviceMemoryBacked, publ
6767
}
6868

6969
//!
70-
virtual bool validateCopies(const core::SRange<const SImageCopy>& regions, const IGPUImage* src, const asset::VkExtent3D& minImageTransferGranularity = {1,1,1}) const
70+
virtual bool validateCopies(const SImageCopy* pRegionsBegin, const SImageCopy* pRegionsEnd, const IGPUImage* src, const asset::VkExtent3D& minImageTransferGranularity = { 1,1,1 }) const
7171
{
72-
if (!validateCopies_template(regions.begin(), regions.begin(), src))
72+
if (!validateCopies_template(pRegionsBegin, pRegionsEnd, src))
7373
return false;
7474

7575
const auto srcParams = src->getCreationParameters();
7676
if (!srcParams.usage.hasFlags(asset::IImage::E_USAGE_FLAGS::EUF_TRANSFER_SRC_BIT))
7777
return false;
7878

79-
for (const auto region : regions)
79+
for (auto region = pRegionsBegin; region != pRegionsEnd; region++)
8080
{
81-
auto srcSubresourceSize = src->getMipSize(region.srcSubresource.mipLevel);
82-
if (!validateCopyOffsetAndExtent(region.extent, region.srcOffset, srcSubresourceSize, minImageTransferGranularity))
81+
auto srcSubresourceSize = src->getMipSize(region->srcSubresource.mipLevel);
82+
if (!validateCopyOffsetAndExtent(region->extent, region->srcOffset, srcSubresourceSize, minImageTransferGranularity))
8383
return false;
84-
auto dstSubresourceSize = getMipSize(region.dstSubresource.mipLevel);
85-
if (!validateCopyOffsetAndExtent(region.extent, region.dstOffset, dstSubresourceSize, minImageTransferGranularity))
84+
auto dstSubresourceSize = getMipSize(region->dstSubresource.mipLevel);
85+
if (!validateCopyOffsetAndExtent(region->extent, region->dstOffset, dstSubresourceSize, minImageTransferGranularity))
8686
return false;
8787
}
8888

‎src/nbl/video/utilities/IUtilities.cpp

Lines changed: 17 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,22 @@ IGPUQueue::SSubmitInfo IUtilities::updateImageViaStagingBuffer(
5151
srcFormat = dstImage->getCreationParameters().format;
5252
}
5353

54+
// Validate Copies from srcBuffer to dstImage with these regions
55+
// if the initial regions are valid then ImageRegionIterator will do it's job correctly breaking it down ;)
56+
// note to future self: couldn't use dstImage->validateCopies because it doesn't consider that cpubuffer will be promoted and hence it will get a validation error about size of the buffer being smaller than max accessible offset.
57+
bool regionsValid = true;
58+
for (const auto region : regions)
59+
{
60+
auto subresourceSize = dstImage->getMipSize(region.imageSubresource.mipLevel);
61+
if (!dstImage->validateCopyOffsetAndExtent(region.imageExtent, region.imageOffset, subresourceSize, minImageTransferGranularity))
62+
regionsValid = false;
63+
}
64+
if (!regionsValid)
65+
{
66+
assert(false);
67+
return intendedNextSubmit;
68+
}
69+
5470
ImageRegionIterator regionIterator = ImageRegionIterator(regions, queueFamProps, srcBuffer, srcFormat, dstImage);
5571

5672
// Assuming each thread can handle minImageTranferGranularitySize of texelBlocks:
@@ -231,7 +247,7 @@ ImageRegionIterator::ImageRegionIterator(
231247
size_t ImageRegionIterator::getMemoryNeededForRemainingRegions() const
232248
{
233249
asset::TexelBlockInfo dstImageTexelBlockInfo(dstImageFormat);
234-
assert(dstImageTexelBlockInfo.getBlockByteSize()>0u);
250+
assert(dstImageTexelBlockInfo.getBlockByteSize() > 0u);
235251
auto texelBlockDim = dstImageTexelBlockInfo.getDimension();
236252
uint32_t memoryNeededForRemainingRegions = 0ull;
237253

@@ -246,44 +262,6 @@ size_t ImageRegionIterator::getMemoryNeededForRemainingRegions() const
246262
{
247263
const asset::IImage::SBufferCopy & region = regions[i];
248264

249-
auto subresourceSize = dstImage->getMipSize(region.imageSubresource.mipLevel);
250-
251-
// Validate Region, TODO: move these to IGPUImage::validateCopies and call them on every region at the beginning
252-
253-
assert(static_cast<uint32_t>(region.imageSubresource.aspectMask) != 0u);
254-
assert(core::isPoT(static_cast<uint32_t>(region.imageSubresource.aspectMask)) && "region.aspectMask should only have a single bit set.");
255-
256-
// canTransferMipLevelsPartially = !(minImageTransferGranularity.width == 0 && minImageTransferGranularity.height == 0 && minImageTransferGranularity.depth == 0);
257-
if (canTransferMipLevelsPartially)
258-
{
259-
// region.imageOffset.{xyz} should be multiple of minImageTransferGranularity.{xyz} scaled up by block size
260-
bool isImageOffsetAlignmentValid =
261-
(region.imageOffset.x % (minImageTransferGranularity.width * texelBlockDim.x) == 0) &&
262-
(region.imageOffset.y % (minImageTransferGranularity.height * texelBlockDim.y) == 0) &&
263-
(region.imageOffset.z % (minImageTransferGranularity.depth * texelBlockDim.z) == 0);
264-
assert(isImageOffsetAlignmentValid);
265-
266-
// region.imageExtent.{xyz} should be multiple of minImageTransferGranularity.{xyz} scaled up by block size,
267-
// OR ELSE (region.imageOffset.{x/y/z} + region.imageExtent.{width/height/depth}) MUST be equal to subresource{Width,Height,Depth}
268-
bool isImageExtentAlignmentValid =
269-
(region.imageExtent.width % (minImageTransferGranularity.width * texelBlockDim.x) == 0 || (region.imageOffset.x + region.imageExtent.width == subresourceSize.x)) &&
270-
(region.imageExtent.height % (minImageTransferGranularity.height * texelBlockDim.y) == 0 || (region.imageOffset.y + region.imageExtent.height == subresourceSize.y)) &&
271-
(region.imageExtent.depth % (minImageTransferGranularity.depth * texelBlockDim.z) == 0 || (region.imageOffset.z + region.imageExtent.depth == subresourceSize.z));
272-
assert(isImageExtentAlignmentValid);
273-
274-
bool isImageExtentAndOffsetValid =
275-
(region.imageExtent.width + region.imageOffset.x <= subresourceSize.x) &&
276-
(region.imageExtent.height + region.imageOffset.y <= subresourceSize.y) &&
277-
(region.imageExtent.depth + region.imageOffset.z <= subresourceSize.z);
278-
assert(isImageExtentAndOffsetValid);
279-
}
280-
else
281-
{
282-
assert(region.imageOffset.x == 0 && region.imageOffset.y == 0 && region.imageOffset.z == 0);
283-
assert(region.imageExtent.width == subresourceSize.x && region.imageExtent.height == subresourceSize.y && region.imageExtent.depth == subresourceSize.z);
284-
}
285-
286-
287265
auto imageExtent = core::vector3du32_SIMD(region.imageExtent.width, region.imageExtent.height, region.imageExtent.depth);
288266
auto imageExtentInBlocks = dstImageTexelBlockInfo.convertTexelsToBlocks(imageExtent);
289267
auto imageExtentBlockStridesInBytes = dstImageTexelBlockInfo.convert3DBlockStridesTo1DByteStrides(imageExtentInBlocks);
@@ -681,7 +659,6 @@ bool ImageRegionIterator::advanceAndCopyToStagingBuffer(asset::IImage::SBufferCo
681659
}
682660
else if (currentBlockInRow == 0 && currentRowInSlice == 0 && canTransferMipLevelsPartially && uploadableSlices > 0)
683661
{
684-
// tryFillLayer();
685662
uint32_t slicesToUploadMemorySize = eachSliceNeededMemory * uploadableSlices;
686663

687664
regionToCopyNext.bufferOffset = stagingBufferOffset;
@@ -720,7 +697,6 @@ bool ImageRegionIterator::advanceAndCopyToStagingBuffer(asset::IImage::SBufferCo
720697
}
721698
else if (currentBlockInRow == 0 && canTransferMipLevelsPartially && uploadableRows > 0)
722699
{
723-
// tryFillSlice();
724700
uint32_t rowsToUploadMemorySize = eachRowNeededMemory * uploadableRows;
725701

726702
regionToCopyNext.bufferOffset = stagingBufferOffset;

0 commit comments

Comments
 (0)