[Beignet] [PATCH 1/2] [opencl-2.0] enable create image 2d from buffer in clCreateImage.

[He Junyan]

Some comments,

On 2015年04月03日 13:39, xionghu.luo at intel.com wrote:
> From: Luo Xionghu <xionghu.luo at intel.com>
>
> this patch allows create 2d image with a cl buffer.
>
> Signed-off-by: Luo Xionghu <xionghu.luo at intel.com>
> ---
>   src/cl_api.c |  3 ++-
>   src/cl_mem.c | 67 +++++++++++++++++++++++++++++++++++++++++++-----------------
>   2 files changed, 50 insertions(+), 20 deletions(-)
>
> diff --git a/src/cl_api.c b/src/cl_api.c
> index cd4020e..25e621a 100644
> --- a/src/cl_api.c
> +++ b/src/cl_api.c
> @@ -549,8 +549,9 @@ clCreateImage(cl_context context,
>       goto error;
>     }
>     /* buffer refers to a valid buffer memory object if image_type is
> -     CL_MEM_OBJECT_IMAGE1D_BUFFER. Otherwise it must be NULL. */
> +     CL_MEM_OBJECT_IMAGE1D_BUFFER or CL_MEM_OBJECT_IMAGE2D. Otherwise it must be NULL. */
>     if (image_desc->image_type != CL_MEM_OBJECT_IMAGE1D_BUFFER &&
> +      image_desc->image_type != CL_MEM_OBJECT_IMAGE2D &&
>            image_desc->buffer) {
>       err = CL_INVALID_IMAGE_DESCRIPTOR;
>       goto error;
> diff --git a/src/cl_mem.c b/src/cl_mem.c
> index b41ec14..3c5667e 100644
> --- a/src/cl_mem.c
> +++ b/src/cl_mem.c
> @@ -971,26 +971,47 @@ _cl_mem_new_image_from_buffer(cl_context ctx,
>     if (UNLIKELY((err = cl_image_byte_per_pixel(image_format, &bpp)) != CL_SUCCESS))
>       goto error;
>   
> -  // Per bspec, a image should has a at least 2 line vertical alignment,
> -  // thus we can't simply attach a buffer to a 1d image surface which has the same size.
> -  // We have to create a new image, and copy the buffer data to this new image.
> -  // And replace all the buffer object's reference to this image.
> -  image = _cl_mem_new_image(ctx, flags, image_format, image_desc->image_type,
> +  if(image_desc->image_type == CL_MEM_OBJECT_IMAGE2D) {
Spec says:
The restrictions are:
all the values specified in image_desc except for mem_object must match 
the image descriptor information associated with mem_object.
the channel data type specified in image_format must match the channel 
data type associated with mem_object.

So I think here we may need to add some check.

> +    image = _cl_mem_new_image(ctx, flags, image_format, image_desc->image_type,
> +                             image_desc->image_width, image_desc->image_height, image_desc->image_depth,
> +                             image_desc->image_row_pitch, image_desc->image_slice_pitch,
> +                             image_desc->buffer, errcode_ret);
                                   ~~~~~~~~~~~~~~~ here, why 
image_desc->buffer?
> +  } else if (image_desc->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER) {
> +    // Per bspec, a image should has a at least 2 line vertical alignment,
> +    // thus we can't simply attach a buffer to a 1d image surface which has the same size.
> +    // We have to create a new image, and copy the buffer data to this new image.
> +    // And replace all the buffer object's reference to this image.
> +    image = _cl_mem_new_image(ctx, flags, image_format, image_desc->image_type,
>                       mem_buffer->base.size / bpp, 0, 0, 0, 0, NULL, errcode_ret);
> +  }
> +  else
> +    assert(0);
> +
>     if (image == NULL)
>       return NULL;
> -  void *src = cl_mem_map(buffer, 0);
> -  void *dst = cl_mem_map(image, 1);
> -  //
> -  // FIXME, we could use copy buffer to image to do this on GPU latter.
> -  // currently the copy buffer to image function doesn't support 1D image.
> -  //
> -  // There is a potential risk that this buffer was mapped and the caller
> -  // still hold the pointer and want to access it again. This scenario is
> -  // not explicitly forbidden in the spec, although it should not be permitted.
> -  memcpy(dst, src, mem_buffer->base.size);
> -  cl_mem_unmap(buffer);
> -  cl_mem_unmap(image);
> +
> +  if(image_desc->image_type == CL_MEM_OBJECT_IMAGE2D)
> +  {
> +    size_t origin[] = {0,0,0};
> +    size_t region[] = {image_desc->image_width, image_desc->image_height, 1};
> +    clEnqueueCopyBufferToImage(ctx->queues, buffer, image, 0, origin, region, 0, NULL, NULL);
> +  }
> +  else if (image_desc->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER)
> +  {
> +    // FIXME, we could use copy buffer to image to do this on GPU latter.
> +    // currently the copy buffer to image function doesn't support 1D image.
> +    //
> +    // There is a potential risk that this buffer was mapped and the caller
> +    // still hold the pointer and want to access it again. This scenario is
> +    // not explicitly forbidden in the spec, although it should not be permitted.
> +    void *src = cl_mem_map(buffer, 0);
> +    void *dst = cl_mem_map(image, 1);
> +    memcpy(dst, src, mem_buffer->base.size);
> +    cl_mem_unmap(image);
> +    cl_mem_unmap(buffer);
> +  }
> +  else
> +    assert(0);
>   
>     if (err != 0)
>       goto error;
> @@ -1025,12 +1046,20 @@ cl_mem_new_image(cl_context context,
>   {
>     switch (image_desc->image_type) {
>     case CL_MEM_OBJECT_IMAGE1D:
> -  case CL_MEM_OBJECT_IMAGE2D:
>     case CL_MEM_OBJECT_IMAGE3D:
>       return _cl_mem_new_image(context, flags, image_format, image_desc->image_type,
>                                image_desc->image_width, image_desc->image_height, image_desc->image_depth,
>                                image_desc->image_row_pitch, image_desc->image_slice_pitch,
>                                host_ptr, errcode_ret);
> +  case CL_MEM_OBJECT_IMAGE2D:
> +    if(image_desc->buffer)
> +      return _cl_mem_new_image_from_buffer(context, flags, image_format,
> +                             image_desc, errcode_ret);
> +    else
> +      return _cl_mem_new_image(context, flags, image_format, image_desc->image_type,
> +                             image_desc->image_width, image_desc->image_height, image_desc->image_depth,
> +                             image_desc->image_row_pitch, image_desc->image_slice_pitch,
> +                             host_ptr, errcode_ret);
>     case CL_MEM_OBJECT_IMAGE1D_ARRAY:
>     case CL_MEM_OBJECT_IMAGE2D_ARRAY:
>       return _cl_mem_new_image(context, flags, image_format, image_desc->image_type,
> @@ -1065,7 +1094,7 @@ cl_mem_delete(cl_mem mem)
>     /* iff we are a image, delete the 1d buffer if has. */
>     if (IS_IMAGE(mem)) {
>       if (cl_mem_image(mem)->buffer_1d) {
> -      assert(cl_mem_image(mem)->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER);
> +      assert(cl_mem_image(mem)->image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER ||cl_mem_image(mem)->image_type == CL_MEM_OBJECT_IMAGE2D);
>         cl_mem_delete(cl_mem_image(mem)->buffer_1d);
>         cl_mem_image(mem)->buffer_1d = NULL;
>       }