P4 to Git Change 1529531 by gandryey@gera-w8 on 2018/03/20 11:50:12

SWDEV-79445 - OCL generic changes and code clean-up
	- Fix a regression in the AMF test and reenable the suballoc optimization. Rearrange the locks around cache field access only to avoid calling memory release under the cache lock.

Affected files ...

... //depot/stg/opencl/drivers/opencl/runtime/device/pal/palprogram.cpp#57 edit
... //depot/stg/opencl/drivers/opencl/runtime/device/pal/palresource.cpp#53 edit
... //depot/stg/opencl/drivers/opencl/runtime/device/pal/palresource.hpp#18 edit
... //depot/stg/opencl/drivers/opencl/runtime/utils/flags.hpp#287 edit

rocclr/runtime/device/pal/palprogram.cpp

@@ -86,12 +86,12 @@ void Segment::copy(size_t offset, const void* src, size_t size) {
   if (cpuAccess_ != nullptr) {
     amd::Os::fastMemcpy(cpuAddress(offset), src, size);
   } else {
-    amd::ScopedLock k(gpuAccess_->dev().xferMgr().lockXfer());
     VirtualGPU& gpu = *gpuAccess_->dev().xferQueue();
     Memory& xferBuf = gpuAccess_->dev().xferWrite().acquire();
     size_t tmpSize = std::min(static_cast<size_t>(xferBuf.size()), size);
     size_t srcOffs = 0;
     while (size != 0) {
+      amd::ScopedLock k(gpuAccess_->dev().xferMgr().lockXfer());
       xferBuf.hostWrite(&gpu, reinterpret_cast<const_address>(src) + srcOffs, 0, tmpSize);
       xferBuf.partialMemCopyTo(gpu, 0, (offset + srcOffs), tmpSize, *gpuAccess_, false, true);
       size -= tmpSize;

rocclr/runtime/device/pal/palresource.cpp

@@ -561,7 +561,7 @@ bool Resource::CreateImage(CreateParams* params)
     desc_.pitch_ = rowPitch;
     // Make sure the row pitch is aligned to pixels
     imgCreateInfo.rowPitch =
-      elementSize() * amd::alignUp(rowPitch, dev().info().imagePitchAlignment_);
+        amd::alignUp(elementSize() * rowPitch, dev().info().imagePitchAlignment_);
     imgCreateInfo.depthPitch = imgCreateInfo.rowPitch * desc().height_;
     imgCreateInfo.tiling = tiling;
 
@@ -1983,20 +1983,27 @@ GpuMemoryReference* MemorySubAllocator::Allocate(
 }
 
 // ================================================================================================
-bool MemorySubAllocator::Free(GpuMemoryReference* ref, Pal::gpusize offset)
+bool MemorySubAllocator::Free(amd::Monitor* monitor, GpuMemoryReference* ref, Pal::gpusize offset)
 {
-  // Find if current memory reference is a chunk allocation
-  auto it = mem_heap_.find(ref);
-  if (it == mem_heap_.end()) {
-    return false;
+  bool releaseMem =  false;
+  {
+    amd::ScopedLock l(monitor);
+    // Find if current memory reference is a chunk allocation
+    auto it = mem_heap_.find(ref);
+    if (it == mem_heap_.end()) {
+      return false;
+    }
+    // Free suballocation at the specified offset
+    it->second->Free(offset);
+    // If this suballocator empty, then release memory chunk
+    if (it->second->IsEmpty()) {
+      delete it->second;
+      mem_heap_.erase(it);
+      releaseMem = true;
+    }
   }
-  // Free suballocation at the specified offset
-  it->second->Free(offset);
-  // If this suballocator empty, then release memory chunk
-  if (it->second->IsEmpty()) {
-    delete it->second;
-    it->first->release();
-    mem_heap_.erase(it);
+  if (releaseMem) {
+    ref->release();
   }
   return true;
 }
@@ -2013,9 +2020,8 @@ bool ResourceCache::addGpuMemory(Resource::Descriptor* desc,
   size_t size = ref->iMem()->Desc().size;
 
   if (desc->type_ == Resource::Local) {
-      amd::ScopedLock l(&lockCacheOps_);
     // Check if runtime can free suballocation in local memory
-    if (memSubAllocLocal_.Free(ref, offset)) {
+    if (memSubAllocLocal_.Free(&lockCacheOps_, ref, offset)) {
       return true;
     }
   }
@@ -2024,16 +2030,17 @@ bool ResourceCache::addGpuMemory(Resource::Descriptor* desc,
   if (((desc->type_ == Resource::Local) || (desc->type_ == Resource::Persistent) ||
        (desc->type_ == Resource::Remote) || (desc->type_ == Resource::RemoteUSWC)) &&
       (size < cacheSizeLimit_) && !desc->SVMRes_) {
-    amd::ScopedLock l(&lockCacheOps_);
     // Validate the cache size limit. Loop until we have enough space
     while ((cacheSize_ + size) > cacheSizeLimit_) {
       removeLast();
     }
+
     Resource::Descriptor* descCached = new Resource::Descriptor;
     if (descCached != nullptr) {
       // Copy the original desc to the cached version
       memcpy(descCached, desc, sizeof(Resource::Descriptor));
 
+      amd::ScopedLock l(&lockCacheOps_);
       // Add the current resource to the cache
       resCache_.push_front(std::make_pair(descCached, ref));
       ref->gpu_ = nullptr;
@@ -2085,37 +2092,32 @@ GpuMemoryReference* ResourceCache::findGpuMemory(Resource::Descriptor* desc, Pal
 }
 
 // ================================================================================================
-bool ResourceCache::free(size_t minCacheEntries) {
-  amd::ScopedLock l(&lockCacheOps_);
-  bool result = false;
-
+void ResourceCache::free(size_t minCacheEntries) {
   if (minCacheEntries < resCache_.size()) {
-    if (static_cast<int>(cacheSize_) > 0) {
-      result = true;
-    }
     // Clear the cache
     while (static_cast<int>(cacheSize_) > 0) {
       removeLast();
     }
     CondLog((cacheSize_ != 0), "Incorrect size for cache release!");
   }
-  return result;
 }
 
 // ================================================================================================
-void ResourceCache::removeLast() {
+void ResourceCache::removeLast()
+{
   std::pair<Resource::Descriptor*, GpuMemoryReference*> entry;
-  entry = resCache_.back();
-  resCache_.pop_back();
-
-  size_t size = entry.second->iMem()->Desc().size;
-
-  // Delete Descriptor
-  delete entry.first;
+  {
+    // Protect access to the global data
+    amd::ScopedLock l(&lockCacheOps_);
+    entry = resCache_.back();
+    resCache_.pop_back();
+    // Delete Descriptor
+    delete entry.first;
+    cacheSize_ -= entry.second->iMem()->Desc().size;
+  }
 
   // Destroy PAL resource
   entry.second->release();
-  cacheSize_ -= size;
 }
 
 }  // namespace pal

rocclr/runtime/device/pal/palresource.hpp

@@ -258,7 +258,8 @@ class Resource : public amd::HeapObject {
   uint64_t vmAddress() const { return iMem()->Desc().gpuVirtAddr + offset_; }
 
   //! Returns global memory offset
-  uint64_t vmSize() const { return desc_.width_ * elementSize(); }
+  uint64_t vmSize() const
+    { return desc_.width_ * desc_.height_ * desc_.depth_ * elementSize(); }
 
   //! Returns global memory offset
   bool mipMapped() const { return (desc().mipLevels_ > 1) ? true : false; }
@@ -484,7 +485,7 @@ public:
 
   GpuMemoryReference*  Allocate(Pal::gpusize size,
     Pal::gpusize alignment, Pal::gpusize* offset);
-  bool Free(GpuMemoryReference* ref, Pal::gpusize offset);
+  bool Free(amd::Monitor* monitor, GpuMemoryReference* ref, Pal::gpusize offset);
 
 private:
   Device* device_;
@@ -515,7 +516,7 @@ class ResourceCache : public amd::HeapObject {
       Pal::gpusize size, Pal::gpusize alignment, Pal::gpusize* offset);
 
   //! Destroys cache
-  bool free(size_t minCacheEntries = 0);
+  void free(size_t minCacheEntries = 0);
 
  private:
   //! Disable copy constructor

rocclr/runtime/utils/flags.hpp

@@ -86,7 +86,7 @@ release(size_t, GPU_PINNED_MIN_XFER_SIZE, 512,                                \
         "The minimal buffer size for pinned read/write transfers in KBytes")  \
 release(size_t, GPU_RESOURCE_CACHE_SIZE, 64,                                  \
         "The resource cache size in MB")                                      \
-release(size_t, GPU_MAX_SUBALLOC_SIZE, 0,                                     \
+release(size_t, GPU_MAX_SUBALLOC_SIZE, 4096,                                  \
         "The maximum size accepted for suballocaitons in KB")                 \
 release(uint, GPU_ASYNC_MEM_COPY, 0,                                          \
         "Enables async memory transfers with DRM engine")                     \