[Intel-gfx] [PATCH 3/4] drm/i915/uc: Place uC firmware in upper range of GGTT

[Chris Wilson]

Quoting Fernando Pacheco (2019-04-09 22:31:01)
> Currently we pin the GuC or HuC firmware image just
> before uploading. Perma-pin during uC initialization
> instead and use the range reserved at the top of the
> address space.
> 
> Moving the firmware resulted in needing to:
> - restore the ggtt mapping during the suspend/resume path.
> - use an additional pinning for the rsa signature which will
>   be used during HuC auth as addresses above GUC_GGTT_TOP
>   do not map through GTT.
> 
> Signed-off-by: Fernando Pacheco <fernando.pacheco at intel.com>
> ---
> @@ -315,3 +287,67 @@ void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p)
>         drm_printf(p, "\tRSA: offset %u, size %u\n",
>                    uc_fw->rsa_offset, uc_fw->rsa_size);
>  }
> +
> +void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw,
> +                          struct i915_ggtt *ggtt, u64 start)
> +{
> +       struct drm_i915_gem_object *obj = uc_fw->obj;
> +       struct i915_vma dummy = {
> +               .node = { .start = start, .size = obj->base.size },
> +               .size = obj->base.size,
> +               .pages = obj->mm.pages,
> +               .obj = obj,

Shouldn't need .size or .obj, but usually needs .vm.

> +       };
> +
> +       GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
> +       ggtt->vm.insert_entries(&ggtt->vm, &dummy, obj->cache_level, 0);
> +}
> +
> +int intel_uc_fw_ggtt_pin(struct intel_uc_fw *uc_fw,
> +                        struct i915_ggtt *ggtt, u64 start)
> +{
> +       struct drm_i915_gem_object *obj = uc_fw->obj;
> +       int err;
> +
> +       err = i915_gem_object_set_to_gtt_domain(obj, false);

Currently requires struct_mutex, and is not required as we can ensure
the pages are flushed on creation.

> +       if (err) {
> +               DRM_DEBUG_DRIVER("%s fw set-domain err=%d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type), err);
> +               return err;
> +       }
> +
> +       err = i915_gem_object_pin_pages(obj);

I'm pretty sure we don't need to pin the pages here, as the caller
should be holding the pages already for the duration of the bind.

So I think this should just reduce to the ggtt bind.

> +       if (err) {
> +               DRM_DEBUG_DRIVER("%s fw pin-pages err=%d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type), err);
> +               return err;
> +       }
> +
> +       intel_uc_fw_ggtt_bind(uc_fw, ggtt, start);
> +
> +       return 0;
> +}
> +
> +void intel_uc_fw_ggtt_unpin(struct intel_uc_fw *uc_fw,
> +                           struct i915_ggtt *ggtt, u64 start)
> +{
> +       struct drm_i915_gem_object *obj = uc_fw->obj;
> +       u64 length = obj->base.size;
> +
> +       ggtt->vm.clear_range(&ggtt->vm, start, length);
> +
> +       if (i915_gem_object_has_pinned_pages(obj))
> +               i915_gem_object_unpin_pages(obj);

No. You either own the pages, or you do not. Don't guess.
-Chris