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

[Martin Peres]

On 10/10/2019 04:02, 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.
> 
> v2: add a comment on HuC being optional and descrive how HuC accesses
>     memory (Martin)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Martin Peres <martin.peres at linux.intel.com>
> Acked-by: Anna Karas <anna.karas at intel.com>
> ---
>  Documentation/gpu/i915.rst                | 16 +++++++++--
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 33 ++++++++++++++++++++---
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 15 -----------
>  3 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index 357e9dfa7de1..60bd6e6403da 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -471,8 +471,20 @@ 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 Memory Management
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +   :doc: HuC Memory Management
> +
> +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 33608a114d4e..c802e5b68c05 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -9,6 +9,32 @@
>  #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.

Would be good to separate paragraphs with an empty line, for consistency.

> + * 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. Loading the HuC is optional; however, not using
> + * the HuC might negatively impact power usage and/or performance of media
> + * workloads, depending on the use-cases.

Same here.

> + * See https://github.com/intel/media-driver for the latest details on HuC
> + * functionality.

Well, isn't that the perfect link!? The media team has done a great job
on their communication!

> + */
> +
> +/**
> + * DOC: HuC Memory Management
> + *
> + * Similarly to the GuC, the HuC can't do any memory allocations on its own,
> + * with the difference being that the allocations for HuC usage are handled by
> + * the userspace driver instead of the kernel one. The HuC accesses the memory
> + * via the PPGTT belonging to the context loaded on the VCS executing the
> + * HuC-specific commands.
> + */

Thanks!

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>

Now, only one firmware is left to document! Jani, can you take actions
to improve the DMC documentation?

Martin

> +
>  void intel_huc_init_early(struct intel_huc *huc)
>  {
>  	struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
> @@ -118,10 +144,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.
> - *
> - * 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.
> - */
> -
>  /**
>   * intel_huc_fw_init_early() - initializes HuC firmware struct
>   * @huc: intel_huc struct
>