On Wednesday, July 19, 2017 3:09:10 AM PDT Chris Wilson wrote: > Sometimes we want to emit a relocation to a NULL surface when the > constructing the batch. If we push the NULL handling into the common > brw_emit_reloc() we can make the batch construction itself more > readable.
I don't like this...
There is no such thing as a "relocation to a NULL surface". No relocation
is emittted in this case. It either means the field is relative to a base
address, and is simply an offset, or the address is unused and we're setting
a NULL pointer combined with other bits packed into the same DWord.
Calling brw_emit_reloc should always emit a relocation to a real BO.
I would prefer to see NULL checks in the callers. Adding assert(target)
seems like a reasonable thing.
> On the other hand, we often test for the existence of the bo separately
> and so would like to skip that extra test for !bo, so we also add
> __brw_emit_reloc that can depend upon bo being valid.
I'm also a bit allergic to having brw_emit_reloc and __brw_emit_reloc,
since it begs the question of...what's the difference? Which do I use?
The answer ends up being simple, but you have to ask the question.
> Cc: Kenneth Graunke <kenn...@whitecape.org>
> Cc: Matt Turner <matts...@gmail.com>
> Cc: Jason Ekstrand <jason.ekstr...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 6 +++---
> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 30
> ++++++++++++++++++++-------
> 2 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 7af7d9b0a3..81cd578b28 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -752,11 +752,11 @@ brw_batch_references(struct intel_batchbuffer *batch,
> struct brw_bo *bo)
> /* This is the only way buffers get added to the validate list.
> */
> uint64_t
> -brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t batch_offset,
> +__brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t batch_offset,
> struct brw_bo *target, uint32_t target_offset,
> uint32_t read_domains, uint32_t write_domain)
> {
> - uint64_t offset64;
> + assert(target);
>
> if (batch->reloc_count == batch->reloc_array_size) {
> batch->reloc_array_size *= 2;
> @@ -778,7 +778,7 @@ brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t
> batch_offset,
> batch->reloc_count++;
>
> /* ensure gcc doesn't reload */
> - offset64 = *((volatile uint64_t *)&target->offset64);
> + uint64_t offset64 = *((volatile uint64_t *)&target->offset64);
> reloc->offset = batch_offset;
> reloc->delta = target_offset;
> reloc->target_handle = target->gem_handle;
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> index f1a5c1fd51..684c9f359c 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> @@ -70,9 +70,25 @@ bool brw_batch_has_aperture_space(struct brw_context *brw,
>
> bool brw_batch_references(struct intel_batchbuffer *batch, struct brw_bo
> *bo);
>
> -uint64_t brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t
> batch_offset,
> - struct brw_bo *target, uint32_t target_offset,
> - uint32_t read_domains, uint32_t write_domain);
> +uint64_t __brw_emit_reloc(struct intel_batchbuffer *batch,
> + uint32_t batch_offset,
> + struct brw_bo *target,
> + uint32_t target_offset,
> + uint32_t read_domains,
> + uint32_t write_domain);
> +
> +static inline uint64_t
> +brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t batch_offset,
> + struct brw_bo *target, uint32_t target_offset,
> + uint32_t read_domains, uint32_t write_domain)
> +{
> + if (!target)
> + return target_offset;
> +
> + return __brw_emit_reloc(batch, batch_offset,
> + target, target_offset,
> + read_domains, write_domain);
> +}
>
> #define USED_BATCH(batch) ((uintptr_t)((batch).map_next - (batch).map))
>
> @@ -161,8 +177,8 @@ intel_batchbuffer_advance(struct brw_context *brw)
> #define OUT_RELOC(buf, read_domains, write_domain, delta) do { \
> uint32_t __offset = (__map - brw->batch.map) * 4; \
> uint32_t reloc = \
> - brw_emit_reloc(&brw->batch, __offset, (buf), (delta), \
> - (read_domains), (write_domain)); \
> + __brw_emit_reloc(&brw->batch, __offset, (buf), (delta), \
> + (read_domains), (write_domain)); \
> OUT_BATCH(reloc); \
> } while (0)
>
> @@ -170,8 +186,8 @@ intel_batchbuffer_advance(struct brw_context *brw)
> #define OUT_RELOC64(buf, read_domains, write_domain, delta) do { \
> uint32_t __offset = (__map - brw->batch.map) * 4; \
> uint64_t reloc64 = \
> - brw_emit_reloc(&brw->batch, __offset, (buf), (delta), \
> - (read_domains), (write_domain)); \
> + __brw_emit_reloc(&brw->batch, __offset, (buf), (delta), \
> + (read_domains), (write_domain)); \
> OUT_BATCH(reloc64); \
> OUT_BATCH(reloc64 >> 32); \
> } while (0)
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
