Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix support for read of 3D images with unnormalised sampler #614

Merged
merged 2 commits into from
Oct 29, 2023
Merged

fix support for read of 3D images with unnormalised sampler #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d707806
fix support for read of 3D images with unnormalised sampler
rjodinchr Sep 7, 2023
f6d0893
Merge branch 'main' into dev/sampler-arg
kpet Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion src/kernel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include <algorithm>

#include "clspv/Sampler.h"

#include "kernel.hpp"
#include "memory.hpp"

Expand Down Expand Up @@ -44,6 +46,10 @@ cl_int cvk_kernel::init() {
m_image_metadata = md;
}

if (const auto* md = m_entry_point->sampler_metadata()) {
m_sampler_metadata = md;
}

// Init argument values
m_argument_values = cvk_kernel_argument_values::create(m_entry_point);
if (m_argument_values == nullptr) {
Expand Down Expand Up @@ -100,6 +106,45 @@ void cvk_kernel::set_image_metadata(cl_uint index, const void* image) {
}
}

void cvk_kernel::set_sampler_metadata(cl_uint index, const void* sampler) {
if (!m_sampler_metadata) {
return;
}
auto md = m_sampler_metadata->find(index);
if (md != m_sampler_metadata->end()) {
auto apisampler = *reinterpret_cast<const cl_sampler*>(sampler);
auto offset = md->second;
auto sampler = icd_downcast(apisampler);
uint32_t sampler_mask = (sampler->normalized_coords()
? clspv::CLK_NORMALIZED_COORDS_TRUE
: clspv::CLK_NORMALIZED_COORDS_FALSE) |
(sampler->filter_mode() == CL_FILTER_NEAREST
? clspv::CLK_FILTER_NEAREST
: clspv::CLK_FILTER_LINEAR);
rjodinchr marked this conversation as resolved.
Show resolved Hide resolved
switch (sampler->addressing_mode()) {
case CL_ADDRESS_NONE:
sampler_mask |= clspv::CLK_ADDRESS_NONE;
break;
case CL_ADDRESS_CLAMP:
sampler_mask |= clspv::CLK_ADDRESS_CLAMP;
break;
case CL_ADDRESS_REPEAT:
sampler_mask |= clspv::CLK_ADDRESS_REPEAT;
break;
case CL_ADDRESS_CLAMP_TO_EDGE:
sampler_mask |= clspv::CLK_ADDRESS_CLAMP_TO_EDGE;
break;
case CL_ADDRESS_MIRRORED_REPEAT:
sampler_mask |= clspv::CLK_ADDRESS_MIRRORED_REPEAT;
break;
default:
break;
}
m_argument_values->set_pod_data(offset, sizeof(sampler_mask),
&sampler_mask);
}
}

cl_int cvk_kernel::set_arg(cl_uint index, size_t size, const void* value) {
std::lock_guard<std::mutex> lock(m_lock);

Expand All @@ -125,6 +170,10 @@ cl_int cvk_kernel::set_arg(cl_uint index, size_t size, const void* value) {
set_image_metadata(index, value);
}

if (arg.kind == kernel_argument_kind::sampler) {
set_sampler_metadata(index, value);
}

return ret;
}

