On Wed, 2014-01-15 at 18:41 +0100, Francisco Jerez wrote: > Jan Vesely <[email protected]> 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 <[email protected]> > > --- > > 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.
Like an image of stack-passed parameters?
So inserting two NULLs produced two 0 bytes, and the only reason it
worked was because of the alignment for the next argument?
thanks,
Jan
>
> > 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
--
Jan Vesely <[email protected]>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
