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

[Daniele Ceraolo Spurio]

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?

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