SWDEV-547291 - Interop for OpenGL (#2350)

Updated to convert flags correctly

Added ObjectRegistry to track registered and mapped resources and incorporated it into hip_gl.

Added mip level check

Made functions static in-line

Reworked validation to be more clear.

projects/clr/hipamd/include/hip/amd_detail/amd_hip_gl_interop.h

@@ -81,7 +81,7 @@ hipError_t hipGLGetDevices(unsigned int* pHipDeviceCount, int* pHipDevices,
  *
  * @param [out] resource - Returns pointer of graphics resource.
  * @param [in] buffer - Buffer to be registered.
- * @param [in] flags - Register flags.
+ * @param [in] flags - Register OpenGL flags.
  *
  * @returns #hipSuccess, #hipErrorInvalidValue, #hipErrorUnknown, #hipErrorInvalidResourceHandle
  *
@@ -94,7 +94,7 @@ hipError_t hipGraphicsGLRegisterBuffer(hipGraphicsResource** resource, GLuint bu
  * @param [out] resource - Returns pointer of graphics resource.
  * @param [in] image - Image to be registered.
  * @param [in] target - Valid target value Id.
- * @param [in] flags - Register flags.
+ * @param [in] flags - Register OpenGL flags.
  *
  * @returns #hipSuccess, #hipErrorInvalidValue, #hipErrorUnknown, #hipErrorInvalidResourceHandle
  *

projects/clr/hipamd/src/hip_device.cpp

@@ -325,6 +325,8 @@ Device::~Device() {
     default_mem_pool_->release();
   }
 
+  clearAllTrackedObjects();
+
   if (graph_mem_pool_ != nullptr) {
     graph_mem_pool_->release();
   }

projects/clr/hipamd/src/hip_gl.cpp

@@ -64,6 +64,35 @@ void setupGLInteropOnce() {
   }
 }
 
+static inline hipError_t validateResources(hipGraphicsResource_t* resources, int count = 1) {
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    return hipErrorNoDevice;
+  }
+
+  if (count <= 0) {
+    LogError("invalid count");
+    return hipErrorInvalidValue;
+  }
+
+  if (resources == nullptr) {
+    return hipErrorInvalidValue;
+  }
+
+  for (int i = 0; i < count; i++) {
+    if (resources[i] == nullptr) {
+      return hipErrorInvalidValue;
+    }
+    if (!device->registeredGraphics().isValid(resources[i])) {
+      return hipErrorInvalidHandle;
+    }
+    if (!device->mappedGraphics().isValid(resources[i])) {
+      return hipErrorNotMapped;
+    }
+  }
+  return hipSuccess;
+}
+
 static inline hipError_t hipSetInteropObjects(int num_objects, void** mem_objects,
                                               std::vector<amd::Memory*>& interopObjects) {
   if ((num_objects == 0 && mem_objects != nullptr) ||
@@ -74,13 +103,13 @@ static inline hipError_t hipSetInteropObjects(int num_objects, void** mem_object
   while (num_objects-- > 0) {
     void* obj = *mem_objects++;
     if (obj == nullptr) {
-      return hipErrorInvalidResourceHandle;
+      return hipErrorInvalidHandle;
     }
 
     amd::Memory* mem = reinterpret_cast<amd::Memory*>(obj);
 
     if (mem->getInteropObj() == nullptr) {
-      return hipErrorInvalidResourceHandle;
+      return hipErrorInvalidHandle;
     }
 
     interopObjects.push_back(mem);
@@ -184,20 +213,39 @@ hipError_t hipGraphicsSubResourceGetMappedArray(hipArray_t* array, hipGraphicsRe
   HIP_INIT_API(hipGraphicsSubResourceGetMappedArray, array, resource, arrayIndex, mipLevel);
 
   amd::Context& amdContext = *(hip::getCurrentDevice()->asContext());
-  if (array == nullptr || resource == nullptr) {
-    LogError("invalid array/resource");
+  if (array == nullptr) {
+    LogError("invalid array");
     HIP_RETURN(hipErrorInvalidValue);
   }
 
+  hipError_t status = validateResources(&resource);
+  if (status != hipSuccess) {
+    LogError("invalid resource");
+    HIP_RETURN(status);
+  }
+
   amd::Image* image = (reinterpret_cast<amd::Memory*>(resource))->asImage();
   if (image == nullptr) {
     LogError("invalid resource/image");
+    HIP_RETURN(hipErrorNotMappedAsArray);
+  }
+  // arrayIndex higher than zero not implemented
+  if (arrayIndex > 0) {
+    LogError("invalid arrayIndex, arrayIndex higher than zero not implemented");
     HIP_RETURN(hipErrorInvalidValue);
   }
-  // arrayIndex higher than zero not implmented
-  if (arrayIndex > 0) {
-    return hipErrorInvalidValue;
+  
+  size_t height = image->getHeight();
+  size_t width = image->getWidth();
+  size_t depth = image->getDepth();
+  size_t max_dim = std::max({height, width, depth});
+  unsigned int max_mipLevel = 1 + static_cast<unsigned int>(std::floor(std::log2(max_dim)));
+
+  if (mipLevel > max_mipLevel) {
+    LogError("invalid mipLevel");
+    HIP_RETURN(hipErrorInvalidValue);
   }
+
   amd::Image* view = image->createView(amdContext, image->getImageFormat(), nullptr, mipLevel, 0);
 
   hipArray* myarray = new hipArray();
@@ -225,16 +273,43 @@ hipError_t hipGraphicsSubResourceGetMappedArray(hipArray_t* array, hipGraphicsRe
   HIP_RETURN(hipSuccess);
 }
 
+// Helper function to convert from OpenGL Flags to HIP Memory Flags
+hipError_t HipToClMemoryFlags(uint32_t gl_flags, cl_mem_flags* cl_flags) {
+  if (cl_flags == nullptr) {
+    return hipErrorInvalidValue;
+  }
+  switch (gl_flags) {
+      case hipGraphicsRegisterFlagsNone:
+        *cl_flags = 0;
+        break;
+      case hipGraphicsRegisterFlagsReadOnly:
+        *cl_flags = CL_MEM_READ_ONLY;
+        break;
+      case hipGraphicsRegisterFlagsWriteDiscard:
+        *cl_flags = CL_MEM_WRITE_ONLY;
+        break;
+      case hipGraphicsRegisterFlagsSurfaceLoadStore:
+        *cl_flags = CL_MEM_READ_WRITE;
+        break;
+      case hipGraphicsRegisterFlagsTextureGather:
+        *cl_flags = CL_MEM_READ_WRITE | CL_MEM_READ_ONLY;
+        break;
+      default:
+        return hipErrorInvalidValue;
+        break;
+  }
+  return hipSuccess;
+}
+
 hipError_t hipGraphicsGLRegisterImage(hipGraphicsResource** resource, GLuint image, GLenum target,
                                       unsigned int flags) {
   HIP_INIT_API(hipGraphicsGLRegisterImage, resource, image, target, flags);
 
-  if (!((flags == hipGraphicsRegisterFlagsNone) || (flags & hipGraphicsRegisterFlagsReadOnly) ||
-        (flags & hipGraphicsRegisterFlagsWriteDiscard) ||
-        (flags & hipGraphicsRegisterFlagsSurfaceLoadStore) ||
-        (flags & hipGraphicsRegisterFlagsTextureGather))) {
-    LogError("invalid parameter \"flags\"");
-    HIP_RETURN(hipErrorInvalidValue);
+  cl_mem_flags cl_flags = 0;
+  hipError_t status = HipToClMemoryFlags(flags, &cl_flags);
+  if (status != hipSuccess) {
+    LogPrintfError("invalid parameter \"flags\" %u, gl interop can not convert", flags);
+    HIP_RETURN(status);
   }
 
   if (resource == nullptr) {
@@ -275,7 +350,7 @@ hipError_t hipGraphicsGLRegisterImage(hipGraphicsResource** resource, GLuint ima
   if ((GL_FALSE == amdContext.glenv()->glIsTexture_(image)) ||
       (GL_NO_ERROR != (glErr = amdContext.glenv()->glGetError_()))) {
     LogWarning("\"texture\" is not a GL texture object");
-    HIP_RETURN(hipErrorUnknown);
+    HIP_RETURN(hipErrorInvalidValue);
   }
 
   bool isImage = true;
@@ -470,7 +545,7 @@ hipError_t hipGraphicsGLRegisterImage(hipGraphicsResource** resource, GLuint ima
     // Now get CL format from GL format and bytes per pixel
     int iBytesPerPixel = 0;
     if (!amd::getCLFormatFromGL(amdContext, glInternalFormat, &clImageFormat, &iBytesPerPixel,
-                                flags)) {
+                                cl_flags)) {
       LogWarning("\"texture\" format does not map to an appropriate CL image format");
       HIP_RETURN(hipErrorInvalidValue);
     }
@@ -496,7 +571,7 @@ hipError_t hipGraphicsGLRegisterImage(hipGraphicsResource** resource, GLuint ima
   target = (glTarget == GL_TEXTURE_CUBE_MAP) ? target : 0;
 
   pImageGL = new (amdContext)
-      amd::ImageGL(amdContext, clType, flags, clImageFormat, static_cast<size_t>(gliTexWidth),
+      amd::ImageGL(amdContext, clType, cl_flags, clImageFormat, static_cast<size_t>(gliTexWidth),
                    static_cast<size_t>(gliTexHeight), static_cast<size_t>(gliTexDepth), glTarget,
                    image, 0, glInternalFormat, clGLType, numSamples, target);
 
@@ -530,6 +605,16 @@ hipError_t hipGraphicsGLRegisterImage(hipGraphicsResource** resource, GLuint ima
   mem->processGLResource(device::Memory::GLDecompressResource);
 
   *resource = reinterpret_cast<hipGraphicsResource*>(pImageGL);
+
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    return hipErrorNoDevice;
+  }
+
+  if (!device->registeredGraphics().add(*resource)) {
+    LogError("duplicate resource");
+    HIP_RETURN(hipErrorUnknown);
+  }
   HIP_RETURN(hipSuccess);
 }
 
@@ -537,10 +622,11 @@ hipError_t hipGraphicsGLRegisterBuffer(hipGraphicsResource** resource, GLuint bu
                                        unsigned int flags) {
   HIP_INIT_API(hipGraphicsGLRegisterBuffer, resource, buffer, flags);
 
-  if (!((flags == hipGraphicsRegisterFlagsNone) || (flags & hipGraphicsRegisterFlagsReadOnly) ||
-        (flags & hipGraphicsRegisterFlagsWriteDiscard))) {
-    LogError("invalid parameter \"flags\"");
-    HIP_RETURN(hipErrorInvalidValue);
+  cl_mem_flags cl_flags = 0;
+  hipError_t status = HipToClMemoryFlags(flags, &cl_flags);
+  if (status != hipSuccess) {
+    LogPrintfError("invalid parameter \"flags\" %u, gl interop can not convert", flags);
+    HIP_RETURN(status);
   }
 
   if (resource == nullptr) {
@@ -574,7 +660,7 @@ hipError_t hipGraphicsGLRegisterBuffer(hipGraphicsResource** resource, GLuint bu
     if ((GL_FALSE == amdContext.glenv()->glIsBuffer_(buffer)) ||
         (GL_NO_ERROR != (glErr = amdContext.glenv()->glGetError_()))) {
       LogWarning("\"buffer\" is not a GL buffer object");
-      HIP_RETURN(hipErrorInvalidResourceHandle);
+      HIP_RETURN(hipErrorInvalidValue);
     }
 
     // Check if size is available - data store is created
@@ -583,17 +669,17 @@ hipError_t hipGraphicsGLRegisterBuffer(hipGraphicsResource** resource, GLuint bu
     amdContext.glenv()->glGetBufferParameteriv_(glTarget, GL_BUFFER_SIZE, &gliSize);
     if (GL_NO_ERROR != (glErr = amdContext.glenv()->glGetError_())) {
       LogWarning("cannot get the GL buffer size");
-      HIP_RETURN(hipErrorInvalidResourceHandle);
+      HIP_RETURN(hipErrorInvalidValue);
     }
     if (gliSize == 0) {
       LogWarning("the GL buffer's data store is not created");
-      HIP_RETURN(hipErrorInvalidResourceHandle);
+      HIP_RETURN(hipErrorInvalidValue);
     }
 
   }  // Release scoped lock
 
   // Now create BufferGL object
-  pBufferGL = new (amdContext) amd::BufferGL(amdContext, flags, gliSize, 0, buffer);
+  pBufferGL = new (amdContext) amd::BufferGL(amdContext, cl_flags, gliSize, 0, buffer);
 
   if (!pBufferGL) {
     LogWarning("cannot create object of class BufferGL");
@@ -627,12 +713,35 @@ hipError_t hipGraphicsGLRegisterBuffer(hipGraphicsResource** resource, GLuint bu
 
   *resource = reinterpret_cast<hipGraphicsResource*>(pBufferGL);
 
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    return hipErrorNoDevice;
+  }
+
+  if (!device->registeredGraphics().add(*resource)) {
+    LogError("duplicate resource");
+    HIP_RETURN(hipErrorUnknown);
+  }
   HIP_RETURN(hipSuccess);
 }
-
+ 
 hipError_t hipGraphicsMapResources(int count, hipGraphicsResource_t* resources,
                                    hipStream_t stream) {
   HIP_INIT_API(hipGraphicsMapResources, count, resources, stream);
+
+  if (!hip::isValid(stream)) {
+    HIP_RETURN(hipErrorContextIsDestroyed);
+  }
+
+  hipError_t status = validateResources(resources, count);
+  if (status != hipErrorNotMapped) {
+    LogError("invalid resource(s)");
+    if (status == hipSuccess) {
+      status = hipErrorAlreadyMapped;
+    }
+    HIP_RETURN(status);
+  }
+
   amd::Context* amdContext = hip::getCurrentDevice()->asContext();
   if (!amdContext || !amdContext->glenv()) {
     HIP_RETURN(hipErrorUnknown);
@@ -645,12 +754,12 @@ hipError_t hipGraphicsMapResources(int count, hipGraphicsResource_t* resources,
 
   hip::Stream* hip_stream = hip::getStream(stream);
   if (nullptr == hip_stream) {
-    HIP_RETURN(hipErrorUnknown);
+    HIP_RETURN(hipErrorContextIsDestroyed);
   }
 
   if (!hip_stream->context().glenv() || !hip_stream->context().glenv()->isAssociated()) {
     LogWarning("\"amdContext\" is not created from GL context or share list");
-    HIP_RETURN(hipErrorUnknown);
+    HIP_RETURN(hipErrorContextIsDestroyed);
   }
 
   std::vector<amd::Memory*> memObjects;
@@ -688,12 +797,39 @@ hipError_t hipGraphicsMapResources(int count, hipGraphicsResource_t* resources,
     amd::MemObjMap::AddMemObj(reinterpret_cast<void*>(mem->virtualAddress()), mobj);
     mobj->retain();
   }
+  // Track mapping status
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    return hipErrorNoDevice;
+  }
+  for (int i = 0; i < count; i++) {
+    if (!device->mappedGraphics().add(resources[i])) {
+      HIP_RETURN(hipErrorMapFailed);
+    }
+  }
   HIP_RETURN(hipSuccess);
 }
 
 hipError_t hipGraphicsResourceGetMappedPointer(void** devPtr, size_t* size,
                                                hipGraphicsResource_t resource) {
   HIP_INIT_API(hipGraphicsResourceGetMappedPointer, devPtr, size, resource);
+
+  if (devPtr == nullptr) {
+    LogError("invalid device pointer");
+    HIP_RETURN(hipErrorInvalidValue);
+  }
+
+  if (size == nullptr) {
+    LogError("invalid size");
+    HIP_RETURN(hipErrorInvalidValue);
+  }
+
+  hipError_t status = validateResources(&resource);
+  if (status != hipSuccess) {
+    LogError("invalid resource");
+    HIP_RETURN(status);
+  }
+
   amd::Context* amdContext = hip::getCurrentDevice()->asContext();
   if (!amdContext || !amdContext->glenv()) {
     HIP_RETURN(hipErrorUnknown);
@@ -707,12 +843,27 @@ hipError_t hipGraphicsResourceGetMappedPointer(void** devPtr, size_t* size,
 
   amd::Device* curDev = *it;
   amd::Memory* amdMem = reinterpret_cast<amd::Memory*>(resource);
+
+  // Check if not a buffer
+  amd::Buffer* buffer = amdMem->asBuffer();
+  if (buffer == nullptr) {
+    LogError("resource not mapped as pointer");
+    HIP_RETURN(hipErrorNotMappedAsPointer);
+  }
+
   *size = amdMem->getSize();
 
   // Interop resources don't have svm allocations they are added to
   // amd::MemObjMap using device virtual address during creation.
-  device::Memory* mem = reinterpret_cast<device::Memory*>(amdMem->getDeviceMemory(*curDev));
-  *devPtr = reinterpret_cast<void*>(static_cast<uintptr_t>(mem->virtualAddress()));
+  device::Memory* devMem = reinterpret_cast<device::Memory*>(amdMem->getDeviceMemory(*curDev));
+
+  // Not mapped
+  if (devMem == nullptr) {
+    LogError("resource not mapped");
+    HIP_RETURN(hipErrorNotMapped);
+  }
+
+  *devPtr = reinterpret_cast<void*>(static_cast<uintptr_t>(devMem->virtualAddress()));
   HIP_RETURN(hipSuccess);
 }
 
@@ -726,9 +877,15 @@ hipError_t hipGraphicsUnmapResources(int count, hipGraphicsResource_t* resources
   // Wait for the current host queue
   hip::getStream(stream)->finish();
 
+  hipError_t status = validateResources(resources, count);
+  if (status != hipSuccess) {
+    LogError("resource(s) not mapped");
+    HIP_RETURN(status);
+  }
+
   hip::Stream* hip_stream = hip::getStream(stream);
   if (nullptr == hip_stream) {
-    HIP_RETURN(hipErrorUnknown);
+    HIP_RETURN(hipErrorContextIsDestroyed);
   }
 
   std::vector<amd::Memory*> memObjects;
@@ -760,6 +917,18 @@ hipError_t hipGraphicsUnmapResources(int count, hipGraphicsResource_t* resources
   for (auto& mobj : memObjects) {
     mobj->release();
   }
+
+  // Remove mapping from registry
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    return hipErrorNoDevice;
+  }
+  for (uint8_t i = 0; i < count; i++) {
+    if (!device->mappedGraphics().remove(resources[i])) {
+      LogError("failed to unmap resource");
+      return hipErrorUnknown;
+    }
+  }
   HIP_RETURN(hipSuccess);
 }
 
@@ -769,6 +938,28 @@ hipError_t hipGraphicsUnregisterResource(hipGraphicsResource_t resource) {
   if (resource == nullptr) {
     HIP_RETURN(hipErrorInvalidValue);
   }
+  hip::Device* device = hip::getCurrentDevice();
+  if (device == nullptr) {
+    LogError("no device");
+    HIP_RETURN(hipErrorNoDevice);
+  }
+
+  if (device->mappedGraphics().isValid(resource)) {
+    LogError("resource still mapped");
+    HIP_RETURN(hipErrorArrayIsMapped);
+  }
+
+  if (!device->registeredGraphics().isValid(resource)) {
+    LogError("resource not registered");
+    HIP_RETURN(hipErrorInvalidHandle);
+  }
+
+  // Safe to remove from registered list
+  if (!device->registeredGraphics().remove(resource)) {
+    LogError("failed to unregister resource");
+    HIP_RETURN(hipErrorUnknown);
+  }
+
   reinterpret_cast<amd::BufferGL*>(resource)->release();
 
   HIP_RETURN(hipSuccess);

projects/clr/hipamd/src/hip_internal.hpp

@@ -32,6 +32,7 @@
 #include <stack>
 #include <mutex>
 #include <iterator>
+#include <algorithm>
 #ifdef _WIN32
 #include <process.h>
 #else
@@ -480,6 +481,40 @@ public:
       ~Stream() {};
   };
 
+  // Generic object tracking class of type T
+  template <typename T>
+  class ObjectRegistry {
+  public:
+    // Adds the object to the set. Returns true if successful
+    bool add(T object) {
+      if (object == nullptr) return false;
+
+      std::lock_guard<std::mutex> lock(mtx_);
+      auto result = objects_.insert(object);
+      return result.second;
+    }
+    // Removes the object from the set. Returns true if successful
+    bool remove(T object) {
+      std::lock_guard<std::mutex> lock(mtx_);
+      return objects_.erase(object) > 0;
+    }
+    // Returns true if set contains object
+    bool isValid(T object) const {
+      if (object == nullptr) return false;
+      std::lock_guard<std::mutex> lock(mtx_);
+      return objects_.find(object) != objects_.end();
+    }
+    // Clears the set
+    void clear() {
+      std::lock_guard<std::mutex> lock(mtx_);
+      objects_.clear();
+    }
+
+  private:
+    mutable std::mutex mtx_;
+    std::unordered_set<T> objects_;
+  };
+
   /// HIP Device class
   class Device : public amd::ReferenceCountedObject {
     // Device lock
@@ -508,6 +543,9 @@ public:
     MemoryPool* graph_mem_pool_;    //!< Memory pool, associated with graphs for this device
 
     std::set<MemoryPool*> mem_pools_;
+    // Tracking Objects
+    ObjectRegistry<hipGraphicsResource_t> registeredGraphicsResources_; //!< Track registered graphics resources
+    ObjectRegistry<hipGraphicsResource_t> mappedGraphicsResources_;     //!< Track mapped graphics resources
 
   public:
     Device(amd::Context* ctx, int devId): context_(ctx),
@@ -617,6 +655,19 @@ public:
   /// Wait all active streams on the blocking queue. The method enqueues a wait command and
   /// doesn't stall the current thread
     void WaitActiveStreams(hip::Stream* blocking_stream, bool wait_null_stream = false);
+
+    ObjectRegistry<hipGraphicsResource_t>& registeredGraphics() {
+      return registeredGraphicsResources_;
+    };
+
+    ObjectRegistry<hipGraphicsResource_t>& mappedGraphics() {
+      return mappedGraphicsResources_;
+    };
+
+    void clearAllTrackedObjects() {
+      registeredGraphicsResources_.clear();
+      mappedGraphicsResources_.clear();
+    }
   };
 
   /// Thread Local Storage Variables Aggregator Class

projects/hip-tests/catch/unit/gl_interop/hipGLGetDevices.cc

@@ -96,4 +96,4 @@ TEST_CASE("Unit_hipGLGetDevices_Negative_Parameters") {
     REQUIRE(gl_device_count == 0);
     REQUIRE(gl_devices.at(0) == -1);
   }
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsGLRegisterBuffer.cc

@@ -113,4 +113,4 @@ TEST_CASE("Unit_hipGraphicsGLRegisterBuffer_Negative_Parameters") {
         hipGraphicsGLRegisterBuffer(&vbo_resource, vbo, hipGraphicsRegisterFlagsTextureGather),
         hipErrorInvalidValue);
   }
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsGLRegisterImage.cc

@@ -116,4 +116,4 @@ TEST_CASE("Unit_hipGraphicsGLRegisterImage_Negative_Parameters") {
                                                std::numeric_limits<unsigned int>::max()),
                     hipErrorInvalidValue);
   }
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsMapResources.cc

@@ -108,4 +108,4 @@ TEST_CASE("Unit_hipGraphicsMapResources_Negative_Parameters") {
   }
 
   HIP_CHECK(hipGraphicsUnregisterResource(vbo_resource));
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsResourceGetMappedPointer.cc

@@ -60,7 +60,7 @@ TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Positive_Basic") {
   HIP_CHECK(hipGraphicsUnregisterResource(vbo_resource));
 }
 
-TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Positive_Parameters") {
+TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Null_Parameters") {
   GLContextScopeGuard gl_context;
 
   const int device_count = HipTest::getDeviceCount();
@@ -84,14 +84,23 @@ TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Positive_Parameters") {
   size_t size = 0;
 
   SECTION("devPtr == nullptr") {
-    HIP_CHECK(hipGraphicsResourceGetMappedPointer(nullptr, &size, vbo_resource));
-    REQUIRE(size == vbo.kSize);
+    HIP_CHECK_ERROR(hipGraphicsResourceGetMappedPointer(nullptr, &size, vbo_resource), hipErrorInvalidValue);
   }
 
   SECTION("size == nullptr") {
-    HIP_CHECK(hipGraphicsResourceGetMappedPointer(reinterpret_cast<void**>(&buffer_devptr), nullptr,
-                                                  vbo_resource));
-    REQUIRE(buffer_devptr != nullptr);
+    HIP_CHECK_ERROR(hipGraphicsResourceGetMappedPointer(reinterpret_cast<void**>(&buffer_devptr), nullptr,
+                                                  vbo_resource), hipErrorInvalidValue);
+  }
+
+  SECTION("resource == nullptr") {
+    hipGraphicsResource* null_resource = nullptr;
+    HIP_CHECK_ERROR(hipGraphicsResourceGetMappedPointer(reinterpret_cast<void**>(&buffer_devptr), &size,
+                                                  null_resource), hipErrorInvalidValue);
+  }
+
+  SECTION("devPtr == nullptr && size == nullptr") {
+    HIP_CHECK_ERROR(hipGraphicsResourceGetMappedPointer(nullptr, nullptr,
+                                                  vbo_resource), hipErrorInvalidValue);
   }
 
   HIP_CHECK(hipGraphicsUnmapResources(1, &vbo_resource, 0));
@@ -145,7 +154,7 @@ TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Negative_Parameters") {
     HIP_CHECK(hipGraphicsUnregisterResource(unregistered_resource));
     HIP_CHECK_ERROR(hipGraphicsResourceGetMappedPointer(reinterpret_cast<void**>(&buffer_devptr),
                                                         &size, unregistered_resource),
-                    hipErrorContextIsDestroyed);
+                    hipErrorInvalidHandle);
   }
 
   SECTION("not mapped resource") {
@@ -175,4 +184,4 @@ TEST_CASE("Unit_hipGraphicsResourceGetMappedPointer_Negative_Parameters") {
   HIP_CHECK(hipGraphicsUnmapResources(1, &vbo_resource, 0));
 
   HIP_CHECK(hipGraphicsUnregisterResource(vbo_resource));
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsSubResourceGetMappedArray.cc

@@ -81,7 +81,7 @@ TEST_CASE("Unit_hipGraphicsSubResourceGetMappedArray_Negative_Parameters") {
   hipArray_t image_devptr = nullptr;
 
   SECTION("array == nullptr") {
-    HIP_CHECK(hipGraphicsSubResourceGetMappedArray(nullptr, tex_resource, 0, 0));
+    HIP_CHECK_ERROR(hipGraphicsSubResourceGetMappedArray(nullptr, tex_resource, 0, 0), hipErrorInvalidValue);
   }
 
   SECTION("non-texture resource") {
@@ -105,7 +105,7 @@ TEST_CASE("Unit_hipGraphicsSubResourceGetMappedArray_Negative_Parameters") {
     HIP_CHECK(hipGraphicsUnregisterResource(unregistered_resource));
     HIP_CHECK_ERROR(
         hipGraphicsSubResourceGetMappedArray(&image_devptr, unregistered_resource, 0, 0),
-        hipErrorContextIsDestroyed);
+        hipErrorInvalidHandle);
   }
 
   SECTION("not mapped resource") {
@@ -147,4 +147,4 @@ TEST_CASE("Unit_hipGraphicsSubResourceGetMappedArray_Negative_Parameters") {
   HIP_CHECK(hipGraphicsUnmapResources(1, &tex_resource, 0));
 
   HIP_CHECK(hipGraphicsUnregisterResource(tex_resource));
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsUnmapResources.cc

@@ -51,7 +51,7 @@ TEST_CASE("Unit_hipGraphicsUnmapResources_Negative_Parameters") {
   }
 
   SECTION("resources == nullptr") {
-    HIP_CHECK_ERROR(hipGraphicsUnmapResources(1, nullptr, 0), hipErrorUnknown);
+    HIP_CHECK_ERROR(hipGraphicsUnmapResources(1, nullptr, 0), hipErrorInvalidValue);
   }
 
   SECTION("not mapped resource") {
@@ -72,4 +72,4 @@ TEST_CASE("Unit_hipGraphicsUnmapResources_Negative_Parameters") {
   HIP_CHECK(hipGraphicsUnmapResources(1, &vbo_resource, 0));
 
   HIP_CHECK(hipGraphicsUnregisterResource(vbo_resource));
-}
+}

projects/hip-tests/catch/unit/gl_interop/hipGraphicsUnregisterResource.cc

@@ -40,18 +40,23 @@ TEST_CASE("Unit_hipGraphicsUnregisterResource_Negative_Parameters") {
 
   GLBufferObject vbo;
 
-  SECTION("already unregistered resource") {
+  SECTION("null resource") {
+    hipGraphicsResource* null_resource = nullptr;
+    HIP_CHECK_ERROR(hipGraphicsUnregisterResource(null_resource), hipErrorInvalidValue);
+  }
+
+    SECTION("already unregistered resource") {
     hipGraphicsResource* unregistered_resource;
     HIP_CHECK(
         hipGraphicsGLRegisterBuffer(&unregistered_resource, vbo, hipGraphicsRegisterFlagsNone));
     HIP_CHECK(hipGraphicsUnregisterResource(unregistered_resource));
-    HIP_CHECK_ERROR(hipGraphicsUnregisterResource(unregistered_resource), hipErrorInvalidContext);
+    HIP_CHECK_ERROR(hipGraphicsUnregisterResource(unregistered_resource), hipErrorInvalidHandle);
   }
 
   SECTION("mapped resource") {
     hipGraphicsResource* mapped_resource;
     HIP_CHECK(hipGraphicsGLRegisterBuffer(&mapped_resource, vbo, hipGraphicsRegisterFlagsNone));
     HIP_CHECK(hipGraphicsMapResources(1, &mapped_resource, 0));
-    HIP_CHECK_ERROR(hipGraphicsUnregisterResource(mapped_resource), hipErrorAlreadyMapped);
+    HIP_CHECK_ERROR(hipGraphicsUnregisterResource(mapped_resource), hipErrorArrayIsMapped);
   }
-}
+}

projects/hip/include/hip/hip_runtime_api.h

@@ -1446,12 +1446,12 @@ void __hipGetPCH(const char** pch, unsigned int* size);
  */
 typedef enum hipGraphicsRegisterFlags {
   hipGraphicsRegisterFlagsNone = 0,
-  hipGraphicsRegisterFlagsReadOnly = 1,  ///< HIP will not write to this registered resource
+  hipGraphicsRegisterFlagsReadOnly = 1,  ///< HIP will not write to this registered resource, read only
   hipGraphicsRegisterFlagsWriteDiscard =
-      2,  ///< HIP will only write and will not read from this registered resource
-  hipGraphicsRegisterFlagsSurfaceLoadStore = 4,  ///< HIP will bind this resource to a surface
+      2,  ///< HIP will only write and will not read from this registered resource, write only
+  hipGraphicsRegisterFlagsSurfaceLoadStore = 4,  ///< HIP will bind this resource to a surface, read and write
   hipGraphicsRegisterFlagsTextureGather =
-      8  ///< HIP will perform texture gather operations on this registered resource
+      8  ///< HIP will perform texture gather operations on this registered resource, read and write or read only
 } hipGraphicsRegisterFlags;
 
 typedef struct _hipGraphicsResource hipGraphicsResource;