Expand Down Expand Up @@ -268,7 +317,20 @@ bool cvk_kernel_argument_values::setup_descriptor_sets() {
}
case kernel_argument_kind::sampler: {
auto clsampler = static_cast<cvk_sampler*>(get_arg_value(arg));
auto sampler = clsampler->vulkan_sampler();
bool normalized_coord_sampler_required = false;
if (auto md = m_entry_point->sampler_metadata()) {
normalized_coord_sampler_required = md->find(i) != md->end();
}
auto sampler =
normalized_coord_sampler_required &&
!clsampler->normalized_coords()
? clsampler
->get_or_create_vulkan_sampler_with_normalized_coords()
: clsampler->vulkan_sampler();
if (sampler == VK_NULL_HANDLE) {
cvk_error_fn("Could not set descriptor for sampler");
return false;
}

cvk_debug_fn("sampler %p @ set = %u, binding = %u", sampler,
arg.descriptorSet, arg.binding);
Expand Down
13 changes: 11 additions & 2 deletions src/kernel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ struct cvk_kernel : public _cl_kernel, api_object<object_magic::kernel> {

cvk_kernel(cvk_program* program, const char* name)
: api_object(program->context()), m_program(program),
m_entry_point(nullptr), m_name(name), m_image_metadata(nullptr) {}
m_entry_point(nullptr), m_name(name), m_sampler_metadata(nullptr),
m_image_metadata(nullptr) {}

CHECK_RETURN cl_int init();
std::unique_ptr<cvk_kernel> clone(cl_int* errcode_ret) const;
Expand All @@ -42,10 +43,16 @@ struct cvk_kernel : public _cl_kernel, api_object<object_magic::kernel> {
return m_argument_values;
}

const kernel_sampler_metadata_map* get_sampler_metadata() const {
return m_sampler_metadata;
}

const kernel_image_metadata_map* get_image_metadata() const {
return m_image_metadata;
}

void set_sampler_metadata(cl_uint index, const void* sampler);

void set_image_metadata(cl_uint index, const void* image);

CHECK_RETURN cl_int set_arg(cl_uint index, size_t size, const void* value);
Expand Down Expand Up @@ -158,6 +165,7 @@ struct cvk_kernel : public _cl_kernel, api_object<object_magic::kernel> {
std::string m_name;
std::vector<kernel_argument> m_args;
std::shared_ptr<cvk_kernel_argument_values> m_argument_values;
const kernel_sampler_metadata_map* m_sampler_metadata;
const kernel_image_metadata_map* m_image_metadata;
};

Expand Down Expand Up @@ -237,7 +245,8 @@ struct cvk_kernel_argument_values {
}

if (m_entry_point->has_pod_arguments() ||
m_entry_point->has_image_metadata()) {
m_entry_point->has_image_metadata() ||
m_entry_point->has_sampler_metadata()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really time we refactor push constant management to make it more readable. In a lot of places, they're still named "POD". Best done as a standalone changes.

// TODO(#101): host out-of-memory errors are currently unhandled.
auto buffer = std::make_unique<std::vector<uint8_t>>(
m_entry_point->pod_buffer_size());
Expand Down
8 changes: 5 additions & 3 deletions src/memory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ cvk_sampler::create(cvk_context* context, bool normalized_coords,
return sampler.release();
}

bool cvk_sampler::init() {
bool cvk_sampler::init(bool force_normalized_coordinates) {
auto vkdev = context()->device()->vulkan_device();

// Translate addressing mode
Expand Down Expand Up @@ -199,7 +199,7 @@ bool cvk_sampler::init() {

// Translate coordinate type
VkBool32 unnormalized_coordinates;
if (m_normalized_coords) {
if (m_normalized_coords || force_normalized_coordinates) {
unnormalized_coordinates = VK_FALSE;
} else {
unnormalized_coordinates = VK_TRUE;
Expand Down Expand Up @@ -235,7 +235,9 @@ bool cvk_sampler::init() {
unnormalized_coordinates, // unnormalizedCoordinates
};

auto res = vkCreateSampler(vkdev, &create_info, nullptr, &m_sampler);
VkSampler* sampler =
force_normalized_coordinates ? &m_sampler_norm : &m_sampler;
auto res = vkCreateSampler(vkdev, &create_info, nullptr, sampler);

return (res == VK_SUCCESS);
}
Expand Down
19 changes: 16 additions & 3 deletions src/memory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,13 +431,17 @@ struct cvk_sampler : public _cl_sampler, api_object<object_magic::sampler> {
std::vector<cl_sampler_properties>&& properties)
: api_object(context), m_normalized_coords(normalized_coords),
m_addressing_mode(addressing_mode), m_filter_mode(filter_mode),
m_properties(std::move(properties)), m_sampler(VK_NULL_HANDLE) {}
m_properties(std::move(properties)), m_sampler(VK_NULL_HANDLE),
m_sampler_norm(VK_NULL_HANDLE) {}

~cvk_sampler() {
auto vkdev = context()->device()->vulkan_device();
if (m_sampler != VK_NULL_HANDLE) {
auto vkdev = context()->device()->vulkan_device();
vkDestroySampler(vkdev, m_sampler, nullptr);
}
if (m_sampler_norm != VK_NULL_HANDLE) {
vkDestroySampler(vkdev, m_sampler_norm, nullptr);
}
}

static cvk_sampler* create(cvk_context* context, bool normalized_coords,
Expand All @@ -456,17 +460,26 @@ struct cvk_sampler : public _cl_sampler, api_object<object_magic::sampler> {
cl_addressing_mode addressing_mode() const { return m_addressing_mode; }
cl_filter_mode filter_mode() const { return m_filter_mode; }
VkSampler vulkan_sampler() const { return m_sampler; }
VkSampler get_or_create_vulkan_sampler_with_normalized_coords() {
if (m_sampler_norm == VK_NULL_HANDLE) {
if (!init(true)) {
return VK_NULL_HANDLE;
}
}
rjodinchr marked this conversation as resolved.
Show resolved Hide resolved
return m_sampler_norm;
}
const std::vector<cl_sampler_properties>& properties() const {
return m_properties;
}

private:
bool init();
bool init(bool force_normalized_coordinates = false);
bool m_normalized_coords;
cl_addressing_mode m_addressing_mode;
cl_filter_mode m_filter_mode;
const std::vector<cl_sampler_properties> m_properties;
VkSampler m_sampler;
VkSampler m_sampler_norm;
};

static inline cvk_sampler* icd_downcast(cl_sampler sampler) {
Expand Down
82 changes: 59 additions & 23 deletions src/program.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ spv_result_t parse_reflection(void* user_data,
return pushconstant::module_constants_pointer;
case NonSemanticClspvReflectionPrintfBufferPointerPushConstant:
return pushconstant::printf_buffer_pointer;
case NonSemanticClspvReflectionNormalizedSamplerMaskPushConstant:
return pushconstant::normalized_sampler_mask;
default:
cvk_error_fn("Unhandled reflection instruction for push constant");
break;
Expand Down Expand Up @@ -212,6 +214,17 @@ spv_result_t parse_reflection(void* user_data,
parse_data->arg_infos[inst->result_id] = info;
break;
}
case NonSemanticClspvReflectionNormalizedSamplerMaskPushConstant: {
auto kernel = parse_data->strings[inst->words[5]];
auto ordinal = parse_data->constants[inst->words[6]];
auto offset = parse_data->constants[inst->words[7]];
auto size = parse_data->constants[inst->words[8]];
parse_data->binary->add_sampler_metadata(kernel, ordinal,
offset);
auto pc = inst_to_push_constant(ext_inst);
parse_data->binary->add_push_constant(pc, {offset, size});
break;
}
case NonSemanticClspvReflectionImageArgumentInfoChannelOrderPushConstant: {
auto kernel = parse_data->strings[inst->words[5]];
auto ordinal = parse_data->constants[inst->words[6]];
Expand Down Expand Up @@ -1674,8 +1687,9 @@ cvk_entry_point::cvk_entry_point(VkDevice dev, cvk_program* program,
: m_device(dev), m_context(program->context()), m_program(program),
m_name(name), m_pod_descriptor_type(VK_DESCRIPTOR_TYPE_MAX_ENUM),
m_pod_buffer_size(0u), m_has_pod_arguments(false),
m_has_pod_buffer_arguments(false), m_image_metadata(nullptr),
m_descriptor_pool(VK_NULL_HANDLE), m_pipeline_layout(VK_NULL_HANDLE) {}
m_has_pod_buffer_arguments(false), m_sampler_metadata(nullptr),
m_image_metadata(nullptr), m_descriptor_pool(VK_NULL_HANDLE),
m_pipeline_layout(VK_NULL_HANDLE) {}

cvk_entry_point* cvk_program::get_entry_point(std::string& name,
cl_int* errcode_ret) {
Expand Down Expand Up @@ -1895,6 +1909,11 @@ cl_int cvk_entry_point::init() {
m_image_metadata = md;
}

// Get the sampler metadata for this entry point
if (auto* md = m_program->sampler_metadata(m_name)) {
m_sampler_metadata = md;
}

// Get a pointer to the arguments from the program
auto args = m_program->args_for_kernel(m_name);

Expand Down Expand Up @@ -1975,32 +1994,49 @@ cl_int cvk_entry_point::init() {
m_pod_buffer_size = round_up(m_pod_buffer_size, 4);
}

// Take the size of image metadata into account for the pod buffer size
if (m_image_metadata) {
// Find how big the POD buffer should be
// Take the size of image & sampler metadata into account for the pod buffer
// size
{
uint32_t max_offset = 0;
for (const auto& md : *m_image_metadata) {
auto order_offset = md.second.order_offset;
auto data_type_offset = md.second.data_type_offset;
if (md.second.has_valid_order()) {
max_offset = std::max(order_offset, max_offset);
push_constant_range.offset =
std::min(order_offset, push_constant_range.offset);
if (order_offset + sizeof(uint32_t) >
push_constant_range.offset + push_constant_range.size) {
push_constant_range.size = order_offset + sizeof(uint32_t) -
push_constant_range.offset;
if (m_image_metadata) {
// Find how big the POD buffer should be
for (const auto& md : *m_image_metadata) {
auto order_offset = md.second.order_offset;
auto data_type_offset = md.second.data_type_offset;
if (md.second.has_valid_order()) {
max_offset = std::max(order_offset, max_offset);
push_constant_range.offset =
std::min(order_offset, push_constant_range.offset);
if (order_offset + sizeof(uint32_t) >
push_constant_range.offset + push_constant_range.size) {
push_constant_range.size = order_offset +
sizeof(uint32_t) -
push_constant_range.offset;
}
}
if (md.second.has_valid_data_type()) {
max_offset = std::max(data_type_offset, max_offset);
push_constant_range.offset =
std::min(data_type_offset, push_constant_range.offset);
if (data_type_offset + sizeof(uint32_t) >
push_constant_range.offset + push_constant_range.size) {
push_constant_range.size = data_type_offset +
sizeof(uint32_t) -
push_constant_range.offset;
}
}
}
if (md.second.has_valid_data_type()) {
max_offset = std::max(data_type_offset, max_offset);
}
if (m_sampler_metadata) {
for (const auto& md : *m_sampler_metadata) {
auto offset = md.second;
max_offset = std::max(offset, max_offset);
push_constant_range.offset =
std::min(data_type_offset, push_constant_range.offset);
if (data_type_offset + sizeof(uint32_t) >
std::min(offset, push_constant_range.offset);
if (offset + sizeof(uint32_t) >
push_constant_range.offset + push_constant_range.size) {
push_constant_range.size = data_type_offset +
sizeof(uint32_t) -
push_constant_range.offset;
push_constant_range.size =
offset + sizeof(uint32_t) - push_constant_range.offset;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is now screaming "refactor me" :). To be done as a standalone change.

}
}
}
Expand Down
Loading
Loading