[Intel-gfx] [PATCH 3/3] drm/i915/huc: improve documentation

[Daniele Ceraolo Spurio]

On 10/9/19 2:44 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> On 10/9/19 7:41 AM, Martin Peres wrote:
>> On 28/09/2019 00:42, Daniele Ceraolo Spurio wrote:
>>> Better explain the usage of the microcontroller and what i915 is
>>> responsible of. While at it, fix the documentation for the auth
>>> function, which doesn't do any pinning anymore.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>>   Documentation/gpu/i915.rst                | 10 ++++++++--
>>>   drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 19 +++++++++++++++----
>>>   drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 15 ---------------
>>>   3 files changed, 23 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
>>> index 357e9dfa7de1..bfb64337db66 100644
>>> --- a/Documentation/gpu/i915.rst
>>> +++ b/Documentation/gpu/i915.rst
>>> @@ -471,8 +471,14 @@ GuC-based command submission
>>>   HuC
>>>   ---
>>> -.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>> -   :doc: HuC Firmware
>>> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> +   :doc: HuC
>>> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> +   :functions: intel_huc_auth
>>> +
>>> +HuC Firmware Layout
>>> +~~~~~~~~~~~~~~~~~~~
>>> +The HuC FW layout is the same as the GuC one, see `GuC Firmware 
>>> Layout`_
>>>   DMC
>>>   ---
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> index d4625c97b4f9..6e10fe898c90 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>> @@ -9,6 +9,18 @@
>>>   #include "intel_huc.h"
>>>   #include "i915_drv.h"
>>> +/**
>>> + * DOC: HuC
>>> + *
>>> + * The HuC is a dedicated microcontroller for usage in media HEVC (High
>>> + * Efficiency Video Coding) operations. Userspace can directly use 
>>> the firmware
>>> + * capabilities by adding HuC specific commands to batch buffers.
>>> + * The kernel driver is only responsible for loading the HuC 
>>> firmware and
>>> + * triggering its security authentication, which is performed by the 
>>> GuC. For
>>> + * The GuC to correctly perform the authentication, the HuC binary 
>>> must be
>>> + * loaded before the GuC one.
>>> + */
>>> +
>>>   void intel_huc_init_early(struct intel_huc *huc)
>>>   {
>>>       struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
>>> @@ -118,10 +130,9 @@ void intel_huc_fini(struct intel_huc *huc)
>>>    *
>>>    * Called after HuC and GuC firmware loading during 
>>> intel_uc_init_hw().
>>>    *
>>> - * This function pins HuC firmware image object into GGTT.
>>> - * Then it invokes GuC action to authenticate passing the offset to RSA
>>> - * signature through intel_guc_auth_huc(). It then waits for 50ms for
>>> - * firmware verification ACK and unpins the object.
>>> + * This function invokes the GuC action to authenticate the HuC 
>>> firmware,
>>> + * passing the offset of the RSA signature to intel_guc_auth_huc(). 
>>> It then
>>> + * waits for up to 50ms for firmware verification ACK.
>>>    */
>>>   int intel_huc_auth(struct intel_huc *huc)
>>>   {
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>> index 74602487ed67..d654340d4d03 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>> @@ -7,21 +7,6 @@
>>>   #include "intel_huc_fw.h"
>>>   #include "i915_drv.h"
>>> -/**
>>> - * DOC: HuC Firmware
>>> - *
>>> - * Motivation:
>>> - * GEN9 introduces a new dedicated firmware for usage in media HEVC 
>>> (High
>>> - * Efficiency Video Coding) operations. Userspace can use the firmware
>>> - * capabilities by adding HuC specific commands to batch buffers.
>>
>> Having a link to the media driver here that would explain what the HuC
>> enables would be beneficial (we don't want to maintain that).
>>
>>> - *
>>> - * Implementation:
>>> - * The same firmware loader is used as the GuC. However, the actual
>>> - * loading to HW is deferred until GEM initialization is done.
>>> - *
>>> - * Note that HuC firmware loading must be done before GuC loading.
>>> - */
>>
>> Could we add a section explaining the access the HuC has to memory, and
> 
> I'll have to follow up with the HuC team to understand how the memory 
> access works because I'm not too familiar with HuC. Are you ok if I 
> address all your other comments for GuC and HuC and add this as a follow 
> up later once I get the info?
> 

Never mind, I found the info  I needed (Bspec 48058), so I can do all 
the changes in one go.

Daniele

> Daniele
> 
>> one stating that the firmware is optional?
>>
>> Anyways, thanks! The series could be landed as-is already, but a few
>> more additions would be welcomed :)
>>
>> Martin
>>
>>> -
>>>   /**
>>>    * intel_huc_fw_init_early() - initializes HuC firmware struct
>>>    * @huc: intel_huc struct
>>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx