On Mon, 2017-03-13 at 11:30 +0200, Pohjolainen, Topi wrote:
> 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?
I have fixed that locally, thanks!
> Otherwise this looks good to me:
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@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
> > }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
