On 4/9/19 2:53 PM, Chris Wilson wrote:
> 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@xxxxxxxxx>
>> ---
>> @@ -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.
My intent was to maintain what was being done before
but just doing it earlier.
But if it's not required..
>> + 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.
I might be misunderstanding, so could you please clarify
what you mean by "should be holding"? Are you stating
that the caller already holds the pages?
>> + 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
Yeah my bad.
Thanks,
Fernando
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
