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

From: Fernando Pacheco <fernando.pacheco@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/4] drm/i915/uc: Place uC firmware in upper range of GGTT
Date: Tue, 9 Apr 2019 15:53:08 -0700	[thread overview]
Message-ID: <78b1e1f4-9720-ac76-0661-208dce379bca@intel.com> (raw)
In-Reply-To: <155484682410.692.9102248547652289965@skylake-alporthouse-com>

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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx