[Mesa-dev] [PATCH 07/10] i965: Prepare batchbuffer module for softpin support.

[Chris Wilson]

Quoting Kenneth Graunke (2018-05-04 02:12:37)
> @@ -933,6 +945,14 @@ emit_reloc(struct intel_batchbuffer *batch,
>  {
>     assert(target != NULL);
>  
> +   if (target->kflags & EXEC_OBJECT_PINNED) {
> +      brw_use_pinned_bo(batch, target, reloc_flags & RELOC_WRITE);
> +      return target->gtt_offset + target_offset;
> +   }
> +
> +   unsigned int index = add_exec_bo(batch, target);
> +   struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];
> +
>     if (rlist->reloc_count == rlist->reloc_array_size) {
>        rlist->reloc_array_size *= 2;
>        rlist->relocs = realloc(rlist->relocs,
> @@ -940,9 +960,6 @@ emit_reloc(struct intel_batchbuffer *batch,
>                                sizeof(struct drm_i915_gem_relocation_entry));
>     }
>  
> -   unsigned int index = add_exec_bo(batch, target);
> -   struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];
> -
>     if (reloc_flags & RELOC_32BIT) {
>        /* Restrict this buffer to the low 32 bits of the address space.
>         *
> @@ -976,6 +993,21 @@ emit_reloc(struct intel_batchbuffer *batch,
>     return entry->offset + target_offset;
>  }
>  
> +void
> +brw_use_pinned_bo(struct intel_batchbuffer *batch, struct brw_bo *bo,
> +                  unsigned writable_flag)
> +{
> +   assert(bo->kflags & EXEC_OBJECT_PINNED);
> +   assert((writable_flag & ~EXEC_OBJECT_WRITE) == 0);
> +
> +   unsigned int index = add_exec_bo(batch, bo);
> +   struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];
> +   assert(entry->offset == bo->gtt_offset);
> +
> +   if (writable_flag)
> +      entry->flags |= EXEC_OBJECT_WRITE;
> +}

I'm not fond of this (at least the emit_reloc() perspective). In
emit_reloc() we were very careful to order the writes to ensure the
validation object was always consistent with the batchbuffer entry,
and this throws it all away (granted it's not a concern, my worry is
just that the code looks dangerous).

My preference would be something like:

static uint64_t
emit_reloc(struct intel_batchbuffer *batch,
           struct brw_reloc_list *rlist, uint32_t offset,
           struct brw_bo *target, int32_t target_offset,
           unsigned int reloc_flags)
{
   assert(target != NULL);

   unsigned int index = add_exec_bo(batch, target);
   struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];

   if (target->kflags & EXEC_OBJECT_PINNED) {
      assert(!(reloc_flags & ~EXEC_OBJECT_WRITE));
      goto skip_relocs;
   }

   if (rlist->reloc_count == rlist->reloc_array_size) {
      rlist->reloc_array_size *= 2;
      rlist->relocs = realloc(rlist->relocs,
                              rlist->reloc_array_size *
                              sizeof(struct drm_i915_gem_relocation_entry));
   }

   if (reloc_flags & RELOC_32BIT) {
      /* Restrict this buffer to the low 32 bits of the address space.
       *
       * Altering the validation list flags restricts it for this batch,
       * but we also alter the BO's kflags to restrict it permanently
       * (until the BO is destroyed and put back in the cache).  Buffers
       * may stay bound across batches, and we want keep it constrained.
       */
      target->kflags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
      entry->flags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;

      /* RELOC_32BIT is not an EXEC_OBJECT_* flag, so get rid of it. */
      reloc_flags &= ~RELOC_32BIT;
   }

   rlist->relocs[rlist->reloc_count++] =
      (struct drm_i915_gem_relocation_entry) {
         .offset = offset,
         .delta = target_offset,
         .target_handle = batch->use_batch_first ? index : target->gem_handle,
         .presumed_offset = entry->offset,
      };

skip_relocs:
   if (reloc_flags)
      entry->flags |= reloc_flags & batch->valid_reloc_flags;

   /* Using the old buffer offset, write in what the right data would be, in
    * case the buffer doesn't move and we can short-circuit the relocation
    * processing in the kernel
    */
   return entry->offset + target_offset;
}

The relationship between validation object entry and the batch buffer
contents is much easier to verify.
-Chris