Jan Vesely <jan.ves...@rutgers.edu> writes: > Specs say "If the argument is a buffer object, the arg_value > pointer can be NULL or point to a NULL value in which case a NULL > value will be used as the value for the argument declared as a > pointer to __global or __constant memory in the kernel." > > So don't crash when somebody does that. > > v2: Insert NULL into input buffer instead of buffer handle pair > Fix constant_argument too > Drop r600 driver changes > > Signed-off-by: Jan Vesely <jan.ves...@rutgers.edu> > --- > Hi Francisco, > Hi Jan,
> I could not find much info about how ctx.input is supposed to be used.
> It looks like it stores begin-end intervals of parameters so NULL, NULL
> seemed appropriate.
>
It's just an array of bytes that is passed to the driver as input for
the kernel. Some suggestions more below.
> Is this closer to what you had in mind? This patch fixes my issue
> even without the r600 changes.
>
> regards,
> Jan
>
> src/gallium/state_trackers/clover/core/kernel.cpp | 34
> +++++++++++++----------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
> b/src/gallium/state_trackers/clover/core/kernel.cpp
> index 58780d6..d4942b3 100644
> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
> @@ -327,16 +327,19 @@ kernel::global_argument::set(size_t size, const void
> *value) {
> if (size != sizeof(cl_mem))
> throw error(CL_INVALID_ARG_SIZE);
>
> - buf = &obj<buffer>(*(cl_mem *)value);
> + buf = pobj<buffer>(value ? *(cl_mem *)value : NULL);
> _set = true;
> }
>
> void
> kernel::global_argument::bind(exec_context &ctx,
> const module::argument &marg) {
> - align(ctx.input, marg.target_align);
As null pointers have the same alignment restrictions as normal pointers,
you should keep this line here before the 'if (buf)' conditional.
> - ctx.g_handles.push_back(allocate(ctx.input, marg.target_size));
> - ctx.g_buffers.push_back(buf->resource(*ctx.q).pipe);
> + if (buf) {
> + align(ctx.input, marg.target_align);
> + ctx.g_handles.push_back(allocate(ctx.input, marg.target_size));
> + ctx.g_buffers.push_back(buf->resource(*ctx.q).pipe);
> + } else
For consistency use braces around the else block of an if block that
uses braces.
> + insert(ctx.input, {NULL, NULL});
I don't think this does what you want. The easiest way to append zeroes
to the input buffer is:
| } else {
| // Null global pointer.
| allocate(ctx.input, marg.target_size);
| }
> }
>
> void
> @@ -379,22 +382,25 @@ kernel::constant_argument::set(size_t size, const void
> *value) {
> if (size != sizeof(cl_mem))
> throw error(CL_INVALID_ARG_SIZE);
>
> - buf = &obj<buffer>(*(cl_mem *)value);
> + buf = pobj<buffer>(value ? *(cl_mem *)value : NULL);
> _set = true;
> }
>
> void
> kernel::constant_argument::bind(exec_context &ctx,
> const module::argument &marg) {
> - auto v = bytes(ctx.resources.size() << 24);
> -
> - extend(v, module::argument::zero_ext, marg.target_size);
> - byteswap(v, ctx.q->dev.endianness());
> - align(ctx.input, marg.target_align);
> - insert(ctx.input, v);
> -
> - st = buf->resource(*ctx.q).bind_surface(*ctx.q, false);
> - ctx.resources.push_back(st);
> + if (buf) {
> + auto v = bytes(ctx.resources.size() << 24);
> +
> + extend(v, module::argument::zero_ext, marg.target_size);
> + byteswap(v, ctx.q->dev.endianness());
> + align(ctx.input, marg.target_align);
This align() call should be kept outside the conditional too.
> + insert(ctx.input, v);
> +
> + st = buf->resource(*ctx.q).bind_surface(*ctx.q, false);
> + ctx.resources.push_back(st);
> + } else
> + insert(ctx.input, {NULL, NULL});
Same comment as before.
Thank you.
> }
>
> void
> --
> 1.8.4.2
pgpl9vQRIC4cp.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
