Re: [PATCH 3/3] drm/i915/huc: improve documentation

[Martin Peres <martin.peres@linux.intel.com>]

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@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@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
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx