[Beignet] [PATCH 2/2] runtime: fix image1d buffer allocation.

[Zhigang Gong]

Right, that's a potential risk. The spec is not very clear here,
especially the synchronization point. Based on my previous understanding,
after a buffer is accessed. The user need to call clEnqueueMapBuffer
again to get the valid pointer, and these type of APIs, clEnqueueMapBuffer
may be one of those synchronization point.

After offline discussion, we all agree to push this patch this time,
and I will add some comments there before I push it, Thanks for the review
comment.

On Fri, Jun 20, 2014 at 05:10:19PM +0800, He Junyan wrote:
> Spec says:
> 
> For a 1D image buffer
> object, the image pixels are taken from the buffer object’s data store.
> When the contents of a
> buffer object’s data store are modified, those changes are reflected in
> the contents of the 1D
> image buffer object and vice-versa at corresponding sychronization
> points. 
> 
> NOTE:
> Concurrent reading from, writing to and copying between both a buffer
> object and 1D image
> buffer object associated with the buffer object is undefined. Only
> reading from both a buffer
> object and 1D image buffer object associated with the buffer object is
> defined.
> 
> 
> So
> corresponding sychronization points seems very important.
> if the user hold the mapped buffer address, this may cause some problem.
> 
> 
> On 五, 2014-06-20 at 15:47 +0800, Zhigang Gong wrote:
> > 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.
> > 
> > Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
> > ---
> >  src/cl_mem.c | 73 ++++++++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 54 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/cl_mem.c b/src/cl_mem.c
> > index a1d3b25..b27e64a 100644
> > --- a/src/cl_mem.c
> > +++ b/src/cl_mem.c
> > @@ -480,6 +480,23 @@ error:
> >    goto exit;
> >  }
> >  
> > +void cl_mem_replace_buffer(cl_mem buffer, cl_buffer new_bo)
> > +{
> > +  cl_buffer_unreference(buffer->bo);
> > +  buffer->bo = new_bo;
> > +  cl_buffer_reference(new_bo);
> > +  if (buffer->type != CL_MEM_SUBBUFFER_TYPE)
> > +    return;
> > +
> > +  struct _cl_mem_buffer *it = ((struct _cl_mem_buffer*)buffer)->sub_next;
> > +  for( ; it != (struct _cl_mem_buffer*)buffer; it = it->sub_next)
> > +  {
> > +    cl_buffer_unreference(it->base.bo);
> > +    it->base.bo = new_bo;
> > +    cl_buffer_reference(new_bo);
> > +  }
> > +}
> > +
> >  void
> >  cl_mem_copy_image_region(const size_t *origin, const size_t *region,
> >                           void *dst, size_t dst_row_pitch, size_t dst_slice_pitch,
> > @@ -598,10 +615,12 @@ _cl_mem_new_image(cl_context ctx,
> >  
> >    if (UNLIKELY(w == 0)) DO_IMAGE_ERROR;
> >    if (UNLIKELY(h == 0 && (image_type != CL_MEM_OBJECT_IMAGE1D &&
> > -      image_type != CL_MEM_OBJECT_IMAGE1D_ARRAY)))
> > +      image_type != CL_MEM_OBJECT_IMAGE1D_ARRAY &&
> > +      image_type != CL_MEM_OBJECT_IMAGE1D_BUFFER)))
> >      DO_IMAGE_ERROR;
> >  
> > -  if (image_type == CL_MEM_OBJECT_IMAGE1D) {
> > +  if (image_type == CL_MEM_OBJECT_IMAGE1D ||
> > +      image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER) {
> >      size_t min_pitch = bpp * w;
> >      if (data && pitch == 0)
> >        pitch = min_pitch;
> > @@ -809,27 +828,43 @@ _cl_mem_new_image_from_buffer(cl_context ctx,
> >      merged_flags &= ~(CL_MEM_HOST_WRITE_ONLY|CL_MEM_HOST_READ_ONLY|CL_MEM_HOST_NO_ACCESS);
> >      merged_flags |= flags & (CL_MEM_HOST_WRITE_ONLY|CL_MEM_HOST_READ_ONLY|CL_MEM_HOST_NO_ACCESS);
> >    }
> > -
> > -  /* Because the buffer is NO_TILING, the image should be no tiling. */
> > -  image = cl_mem_allocate(CL_MEM_IMAGE_TYPE, ctx, flags, merged_flags, CL_FALSE, &err);
> > -  if (image == NULL || err != CL_SUCCESS)
> > -    goto error;
> > -
> > -  cl_buffer_reference(buffer->bo);
> > -  image->bo = buffer->bo;
> > -  image->size = buffer->size;
> > -  /* If it is a sub buffer, we need to start from the sub offset. */
> > +  struct _cl_mem_buffer *mem_buffer = (struct _cl_mem_buffer*)buffer;
> >    if (buffer->type == CL_MEM_SUBBUFFER_TYPE) {
> >      offset = ((struct _cl_mem_buffer *)buffer)->sub_offset;
> > +    mem_buffer = mem_buffer->parent;
> >    }
> > -  if (image->flags & CL_MEM_USE_HOST_PTR) {
> > -    /* Now point to the right offset if buffer is a SUB_BUFFER. */
> > -    image->host_ptr = buffer->host_ptr + offset;
> > -  }
> > +  /* Get the size of each pixel */
> > +  if (UNLIKELY((err = cl_image_byte_per_pixel(image_format, &bpp)) != CL_SUCCESS))
> > +    goto error;
> >  
> > -  cl_mem_image_init(cl_mem_image(image), image_desc->image_width, 1, image_desc->image_type,
> > -                    1, *image_format, intel_fmt, bpp, image_desc->image_width*bpp, 0, CL_NO_TILE,
> > -                    0, 0, offset);
> > +  // 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);
> > +  if (image == NULL)
> > +    return NULL;
> > +  void *src = cl_mem_map(buffer);
> > +  void *dst = cl_mem_map(image);
> > +  //
> > +  // 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.
> > +  memcpy(dst, src, mem_buffer->base.size);
> > +  cl_mem_unmap(buffer);
> > +  cl_mem_unmap(image);
> > +
> > +  if (err != 0)
> > +    goto error;
> > + 
> > +  // Now replace buffer's bo to this new bo, need to take care of sub buffer
> > +  // case. 
> > +  cl_mem_replace_buffer(buffer, image->bo);
> > +  /* Now point to the right offset if buffer is a SUB_BUFFER. */
> > +  if (buffer->flags & CL_MEM_USE_HOST_PTR)
> > +    image->host_ptr = buffer->host_ptr + offset;
> > +  cl_mem_image(image)->offset = offset;
> > +  cl_mem_image(image)->w = image_desc->image_width;
> >    cl_mem_add_ref(buffer);
> >    cl_mem_image(image)->buffer_1d = buffer;
> >    return image;
> 
> 
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet