[Mesa-dev] [PATCH 1/5] clover/memory: Copy data when creating buffers with CL_MEM_USE_HOST_PTR

[Aaron Watry]

/

On Sat, Jul 22, 2017 at 2:27 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> On Fri, 2017-07-21 at 23:19 -0500, Aaron Watry wrote:
>> Fixes: OpenCL CTS test/conformance/buffers/buffer_copy
>
> Similar patch was pushed in 2013:
> 56647c5d8f8e60269f0a3277e3caa7ee57d1fe6a
> "clover: Append buffers that use CL_MEM_USE_HOST_PTR."
>
> Grigory(added to cc) reverted the change and implemented user_ptr
> mechanism in:
> f972b223c4cb4ec58a9451cbac5d120ac9deb336
> "clover: try userptr for CL_MEM_USE_HOST_PTR"
>
> The buffer-flags piglit still passes, but it maps even CL_USE_HOST_PTR
> buffers. I'm not sure what the CTS does, we might need a
> synchronization point after kernels finish execution. I couldn't find
> the relevant part of specs that would define accessing
> CL_MEM_USE_HOST_PTR buffers without
> clEnqueueMapBuffer or clEnqueueReadBuffer
>

I got rid of this patch in my tree and then started digging into the
user_ptr stuff.

It looks like the issue stems from alignment restrictions.

In clover/core/resource.cpp, there's a call to:
  dev.pipe->resource_from_user_memory(dev.pipe, &info, obj.host_ptr())

That is failing for me.

The CTS allocates memory that is aligned to the value returned by
clGetDeviceInfo (device,CL_DEVICE_MEM_BASE_ADDR_ALIGN, ...) converted
to bytes.

Clover's api/device.cpp  has that hard-coded to 1024 bits/128 bytes at
the moment.

If I change the device info query for the mem_base_addr_align to
forward a call to:
  pipe->get_param(pipe, PIPE_CAP_MIN_MAP_BUFFER_ALIGNMENT)

And also change si_pipe.c:si_get_param's switch statement value to return:
  case PIPE_CAP_MIN_MAP_BUFFER_ALIGNMENT:
    return sscreen->b.info.gart_page_size;

Then I can successfully create buffers from user pointers on my SI card.

I'm a bit fuzzy on what alignment restrictions exist for SI/GCN cards,
but the winsys seems to indicate we should align things to gart page
size, which makes sense on the surface at least.

If the alignment restrictions have changed between R600 and GCN, that
might explain why what's broken for me is working for you/Grigori (on
r600).

--Aaron

>
> Jan
>
>
>>
>> Signed-off-by: Aaron Watry <awatry at gmail.com>
>> CC: Francisco Jerez <currojerez at riseup.net>
>> ---
>>  src/gallium/state_trackers/clover/core/memory.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/core/memory.cpp b/src/gallium/state_trackers/clover/core/memory.cpp
>> index b852e6896f..912d74830a 100644
>> --- a/src/gallium/state_trackers/clover/core/memory.cpp
>> +++ b/src/gallium/state_trackers/clover/core/memory.cpp
>> @@ -30,7 +30,7 @@ memory_obj::memory_obj(clover::context &ctx, cl_mem_flags flags,
>>                         size_t size, void *host_ptr) :
>>     context(ctx), _flags(flags),
>>     _size(size), _host_ptr(host_ptr) {
>> -   if (flags & CL_MEM_COPY_HOST_PTR)
>> +   if (flags & (CL_MEM_COPY_HOST_PTR | CL_MEM_USE_HOST_PTR))
>>        data.append((char *)host_ptr, size);
>>  }
>>
>
> --
> Jan Vesely <jan.vesely at rutgers.edu>