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

[Martin Peres]

On 28/09/2019 00:42, Daniele Ceraolo Spurio wrote:
> Add a short description of what we expect from GuC and some minor
> improvements to existing documentation. Also remove a comment about a
> difference between GuC and HuC that is not true anymore.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> ---
>  Documentation/gpu/i915.rst                    | 22 ++++++++++------
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 26 +++++++++++++++++--
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  6 +++++
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h  |  3 ---
>  4 files changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index f1bae7867045..357e9dfa7de1 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -436,12 +436,24 @@ WOPCM Layout
>  GuC
>  ---
>  
> -Firmware Layout
> -~~~~~~~~~~~~~~~
> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +   :doc: GuC
> +
> +GuC Firmware Layout
> +~~~~~~~~~~~~~~~~~~~
>  
>  .. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>     :doc: Firmware Layout
>  
> +GuC Memory Management
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +   :doc: GuC Memory Management
> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +   :functions: intel_guc_allocate_vma
> +
> +
>  GuC-specific firmware loader
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> @@ -457,12 +469,6 @@ GuC-based command submission
>  .. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>     :internal:
>  
> -GuC Address Space
> -~~~~~~~~~~~~~~~~~
> -
> -.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc.c
> -   :doc: GuC Address Space
> -
>  HuC
>  ---
>  .. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 249c747e9756..c6f018099fd0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -9,6 +9,22 @@
>  #include "intel_guc_submission.h"
>  #include "i915_drv.h"
>  
> +/**
> + * DOC: GuC
> + *
> + * The GuC is a microcontroller inside the GT HW, introduced in gen9. The GuC is
> + * designed to offload some of the functionality usually run on the host driver;

"... functionality usually run on the host driver" -> "... functionality
usually performed by the host driver"?

> + * currently the main operations it can take care of are:
> + *
> + * - Authentication of the HuC, which is required to fully enable HuC usage.
> + * - Low latency graphics context scheduling (a.k.a. GuC submission).
> + * - GT Power management.
> + *
> + * The enable_guc module parameter can be used to select which of those
> + * operations to enable within GuC. Note that not all the operations are
> + * supported on all gen9+ platforms.

It would be good to say that the FW is optional and that we support
running without it. Also, it would be good to explain what is lost when
not using this FW (for now, only losing the ability to use the HuC).

> + */
> +
>  static void gen8_guc_raise_irq(struct intel_guc *guc)
>  {
>  	struct intel_gt *gt = guc_to_gt(guc);
> @@ -548,9 +564,15 @@ int intel_guc_resume(struct intel_guc *guc)
>  }
>  
>  /**
> - * DOC: GuC Address Space
> + * DOC: GuC Memory Management
>   *
> - * The layout of GuC address space is shown below:
> + * GuC can't allocate any memory for its own usage, so all the allocations must
> + * be handled by the host driver. GuC accesses the memory via the GGTT, with the
> + * exception of the top and bottom parts of the 4GB address space, which are
> + * instead re-mapped by the GuC HW to memory location of the FW itself (WOPCM)
> + * or other parts of the HW. The driver must take care not to place objects that
> + * the GuC is going to access in these reserved ranges. The layout of the GuC
> + * address space is shown below:

Thanks for writing this! This is really beneficial!

>   *
>   * ::
>   *
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index f325d3dd564f..849a44add424 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -29,6 +29,12 @@ enum {
>  /**
>   * DOC: GuC-based command submission
>   *
> + * IMPORTANT NOTE: GuC submission is currently not supported in i915. The GuC
> + * firmware is moving to an updated submission interface and we plan to
> + * turn submission back on when that lands. The below documentation (and related
> + * code) matches the old submission model and will be updated as part of the
> + * upgrade to the new flow.

Thanks for adding this :)

> + *
>   * GuC client:
>   * A intel_guc_client refers to a submission path through GuC. Currently, there
>   * is only one client, which is charged with all submissions to the GuC. This
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> index f8f6c91a0df6..029214cdedd5 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> @@ -39,9 +39,6 @@
>   * 3. Length info of each component can be found in header, in dwords.
>   * 4. Modulus and exponent key are not required by driver. They may not appear
>   *    in fw. So driver will load a truncated firmware in this case.
> - *
> - * The only difference between GuC and HuC firmwares is how the version
> - * information is saved.
>   */
>  
>  struct uc_css_header {
>