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

[Jan Vesely]

On Thu, 2017-08-03 at 00:28 -0500, Aaron Watry wrote:
> /
> 
> 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)

I'd say we should be doing this anyway instead of hardcoding the value.

> 
> 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;

I'm not sure what the correct value is here. AFAIK, EG uses 256B cache
lines so I'd expect the value of to be at least that

Both NI and GCN should be able to use 4K pages (which is what
gart_page_size is set to), but we might want higher alignment for
better performance[0]

[0]https://lists.freedesktop.org/archives/dri-devel/2014-May/058858.htm
l
> 
> 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).

I remember there was a buffer alignment patch form AMD recently for
SI/CI vs. VI+, but I can't find it.
It looks like a separate issue however. if incorrect alignment makes
user_ptr fail, and the test still fails, it looks like the no-user_ptr
fallback is broken.

Jan

> 
> --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>
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170803/2982d0de/attachment.sig>