Serge Martin <edb+m...@sigluy.net> writes:
> ---
> src/gallium/state_trackers/clover/api/memory.cpp | 117
> +++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp
> b/src/gallium/state_trackers/clover/api/memory.cpp
> index 1efb95b..6e95931 100644
> --- a/src/gallium/state_trackers/clover/api/memory.cpp
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> @@ -117,6 +117,113 @@ clCreateSubBuffer(cl_mem d_mem, cl_mem_flags d_flags,
> }
>
> CLOVER_API cl_mem
> +clCreateImage(cl_context d_ctx, cl_mem_flags d_flags,
> + const cl_image_format *format,
> + const cl_image_desc *desc,
> + void *host_ptr, cl_int *r_errcode) try {
> + auto &ctx = obj(d_ctx);
> +
> + if (!format)
> + throw error(CL_INVALID_IMAGE_FORMAT_DESCRIPTOR);
> +
> + if (!desc)
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> +
> + if (desc->image_array_size == 0 && (
> + (desc->image_type == CL_MEM_OBJECT_IMAGE1D_ARRAY) ||
Redundant parentheses.
> + (desc->image_type == CL_MEM_OBJECT_IMAGE2D_ARRAY)))
Same here.
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> +
> + if (!host_ptr &&
> + (desc->image_row_pitch || desc->image_slice_pitch))
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> +
> + if (desc->num_mip_levels || desc->num_samples)
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> +
> + if (desc->buffer && (desc->image_type !=
> + CL_MEM_OBJECT_IMAGE1D_BUFFER))
We should also make sure that desc->buffer is non-NULL for
CL_MEM_OBJECT_IMAGE1D_BUFFER, e.g.:
| if (bool(desc->buffer) != (desc->image_type ==
CL_MEM_OBJECT_IMAGE1D_BUFFER))
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> +
> + const cl_mem_flags flags = d_flags |
> + ((d_flags & dev_access_flags) &&
> + !(desc->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER) ?
> + 0 : CL_MEM_READ_WRITE);
> +
Weird indentation and redundant parentheses. And in the image1d buffer
case this should check that the specified memory flags are compatible
with the parent memory object, and if they are implement the required
logic to inherit only those flags of the parent which are left
unspecified. Apparently the spec wording is equivalent to the behaviour
of clCreateSubBuffer so we should probably share it. The attached patch
factors it out into validate_flags(), can you rebase this patch on top
of mine and give me your Reviewed/Tested-by if you like?
> + if (desc->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER) {
> + auto &buf = obj(desc->buffer);
> + validate_flags(flags | buf.flags(),
> + dev_access_flags | host_access_flags);
> + } else {
> + validate_flags(flags, all_mem_flags);
> + }
> +
If you rebase this on top of the attached patch this whole if-block will
become unnecessary, you can replace it with:
| const cl_mem_flags flags = validate_flags(desc->buffer, d_flags);
> + if (bool(host_ptr) != bool(flags & (CL_MEM_USE_HOST_PTR |
> + CL_MEM_COPY_HOST_PTR)))
> + throw error(CL_INVALID_HOST_PTR);
> +
> + if (!any_of(std::mem_fn(&device::image_support), ctx.devices()))
> + throw error(CL_INVALID_OPERATION);
> +
> + if (!supported_formats(ctx, desc->image_type).count(*format))
> + throw error(CL_IMAGE_FORMAT_NOT_SUPPORTED);
> +
> + cl_mem img = NULL;
Move the ret_error(r_errcode, CL_SUCCESS) call up here so you can get
rid of this declaration and replace the breaks by returns below.
> + switch (desc->image_type) {
> + case CL_MEM_OBJECT_IMAGE3D:
> + if (!desc->image_width || !desc->image_height || !desc->image_depth)
> + throw error(CL_INVALID_IMAGE_SIZE);
> +
> + if (any_of([=](device &dev) {
The spec implies that this should be all_of(), and the device argument
could be const.
> + const size_t max = 1 << dev.max_image_levels_3d();
> + return desc->image_width > max ||
> + desc->image_height > max ||
> + desc->image_depth > max;
> + }, ctx.devices()))
> + throw error(CL_INVALID_IMAGE_SIZE);
> +
> + img = new image3d(ctx, flags, format,
> + desc->image_width, desc->image_height,
> + desc->image_depth, desc->image_row_pitch,
> + desc->image_slice_pitch, host_ptr);
> + break;
> +
> + case CL_MEM_OBJECT_IMAGE2D:
> + if (!desc->image_width || !desc->image_height)
> + throw error(CL_INVALID_IMAGE_SIZE);
> +
> + if (any_of([=](device &dev) {
Same here.
> + const size_t max = 1 << dev.max_image_levels_2d();
> + return desc->image_width > max ||
> + desc->image_height > max;
> + }, ctx.devices()))
> + throw error(CL_INVALID_IMAGE_SIZE);
> +
> + img = new image2d(ctx, flags, format,
> + desc->image_width, desc->image_height,
> + desc->image_row_pitch, host_ptr);
> + break;
> +
> + case CL_MEM_OBJECT_IMAGE2D_ARRAY:
> + case CL_MEM_OBJECT_IMAGE1D_ARRAY:
> + case CL_MEM_OBJECT_IMAGE1D_BUFFER:
> + case CL_MEM_OBJECT_IMAGE1D:
You could add a comment here like:
| // XXX - Not implemented.
> + throw error(CL_IMAGE_FORMAT_NOT_SUPPORTED);
> + break;
Redundant break.
> +
> + default:
> + throw error(CL_INVALID_IMAGE_DESCRIPTOR);
> + }
> +
> + ret_error(r_errcode, CL_SUCCESS);
> + return img;
> +
> +} catch (error &e) {
> + ret_error(r_errcode, e);
> + return NULL;
> +}
> +
> +CLOVER_API cl_mem
> clCreateImage2D(cl_context d_ctx, cl_mem_flags d_flags,
> const cl_image_format *format,
> size_t width, size_t height, size_t row_pitch,
> @@ -352,16 +459,6 @@ clSetMemObjectDestructorCallback(cl_mem d_mem,
> return e.get();
> }
>
> -CLOVER_API cl_mem
> -clCreateImage(cl_context d_ctx, cl_mem_flags flags,
> - const cl_image_format *format,
> - const cl_image_desc *image_desc,
> - void *host_ptr, cl_int *r_errcode) {
> - CLOVER_NOT_SUPPORTED_UNTIL("1.2");
> - ret_error(r_errcode, CL_INVALID_OPERATION);
> - return NULL;
> -}
> -
> CLOVER_API cl_int
> clEnqueueFillBuffer(cl_command_queue command_queue, cl_mem buffer,
> const void *pattern, size_t pattern_size,
> --
> 2.5.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
From d43368d708070b0f43d82be6cbc6b4d4865725d8 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Fri, 18 Sep 2015 18:42:55 +0200
Subject: [PATCH] clover: Move down canonicalization of memory object flags
into validate_flags().
This will be used to share the same logic between buffer and image
creation.
---
src/gallium/state_trackers/clover/api/memory.cpp | 68 ++++++++++++------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
index 1efb95b..e8ae5b3 100644
--- a/src/gallium/state_trackers/clover/api/memory.cpp
+++ b/src/gallium/state_trackers/clover/api/memory.cpp
@@ -34,31 +34,48 @@ namespace {
CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | CL_MEM_COPY_HOST_PTR;
const cl_mem_flags host_access_flags =
CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS;
- const cl_mem_flags all_mem_flags =
- dev_access_flags | host_ptr_flags | host_access_flags;
-
- void
- validate_flags(cl_mem_flags flags, cl_mem_flags valid) {
- if ((flags & ~valid) ||
- util_bitcount(flags & dev_access_flags) > 1 ||
- util_bitcount(flags & host_access_flags) > 1)
+
+ cl_mem_flags
+ validate_flags(cl_mem d_parent, cl_mem_flags d_flags) {
+ const cl_mem_flags valid = (dev_access_flags | host_access_flags |
+ (d_parent ? 0 : host_ptr_flags));
+
+ if ((d_flags & ~valid) ||
+ util_bitcount(d_flags & dev_access_flags) > 1 ||
+ util_bitcount(d_flags & host_access_flags) > 1)
throw error(CL_INVALID_VALUE);
- if ((flags & CL_MEM_USE_HOST_PTR) &&
- (flags & (CL_MEM_COPY_HOST_PTR | CL_MEM_ALLOC_HOST_PTR)))
+ if ((d_flags & CL_MEM_USE_HOST_PTR) &&
+ (d_flags & (CL_MEM_COPY_HOST_PTR | CL_MEM_ALLOC_HOST_PTR)))
throw error(CL_INVALID_VALUE);
+
+ if (d_parent) {
+ const auto &parent = obj(d_parent);
+ const cl_mem_flags flags = (d_flags |
+ (d_flags & dev_access_flags ? 0 :
+ parent.flags() & dev_access_flags) |
+ (d_flags & host_access_flags ? 0 :
+ parent.flags() & host_access_flags) |
+ (parent.flags() & host_ptr_flags));
+
+ if (~flags & parent.flags() &
+ ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
+ throw error(CL_INVALID_VALUE);
+
+ return flags;
+
+ } else {
+ return d_flags | (d_flags & dev_access_flags ? 0 : CL_MEM_READ_WRITE);
+ }
}
}
CLOVER_API cl_mem
clCreateBuffer(cl_context d_ctx, cl_mem_flags d_flags, size_t size,
void *host_ptr, cl_int *r_errcode) try {
- const cl_mem_flags flags = d_flags |
- (d_flags & dev_access_flags ? 0 : CL_MEM_READ_WRITE);
+ const cl_mem_flags flags = validate_flags(NULL, d_flags);
auto &ctx = obj(d_ctx);
- validate_flags(d_flags, all_mem_flags);
-
if (bool(host_ptr) != bool(flags & (CL_MEM_USE_HOST_PTR |
CL_MEM_COPY_HOST_PTR)))
throw error(CL_INVALID_HOST_PTR);
@@ -82,16 +99,7 @@ clCreateSubBuffer(cl_mem d_mem, cl_mem_flags d_flags,
cl_buffer_create_type op,
const void *op_info, cl_int *r_errcode) try {
auto &parent = obj<root_buffer>(d_mem);
- const cl_mem_flags flags = d_flags |
- (d_flags & dev_access_flags ? 0 : parent.flags() & dev_access_flags) |
- (d_flags & host_access_flags ? 0 : parent.flags() & host_access_flags) |
- (parent.flags() & host_ptr_flags);
-
- validate_flags(d_flags, dev_access_flags | host_access_flags);
-
- if (~flags & parent.flags() &
- ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
- throw error(CL_INVALID_VALUE);
+ const cl_mem_flags flags = validate_flags(d_mem, d_flags);
if (op == CL_BUFFER_CREATE_TYPE_REGION) {
auto reg = reinterpret_cast<const cl_buffer_region *>(op_info);
@@ -121,12 +129,9 @@ clCreateImage2D(cl_context d_ctx, cl_mem_flags d_flags,
const cl_image_format *format,
size_t width, size_t height, size_t row_pitch,
void *host_ptr, cl_int *r_errcode) try {
- const cl_mem_flags flags = d_flags |
- (d_flags & dev_access_flags ? 0 : CL_MEM_READ_WRITE);
+ const cl_mem_flags flags = validate_flags(NULL, d_flags);
auto &ctx = obj(d_ctx);
- validate_flags(d_flags, all_mem_flags);
-
if (!any_of(std::mem_fn(&device::image_support), ctx.devices()))
throw error(CL_INVALID_OPERATION);
@@ -158,12 +163,9 @@ clCreateImage3D(cl_context d_ctx, cl_mem_flags d_flags,
size_t width, size_t height, size_t depth,
size_t row_pitch, size_t slice_pitch,
void *host_ptr, cl_int *r_errcode) try {
- const cl_mem_flags flags = d_flags |
- (d_flags & dev_access_flags ? 0 : CL_MEM_READ_WRITE);
+ const cl_mem_flags flags = validate_flags(NULL, d_flags);
auto &ctx = obj(d_ctx);
- validate_flags(d_flags, all_mem_flags);
-
if (!any_of(std::mem_fn(&device::image_support), ctx.devices()))
throw error(CL_INVALID_OPERATION);
@@ -196,7 +198,7 @@ clGetSupportedImageFormats(cl_context d_ctx, cl_mem_flags flags,
auto &ctx = obj(d_ctx);
auto formats = supported_formats(ctx, type);
- validate_flags(flags, all_mem_flags);
+ validate_flags(NULL, flags);
if (r_buf && !r_count)
throw error(CL_INVALID_VALUE);
--
2.5.1
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
