[Mesa-dev] [PATCH v2 09/24] anv: avoid crashes when failing to allocate batches
Pohjolainen, Topi
topi.pohjolainen at gmail.com
Mon Mar 13 09:30:20 UTC 2017
On Fri, Mar 10, 2017 at 01:38:22PM +0100, Iago Toral Quiroga wrote:
> Most of the time we use macros that handle this situation transparently,
> but there are some cases were we need to handle this explicitly.
>
> This patch makes sure we don't crash, notice that error handling takes
> place in the function that actually failed the allocation,
> anv_batch_emit_dwords(), which will set the status field of the batch
> so it can be used at a later moment to report the error to the user.
>
> v2:
> - Not crashing is not good enough, we need to keep track of the error
> (Topi, Jason). Iago: now that we track errors in the batch, this
> is being handled.
> - Added guards in a few more places that needed it (Iago)
> ---
> src/intel/blorp/blorp_genX_exec.h | 25 +++++++++++++++++--------
> src/intel/vulkan/anv_private.h | 22 +++++++++++++---------
> src/intel/vulkan/genX_pipeline.c | 4 ++++
Ganv_batch_emitn() gives me also emit_vertex_input() which calls memset()
against the pointer gotten from anv_batch_emitn(). Can you check that?
Otherwise this looks good to me:
Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_genX_exec.h b/src/intel/blorp/blorp_genX_exec.h
> index f0c4f38..dc1b773 100644
> --- a/src/intel/blorp/blorp_genX_exec.h
> +++ b/src/intel/blorp/blorp_genX_exec.h
> @@ -110,14 +110,16 @@ _blorp_combine_address(struct blorp_batch *batch, void *location,
> _blorp_cmd_pack(cmd)(batch, (void *)_dst, &name), \
> _dst = NULL)
>
> -#define blorp_emitn(batch, cmd, n) ({ \
> - uint32_t *_dw = blorp_emit_dwords(batch, n); \
> - struct cmd template = { \
> - _blorp_cmd_header(cmd), \
> - .DWordLength = n - _blorp_cmd_length_bias(cmd), \
> - }; \
> - _blorp_cmd_pack(cmd)(batch, _dw, &template); \
> - _dw + 1; /* Array starts at dw[1] */ \
> +#define blorp_emitn(batch, cmd, n) ({ \
> + uint32_t *_dw = blorp_emit_dwords(batch, n); \
> + if (_dw) { \
> + struct cmd template = { \
> + _blorp_cmd_header(cmd), \
> + .DWordLength = n - _blorp_cmd_length_bias(cmd), \
> + }; \
> + _blorp_cmd_pack(cmd)(batch, _dw, &template); \
> + } \
> + _dw ? _dw + 1 : NULL; /* Array starts at dw[1] */ \
> })
>
> /* 3DSTATE_URB
> @@ -274,6 +276,8 @@ blorp_emit_vertex_buffers(struct blorp_batch *batch,
>
> const unsigned num_dwords = 1 + GENX(VERTEX_BUFFER_STATE_length) * 2;
> uint32_t *dw = blorp_emitn(batch, GENX(3DSTATE_VERTEX_BUFFERS), num_dwords);
> + if (!dw)
> + return;
>
> for (unsigned i = 0; i < 2; i++) {
> GENX(VERTEX_BUFFER_STATE_pack)(batch, dw, &vb[i]);
> @@ -379,6 +383,8 @@ blorp_emit_vertex_elements(struct blorp_batch *batch,
> const unsigned num_dwords =
> 1 + GENX(VERTEX_ELEMENT_STATE_length) * num_elements;
> uint32_t *dw = blorp_emitn(batch, GENX(3DSTATE_VERTEX_ELEMENTS), num_dwords);
> + if (!dw)
> + return;
>
> for (unsigned i = 0; i < num_elements; i++) {
> GENX(VERTEX_ELEMENT_STATE_pack)(batch, dw, &ve[i]);
> @@ -1019,6 +1025,9 @@ blorp_emit_depth_stencil_state(struct blorp_batch *batch,
> uint32_t offset = 0;
> uint32_t *dw = blorp_emit_dwords(batch,
> GENX(3DSTATE_WM_DEPTH_STENCIL_length));
> + if (!dw)
> + return 0;
> +
> GENX(3DSTATE_WM_DEPTH_STENCIL_pack)(NULL, dw, &ds);
> #else
> uint32_t offset;
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index d1bb761..7490d0c 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -779,15 +779,17 @@ _anv_combine_address(struct anv_batch *batch, void *location,
> VG(VALGRIND_CHECK_MEM_IS_DEFINED(dst, __anv_cmd_length(struc) * 4)); \
> } while (0)
>
> -#define anv_batch_emitn(batch, n, cmd, ...) ({ \
> - void *__dst = anv_batch_emit_dwords(batch, n); \
> - struct cmd __template = { \
> - __anv_cmd_header(cmd), \
> - .DWordLength = n - __anv_cmd_length_bias(cmd), \
> - __VA_ARGS__ \
> - }; \
> - __anv_cmd_pack(cmd)(batch, __dst, &__template); \
> - __dst; \
> +#define anv_batch_emitn(batch, n, cmd, ...) ({ \
> + void *__dst = anv_batch_emit_dwords(batch, n); \
> + if (__dst) { \
> + struct cmd __template = { \
> + __anv_cmd_header(cmd), \
> + .DWordLength = n - __anv_cmd_length_bias(cmd), \
> + __VA_ARGS__ \
> + }; \
> + __anv_cmd_pack(cmd)(batch, __dst, &__template); \
> + } \
> + __dst; \
> })
>
> #define anv_batch_emit_merge(batch, dwords0, dwords1) \
> @@ -796,6 +798,8 @@ _anv_combine_address(struct anv_batch *batch, void *location,
> \
> STATIC_ASSERT(ARRAY_SIZE(dwords0) == ARRAY_SIZE(dwords1)); \
> dw = anv_batch_emit_dwords((batch), ARRAY_SIZE(dwords0)); \
> + if (!dw) \
> + break; \
> for (uint32_t i = 0; i < ARRAY_SIZE(dwords0); i++) \
> dw[i] = (dwords0)[i] | (dwords1)[i]; \
> VG(VALGRIND_CHECK_MEM_IS_DEFINED(dw, ARRAY_SIZE(dwords0) * 4));\
> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
> index a6ec3b6..7b01ee5 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -389,10 +389,14 @@ emit_3dstate_sbe(struct anv_pipeline *pipeline)
>
> uint32_t *dw = anv_batch_emit_dwords(&pipeline->batch,
> GENX(3DSTATE_SBE_length));
> + if (!dw)
> + return;
> GENX(3DSTATE_SBE_pack)(&pipeline->batch, dw, &sbe);
>
> #if GEN_GEN >= 8
> dw = anv_batch_emit_dwords(&pipeline->batch, GENX(3DSTATE_SBE_SWIZ_length));
> + if (!dw)
> + return;
> GENX(3DSTATE_SBE_SWIZ_pack)(&pipeline->batch, dw, &swiz);
> #endif
> }
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list