[Mesa-dev] [PATCH 02/13] i965: Allow passing target_bo=NULL to brw_emit_reloc()

[Kenneth Graunke]

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 <kenneth at whitecape.org>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> Cc: Daniel Vetter <daniel.vetter at 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)
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170719/cc5c4263/attachment-0001.sig>