From d0196381194e55acbf2ba679f8e63887f89f2716 Mon Sep 17 00:00:00 2001 From: "Hoppe, Mateusz" Date: Fri, 25 May 2018 15:06:44 +0200 Subject: [PATCH] Fix image from subbuffer offsets - change 32 bit offset to size_t to account for offsets bigger than 4 GB in SurfaceOffsets - change ImageInfo offset to size_t type - fix hostptr offseting in subbuffer creation - null hostptr shouldn't be offseted Change-Id: I18a5b8eacbd8c9e40c1f2effe5ded4fa8b453395 --- runtime/helpers/surface_formats.h | 2 +- runtime/mem_obj/buffer.cpp | 2 +- runtime/mem_obj/image.cpp | 6 +-- runtime/mem_obj/image.h | 6 +-- .../mem_obj/image_from_subbuffer_tests.cpp | 26 ++++++++++ unit_tests/mem_obj/image_set_arg_tests.cpp | 16 ++++++ unit_tests/mem_obj/nv12_image_tests.cpp | 11 ++-- unit_tests/mem_obj/sub_buffer_tests.cpp | 50 +++++++++++++++++++ 8 files changed, 106 insertions(+), 13 deletions(-) diff --git a/runtime/helpers/surface_formats.h b/runtime/helpers/surface_formats.h index c5d0640729..7ffea97272 100644 --- a/runtime/helpers/surface_formats.h +++ b/runtime/helpers/surface_formats.h @@ -218,7 +218,7 @@ struct ImageInfo { size_t rowPitch; size_t slicePitch; uint32_t qPitch; - uint32_t offset; + size_t offset; uint32_t xOffset; uint32_t yOffset; uint32_t yOffsetForUVPlane; diff --git a/runtime/mem_obj/buffer.cpp b/runtime/mem_obj/buffer.cpp index f69bbb3a8f..c40b49be49 100644 --- a/runtime/mem_obj/buffer.cpp +++ b/runtime/mem_obj/buffer.cpp @@ -250,7 +250,7 @@ Buffer *Buffer::createSubBuffer(cl_mem_flags flags, DEBUG_BREAK_IF(nullptr == createFunction); auto buffer = createFunction(this->context, flags, region->size, ptrOffset(this->memoryStorage, region->origin), - ptrOffset(this->hostPtr, region->origin), + this->hostPtr ? ptrOffset(this->hostPtr, region->origin) : nullptr, this->graphicsAllocation, this->isZeroCopy, this->isHostPtrSVM, true); diff --git a/runtime/mem_obj/image.cpp b/runtime/mem_obj/image.cpp index 440ccca75e..0f5ab20988 100644 --- a/runtime/mem_obj/image.cpp +++ b/runtime/mem_obj/image.cpp @@ -207,10 +207,8 @@ Image *Image::create(Context *context, parentBuffer->incRefInternal(); Gmm::queryImgFromBufferParams(imgInfo, memory); - auto bufferOffset = static_cast(parentBuffer->getOffset()); - if (bufferOffset != 0) { - imgInfo.offset = bufferOffset; - } + UNRECOVERABLE_IF(imgInfo.offset != 0); + imgInfo.offset = parentBuffer->getOffset(); if (memoryManager->peekVirtualPaddingSupport() && (imageDesc->image_type == CL_MEM_OBJECT_IMAGE2D)) { // Retrieve sizes from GMM and apply virtual padding if buffer storage is not big enough diff --git a/runtime/mem_obj/image.h b/runtime/mem_obj/image.h index a8d2862656..8c300ad4b6 100644 --- a/runtime/mem_obj/image.h +++ b/runtime/mem_obj/image.h @@ -32,7 +32,7 @@ struct KernelInfo; struct SurfaceFormatInfo; struct SurfaceOffsets { - uint32_t offset; + uint64_t offset; uint32_t xOffset; uint32_t yOffset; uint32_t yOffsetForUVplane; @@ -156,7 +156,7 @@ class Image : public MemObj { bool allowTiling() const override { return this->isTiledImage; } void setImageRowPitch(size_t rowPitch) { imageDesc.image_row_pitch = rowPitch; } void setImageSlicePitch(size_t slicePitch) { imageDesc.image_slice_pitch = slicePitch; } - void setSurfaceOffsets(uint32_t offset, uint32_t xOffset, uint32_t yOffset, uint32_t yOffsetForUVPlane) { + void setSurfaceOffsets(uint64_t offset, uint32_t xOffset, uint32_t yOffset, uint32_t yOffsetForUVPlane) { surfaceOffsets.offset = offset; surfaceOffsets.xOffset = xOffset; surfaceOffsets.yOffset = yOffset; @@ -225,7 +225,7 @@ class Image : public MemObj { size_t imageCount = 0; uint32_t cubeFaceIndex; cl_uint mediaPlaneType; - SurfaceOffsets surfaceOffsets; + SurfaceOffsets surfaceOffsets = {0}; uint32_t baseMipLevel = 0; uint32_t mipCount = 1; diff --git a/unit_tests/mem_obj/image_from_subbuffer_tests.cpp b/unit_tests/mem_obj/image_from_subbuffer_tests.cpp index b39b36dc84..b1b6c762fe 100644 --- a/unit_tests/mem_obj/image_from_subbuffer_tests.cpp +++ b/unit_tests/mem_obj/image_from_subbuffer_tests.cpp @@ -101,3 +101,29 @@ TEST_F(ImageFromSubBufferTest, CreateImage2dFromSubBufferWithOffset) { EXPECT_EQ(0u, surfaceOffsets.yOffset); EXPECT_EQ(0u, surfaceOffsets.yOffsetForUVplane); } + +TEST_F(ImageFromSubBufferTest, givenSubbufferWithOffsetGreaterThan4GBWhenImageIsCreatedThenSurfaceOffsetsOffsetHasCorrectValue) { + Buffer *buffer = castToObject(parentBuffer); + uint64_t offsetExpected = 0; + cl_buffer_region region = {0, size / 2}; + + if (is64bit) { + offsetExpected = 8 * GB; + region = {static_cast(offsetExpected), size / 2}; + } + + Buffer *subBufferWithBigOffset = buffer->createSubBuffer(CL_MEM_READ_WRITE, ®ion, retVal); + imageDesc.mem_object = subBufferWithBigOffset; + + std::unique_ptr imageFromSubBuffer(createImage()); + EXPECT_NE(nullptr, imageFromSubBuffer); + + SurfaceOffsets surfaceOffsets = {0}; + imageFromSubBuffer->getSurfaceOffsets(surfaceOffsets); + + EXPECT_EQ(offsetExpected, surfaceOffsets.offset); + EXPECT_EQ(0u, surfaceOffsets.xOffset); + EXPECT_EQ(0u, surfaceOffsets.yOffset); + EXPECT_EQ(0u, surfaceOffsets.yOffsetForUVplane); + subBufferWithBigOffset->release(); +} diff --git a/unit_tests/mem_obj/image_set_arg_tests.cpp b/unit_tests/mem_obj/image_set_arg_tests.cpp index 8d1e3fba90..73611efc89 100644 --- a/unit_tests/mem_obj/image_set_arg_tests.cpp +++ b/unit_tests/mem_obj/image_set_arg_tests.cpp @@ -25,6 +25,7 @@ #include "runtime/helpers/surface_formats.h" #include "runtime/helpers/ptr_math.h" #include "runtime/helpers/aligned_memory.h" +#include "runtime/helpers/basic_math.h" #include "runtime/memory_manager/graphics_allocation.h" #include "runtime/kernel/kernel.h" #include "runtime/mem_obj/image.h" @@ -965,3 +966,18 @@ HWTEST_F(ImageShaderChanelValueTest, ChannelRGBA) { outputChannel = ImageHw::getShaderChannelValue(inputChannel, CL_RGBA); EXPECT_EQ(SURFACE_STATE::SHADER_CHANNEL_SELECT_RED_BLUE, outputChannel); } + +HWTEST_F(ImageSetArgTest, givenImageWithOffsetGreaterThan4GBWhenSurfaceStateIsProgrammedThenCorrectStataBaseAddressIsSet) { + typedef typename FamilyType::RENDER_SURFACE_STATE RENDER_SURFACE_STATE; + RENDER_SURFACE_STATE surfaceState; + + uint64_t surfaceOffset = 8 * GB; + + srcImage->setSurfaceOffsets(surfaceOffset, 0, 0, 0); + srcImage->setImageArg(&surfaceState, false, 0); + + auto expectedAddress = srcImage->getGraphicsAllocation()->getGpuAddressToPatch() + surfaceOffset; + auto surfaceAddress = surfaceState.getSurfaceBaseAddress(); + + EXPECT_EQ(expectedAddress, surfaceAddress); +} diff --git a/unit_tests/mem_obj/nv12_image_tests.cpp b/unit_tests/mem_obj/nv12_image_tests.cpp index cc254c9d7a..c775187821 100644 --- a/unit_tests/mem_obj/nv12_image_tests.cpp +++ b/unit_tests/mem_obj/nv12_image_tests.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, Intel Corporation + * Copyright (c) 2017 - 2018, Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -39,9 +39,9 @@ using namespace OCLRT; class Nv12ImageTest : public testing::Test { public: void computeExpectedOffsets(Image *image) { - SurfaceOffsets expectedSurfaceOffsets = {}; + SurfaceOffsets expectedSurfaceOffsets = {0}; GMM_REQ_OFFSET_INFO reqOffsetInfo = {}; - SurfaceOffsets requestedOffsets = {}; + SurfaceOffsets requestedOffsets = {0}; auto mockResInfo = reinterpret_cast<::testing::NiceMock *>(image->getGraphicsAllocation()->gmm->gmmResourceInfo.get()); mockResInfo->getOffset(reqOffsetInfo); @@ -55,7 +55,10 @@ class Nv12ImageTest : public testing::Test { image->getSurfaceOffsets(requestedOffsets); - EXPECT_TRUE(memcmp(&expectedSurfaceOffsets, &requestedOffsets, sizeof(SurfaceOffsets)) == 0); + EXPECT_EQ(expectedSurfaceOffsets.offset, requestedOffsets.offset); + EXPECT_EQ(expectedSurfaceOffsets.xOffset, requestedOffsets.xOffset); + EXPECT_EQ(expectedSurfaceOffsets.yOffset, requestedOffsets.yOffset); + EXPECT_EQ(expectedSurfaceOffsets.yOffsetForUVplane, requestedOffsets.yOffsetForUVplane); } protected: diff --git a/unit_tests/mem_obj/sub_buffer_tests.cpp b/unit_tests/mem_obj/sub_buffer_tests.cpp index 35b9d3ba8f..eaf076ed1b 100644 --- a/unit_tests/mem_obj/sub_buffer_tests.cpp +++ b/unit_tests/mem_obj/sub_buffer_tests.cpp @@ -22,6 +22,7 @@ #include "runtime/helpers/ptr_math.h" #include "runtime/mem_obj/buffer.h" +#include "runtime/memory_manager/memory_constants.h" #include "unit_tests/fixtures/device_fixture.h" #include "unit_tests/mocks/mock_buffer.h" #include "unit_tests/mocks/mock_context.h" @@ -112,4 +113,53 @@ TEST_F(SubBufferTest, givenSharingHandlerFromParentBufferWhenCreateThenShareHand delete subBuffer; EXPECT_EQ(1, buffer->getRefInternalCount()); } + +TEST_F(SubBufferTest, GivenBufferWithAlignedHostPtrAndSameMemoryStorageWhenSubBufferIsCreatedThenHostPtrAndMemoryStorageAreOffseted) { + cl_buffer_region region = {2, 2}; + cl_int retVal = 0; + + void *alignedPointer = alignedMalloc(MemoryConstants::pageSize, MemoryConstants::preferredAlignment); + Buffer *buffer = Buffer::create(&context, CL_MEM_READ_WRITE | CL_MEM_USE_HOST_PTR, + MemoryConstants::pageSize, alignedPointer, retVal); + + ASSERT_NE(nullptr, buffer); + ASSERT_EQ(CL_SUCCESS, retVal); + EXPECT_EQ(alignedPointer, buffer->getHostPtr()); + EXPECT_EQ(alignedPointer, buffer->getCpuAddress()); + + auto subBuffer = buffer->createSubBuffer(CL_MEM_READ_ONLY, ®ion, retVal); + EXPECT_NE(nullptr, subBuffer); + EXPECT_EQ(CL_SUCCESS, retVal); + + EXPECT_EQ(ptrOffset(alignedPointer, 2), subBuffer->getHostPtr()); + EXPECT_EQ(ptrOffset(alignedPointer, 2), subBuffer->getCpuAddress()); + + subBuffer->release(); + buffer->release(); + alignedFree(alignedPointer); +} + +TEST_F(SubBufferTest, GivenBufferWithMemoryStorageAndNullHostPtrWhenSubBufferIsCreatedThenMemoryStorageIsOffsetedAndHostPtrIsNull) { + cl_buffer_region region = {2, 2}; + cl_int retVal = 0; + + Buffer *buffer = Buffer::create(&context, CL_MEM_READ_WRITE, + MemoryConstants::pageSize, nullptr, retVal); + + ASSERT_NE(nullptr, buffer); + ASSERT_EQ(CL_SUCCESS, retVal); + EXPECT_EQ(nullptr, buffer->getHostPtr()); + EXPECT_NE(nullptr, buffer->getCpuAddress()); + + auto subBuffer = buffer->createSubBuffer(CL_MEM_READ_ONLY, ®ion, retVal); + EXPECT_NE(nullptr, subBuffer); + EXPECT_EQ(CL_SUCCESS, retVal); + + EXPECT_EQ(nullptr, subBuffer->getHostPtr()); + EXPECT_EQ(ptrOffset(buffer->getCpuAddress(), 2), subBuffer->getCpuAddress()); + + subBuffer->release(); + buffer->release(); +} + } // namespace ULT