[Mesa-dev] [PATCH 1/2] clover: implement clCreateImage
Francisco Jerez
currojerez at riseup.net
Fri Sep 18 10:31:00 PDT 2015
Serge Martin <edb+mesa at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-clover-Move-down-canonicalization-of-memory-object-f.patch
Type: text/x-diff
Size: 5694 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150918/94d8f295/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150918/94d8f295/attachment-0001.sig>
More information about the mesa-dev
mailing list