api: implement error handling for clCreateBuffer* (#602)

Also introduce a dedicated context header so we can use functions from cvk_device in cvk_context. Change-Id: Ied90f4a67d7f088f7756121afc3a41a33f2c6226 Signed-off-by: Kévin Petit <[email protected]>
kpet · Aug 19, 2023 · f181479 · f181479
1 parent 200f247
commit f181479
Show file tree

Hide file tree

Showing 5 changed files with 187 additions and 107 deletions.
diff --git a/src/api.cpp b/src/api.cpp
@@ -1681,6 +1681,88 @@ cl_int CLVK_API_CALL clSetCommandQueueProperty(
 }
 
 // Memory Object APIs
+static cl_mem CLVK_API_CALL cvk_create_buffer_with_properties(
+    cl_context context, const cl_mem_properties* properties, cl_mem_flags flags,
+    size_t size, void* host_ptr, cl_int* errcode_ret) {
+    CVK_ASSERT(errcode_ret != nullptr);
+
+    // Validate context
+    if (!is_valid_context(context)) {
+        *errcode_ret = CL_INVALID_CONTEXT;
+        return nullptr;
+    }
+
+    // Validate properties
+    std::vector<cl_mem_properties> props;
+
+    if (properties != nullptr) {
+        while (*properties) {
+            // We dont't currently support any properties so return an error
+            *errcode_ret = CL_INVALID_PROPERTY;
+            return nullptr;
+            props.push_back(*properties);
+            properties++;
+        }
+        props.push_back(0);
+    }
+
+    // Validate flags
+    if ((flags & CL_MEM_READ_WRITE) && (flags & CL_MEM_WRITE_ONLY)) {
+        *errcode_ret = CL_INVALID_VALUE;
+        return nullptr;
+    }
+    if ((flags & CL_MEM_READ_ONLY) &&
+        (flags & (CL_MEM_WRITE_ONLY | CL_MEM_READ_WRITE))) {
+        *errcode_ret = CL_INVALID_VALUE;
+        return nullptr;
+    }
+    if ((flags & (CL_MEM_ALLOC_HOST_PTR | CL_MEM_COPY_HOST_PTR)) &&
+        (flags & CL_MEM_USE_HOST_PTR)) {
+        *errcode_ret = CL_INVALID_VALUE;
+        return nullptr;
+    }
+    if ((flags & CL_MEM_HOST_READ_ONLY) && (flags & CL_MEM_HOST_WRITE_ONLY)) {
+        *errcode_ret = CL_INVALID_VALUE;
+        return nullptr;
+    }
+    if ((flags & CL_MEM_HOST_NO_ACCESS) &&
+        (flags & (CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_WRITE_ONLY))) {
+        *errcode_ret = CL_INVALID_VALUE;
+        return nullptr;
+    }
+
+    // Validate size
+    // TODO CL_INVALID_BUFFER_SIZE if CL_MEM_USE_HOST_PTR is set in flags and
+    // host_ptr is a pointer returned by clSVMAlloc and size is greater than the
+    // size passed to clSVMAlloc.
+    if ((size == 0) ||
+        (!icd_downcast(context)->is_mem_alloc_size_valid(size))) {
+        *errcode_ret = CL_INVALID_BUFFER_SIZE;
+        return nullptr;
+    }
+
+    // Validate host_ptr
+    if ((host_ptr == nullptr) &&
+        (flags & (CL_MEM_USE_HOST_PTR | CL_MEM_COPY_HOST_PTR))) {
+        *errcode_ret = CL_INVALID_HOST_PTR;
+        return nullptr;
+    }
+    if ((host_ptr != nullptr) &&
+        !(flags & (CL_MEM_USE_HOST_PTR | CL_MEM_COPY_HOST_PTR))) {
+        *errcode_ret = CL_INVALID_HOST_PTR;
+        return nullptr;
+    }
+
+    auto buffer = cvk_buffer::create(icd_downcast(context), flags, size,
+                                     host_ptr, std::move(props), errcode_ret);
+
+    if (*errcode_ret != CL_SUCCESS) {
+        return nullptr;
+    } else {
+        return buffer.release();
+    }
+}
+
 cl_mem CLVK_API_CALL clCreateBuffer(cl_context context, cl_mem_flags flags,
                                     size_t size, void* host_ptr,
                                     cl_int* errcode_ret) {
@@ -1689,26 +1771,15 @@ cl_mem CLVK_API_CALL clCreateBuffer(cl_context context, cl_mem_flags flags,
                  "errcode_ret = %p",
                  context, flags, size, host_ptr, errcode_ret);
 
-    if (!is_valid_context(context)) {
-        if (errcode_ret != nullptr) {
-            *errcode_ret = CL_INVALID_CONTEXT;
-        }
-        return nullptr;
-    }
-
     cl_int err;
-    auto buffer =
-        cvk_buffer::create(icd_downcast(context), flags, size, host_ptr, &err);
+    auto buffer = cvk_create_buffer_with_properties(context, nullptr, flags,
+                                                    size, host_ptr, &err);
 
     if (errcode_ret != nullptr) {
         *errcode_ret = err;
     }
 
-    if (err != CL_SUCCESS) {
-        return nullptr;
-    } else {
-        return buffer.release();
-    }
+    return buffer;
 }
 
 cl_mem CLVK_API_CALL clCreateBufferWithProperties(
@@ -1720,29 +1791,15 @@ cl_mem CLVK_API_CALL clCreateBufferWithProperties(
                  "host_ptr = %p, errcode_ret = %p",
                  context, properties, flags, size, host_ptr, errcode_ret);
 
-    std::vector<cl_mem_properties> props;
-
-    if (properties != nullptr) {
-        while (*properties) {
-            props.push_back(*properties);
-            properties++;
-        }
-        props.push_back(0);
-    }
-
     cl_int err;
-    auto buffer = cvk_buffer::create(icd_downcast(context), flags, size,
-                                     host_ptr, std::move(props), &err);
+    auto buffer = cvk_create_buffer_with_properties(context, properties, flags,
+                                                    size, host_ptr, &err);
 
     if (errcode_ret != nullptr) {
         *errcode_ret = err;
     }
 
-    if (err != CL_SUCCESS) {
-        return nullptr;
-    } else {
-        return buffer.release();
-    }
+    return buffer;
 }
 
 cl_mem CLVK_API_CALL clCreateSubBuffer(cl_mem buf, cl_mem_flags flags,

diff --git a/src/context.hpp b/src/context.hpp
@@ -0,0 +1,97 @@
+// Copyright 2018-2023 The clvk authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#pragma once
+
+#include "device.hpp"
+#include "objects.hpp"
+
+using cvk_context_callback_pointer_type = void(CL_CALLBACK*)(cl_context context,
+                                                             void* user_data);
+struct cvk_context_callback {
+    cvk_context_callback_pointer_type pointer;
+    void* data;
+};
+
+struct cvk_context : public _cl_context,
+                     refcounted,
+                     object_magic_header<object_magic::context> {
+
+    cvk_context(cvk_device* device, const cl_context_properties* props)
+        : m_device(device) {
+
+        if (props) {
+            while (*props) {
+                // Save name
+                m_properties.push_back(*props);
+                // Save value
+                m_properties.push_back(*(props + 1));
+                props += 2;
+            }
+            m_properties.push_back(*props);
+        }
+    }
+
+    virtual ~cvk_context() {
+        for (auto cbi = m_destuctor_callbacks.rbegin();
+             cbi != m_destuctor_callbacks.rend(); ++cbi) {
+            auto cb = *cbi;
+            cb.pointer(this, cb.data);
+        }
+    }
+
+    const std::vector<cl_context_properties>& properties() const {
+        return m_properties;
+    }
+
+    cvk_device* device() const { return m_device; }
+    unsigned num_devices() const { return 1u; }
+    bool has_device(const cvk_device* device) const {
+        return device == m_device;
+    }
+
+    void add_destructor_callback(cvk_context_callback_pointer_type ptr,
+                                 void* user_data) {
+        cvk_context_callback cb = {ptr, user_data};
+        std::lock_guard<std::mutex> lock(m_callbacks_lock);
+        m_destuctor_callbacks.push_back(cb);
+    }
+
+    bool is_mem_alloc_size_valid(size_t size) {
+        // TODO support multiple devices
+        return size <= m_device->max_mem_alloc_size();
+    }
+
+private:
+    cvk_device* m_device;
+    std::mutex m_callbacks_lock;
+    std::vector<cvk_context_callback> m_destuctor_callbacks;
+    std::vector<cl_context_properties> m_properties;
+};
+
+static inline cvk_context* icd_downcast(cl_context context) {
+    return static_cast<cvk_context*>(context);
+}
+
+using cvk_context_holder = refcounted_holder<cvk_context>;
+
+template <object_magic magic>
+struct api_object : public refcounted, object_magic_header<magic> {
+
+    api_object(cvk_context* context) : m_context(context) {}
+    cvk_context* context() const { return m_context; }
+
+protected:
+    cvk_context_holder m_context;
+};
diff --git a/src/event.hpp b/src/event.hpp
@@ -15,6 +15,7 @@
 #pragma once
 
 #include "cl_headers.hpp"
+#include "context.hpp"
 #include "icd.hpp"
 #include "objects.hpp"
 #include "tracing.hpp"

diff --git a/src/objects.hpp b/src/objects.hpp
@@ -131,79 +131,3 @@ template <typename T> struct refcounted_holder {
 private:
     T* m_refcounted;
 };
-
-using cvk_context_callback_pointer_type = void(CL_CALLBACK*)(cl_context context,
-                                                             void* user_data);
-struct cvk_context_callback {
-    cvk_context_callback_pointer_type pointer;
-    void* data;
-};
-
-struct cvk_device;
-
-struct cvk_context : public _cl_context,
-                     refcounted,
-                     object_magic_header<object_magic::context> {
-
-    cvk_context(cvk_device* device, const cl_context_properties* props)
-        : m_device(device) {
-
-        if (props) {
-            while (*props) {
-                // Save name
-                m_properties.push_back(*props);
-                // Save value
-                m_properties.push_back(*(props + 1));
-                props += 2;
-            }
-            m_properties.push_back(*props);
-        }
-    }
-
-    virtual ~cvk_context() {
-        for (auto cbi = m_destuctor_callbacks.rbegin();
-             cbi != m_destuctor_callbacks.rend(); ++cbi) {
-            auto cb = *cbi;
-            cb.pointer(this, cb.data);
-        }
-    }
-
-    const std::vector<cl_context_properties>& properties() const {
-        return m_properties;
-    }
-
-    cvk_device* device() const { return m_device; }
-    unsigned num_devices() const { return 1u; }
-    bool has_device(const cvk_device* device) const {
-        return device == m_device;
-    }
-
-    void add_destructor_callback(cvk_context_callback_pointer_type ptr,
-                                 void* user_data) {
-        cvk_context_callback cb = {ptr, user_data};
-        std::lock_guard<std::mutex> lock(m_callbacks_lock);
-        m_destuctor_callbacks.push_back(cb);
-    }
-
-private:
-    cvk_device* m_device;
-    std::mutex m_callbacks_lock;
-    std::vector<cvk_context_callback> m_destuctor_callbacks;
-    std::vector<cl_context_properties> m_properties;
-};
-
-static inline cvk_context* icd_downcast(cl_context context) {
-    return static_cast<cvk_context*>(context);
-}
-
-using cvk_context_holder = refcounted_holder<cvk_context>;
-
-template <object_magic magic>
-struct api_object : public refcounted, object_magic_header<magic> {
-
-    api_object(cvk_context* context) : m_context(context) {}
-    cvk_context* context() const { return m_context; }
-
-protected:
-    cvk_context_holder m_context;
-};
diff --git a/src/semaphore.hpp b/src/semaphore.hpp
@@ -15,6 +15,7 @@
 #pragma once
 
 #include "cl_headers.hpp"
+#include "context.hpp"
 #include "device.hpp"
 #include "icd.hpp"
 #include "objects.hpp"