[Mesa-dev] [PATCH RFC 1/1] clover: Don't crash on NULL global buffer objects.

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> Specs say it's legal.
> Fixes clones.xml gegl test.
>
> ---
> Hi,
>
> this patch is an attempt to support NULL buffer objects in clover.
> It adds NULL handling to few places, and it's enough to get 'clones'
> gegl test running.
>
> The 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."
>
> I was considering using a special buffer instance to represent
> NULL buffers but this seems more straightforward. Using 'pobj'
> should take care of the latter part of the spec requirement too.
>
> BZ: https://bugs.freedesktop.org/show_bug.cgi?id=73571#c2
> ignore the first two comments I forgot I was testing some
> additional patches.
>
> Tom, I'm not sure I understand your comment. Does it mean that at
> the moment it's possible that kernels receive NULL pointers
> that point to valid buffers?
>
> regards,
> Jan
>
>  src/gallium/drivers/r600/evergreen_compute.c      | 9 ++++++---
>  src/gallium/state_trackers/clover/core/kernel.cpp | 4 ++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/drivers/r600/evergreen_compute.c b/src/gallium/drivers/r600/evergreen_compute.c
> index a2db69b..f14b7c5 100644
> --- a/src/gallium/drivers/r600/evergreen_compute.c
> +++ b/src/gallium/drivers/r600/evergreen_compute.c
> @@ -655,10 +655,13 @@ static void evergreen_set_global_binding(
>  
>  	for (int i = 0; i < n; i++)
>  	{
> -		assert(resources[i]->target == PIPE_BUFFER);
> -		assert(resources[i]->bind & PIPE_BIND_GLOBAL);
> +		if (resources[i]) {
> +			assert(resources[i]->target == PIPE_BUFFER);
> +			assert(resources[i]->bind & PIPE_BIND_GLOBAL);
>  
> -		*(handles[i]) = buffers[i]->chunk->start_in_dw * 4;
> +			*(handles[i]) = buffers[i]->chunk->start_in_dw * 4;
> +		} else
> +			*(handles[i]) = NULL;
>  	}
>  
>  	evergreen_set_rat(ctx->cs_shader_state.shader, 0, pool->bo, 0, pool->size_in_dw * 4);

Can you split the r600 changes into a separate patch?

> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp b/src/gallium/state_trackers/clover/core/kernel.cpp
> index ac57c71..412eac4 100644
> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
> @@ -331,7 +331,7 @@ 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;
>  }
>  
> @@ -340,7 +340,7 @@ kernel::global_argument::bind(exec_context &ctx,
>                                const module::argument &marg) {
>     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);
> +   ctx.g_buffers.push_back(buf ? buf->resource(*ctx.q).pipe: NULL);
>  }
>  

Another possibility would be to write NULL to the input buffer here and
not push the handle/buffer pair at all.  That way drivers won't have to
deal with this special case.

You probably need to fix constant buffer arguments too.

Thanks.

>  void
> -- 
> 1.8.4.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140115/a3a70733/attachment.pgp>