Re: [Intel-gfx] [PATCH v12 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Joonas Lahtinen]

Quoting Jackie Li (2018-03-02 02:16:42)
> +/**
> + * intel_guc_init_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
> + * @guc: intel_guc structure.
> + *
> + * This functional will calculate and initialize the ggtt_pin_bias value 
> based

s/functional/function/

> + * on overall WOPCM size and GuC WOPCM size.
> + */
> +void intel_guc_init_ggtt_pin_bias(struct intel_guc *guc)



> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -49,6 +49,9 @@ struct intel_guc {
> struct intel_guc_log log;
> struct intel_guc_ct ct;
>  
> +   /** @ggtt_pin_bias: offset where Non-WOPCM memory starts. */

I'm pretty sure /** description */ should be enough. If we really should
have this in the doxygen is the bigger question, maybe make it a normal
comment like the surrounding ones, for the time being.

> +   u32 ggtt_pin_bias;
> +
> /* Log snapshot if GuC errors during load */
> struct drm_i915_gem_object *load_err_log;
>  
> @@ -108,10 +111,11 @@ static inline void intel_guc_notify(struct intel_guc 
> *guc)
>   * @guc: intel_guc structure.
>   * @vma: i915 graphics virtual memory area.
>   *
> - * GuC does not allow any gfx GGTT address that falls into range [0, 
> WOPCM_TOP),
> - * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top 
> address is
> - * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
> - * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
> + * GuC does not allow any gfx GGTT address that falls into range
> + * [0, GuC ggtt_pin_bias), which is reserved for Boot ROM, SRAM and WOPCM.
> + * Currently, in order to exclude [0, GuC ggtt_pin_bias) address space from
> + * GGTT, all gfx objects used by GuC is allocated with 
> intel_guc_allocate_vma()

s/is allocated/are allocated/

> + * and pinned with PIN_OFFSET_BIAS along with the value of GuC ggtt_pin_bias.
>   *
>   * Return: GGTT offset that meets the GuC gfx address requirement.

This gives an impression that something is done for the vma to make it
comply, instead of just checking it complies. Is that going to be the
case in future? If not, "Return: GGTT offset of the vma" would be more
correct.

> @@ -129,6 +133,7 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc 
> *guc,
>  void intel_guc_init_early(struct intel_guc *guc);
>  void intel_guc_init_send_regs(struct intel_guc *guc);
>  void intel_guc_init_params(struct intel_guc *guc);
> +void intel_guc_init_ggtt_pin_bias(struct intel_guc *guc);

Bit unclear why this amount of details about the initialization sequence get
exposed to header files. Lets fix this in the later series.

> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -207,7 +207,7 @@ int intel_huc_auth(struct intel_huc *huc)
> return -ENOEXEC;
>  
> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> -   PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +   PIN_OFFSET_BIAS | guc->ggtt_pin_bias);

Indent should be fixed while here.

> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -115,6 +115,22 @@ static inline bool intel_uc_fw_is_selected(struct 
> intel_uc_fw *uc_fw)
> return uc_fw->path != NULL;
>  }
>  
> +/**
> + * intel_uc_fw_get_upload_size()() - Get size of firmware needed to be 
> uploaded.

I don't think the "()()" is correct or needed.

> + * @uc_fw: uC firmware.
> + *
> + * Get the size of the firmware that will be uploaded to WOPCM.

"firmware and header"?

> + *
> + * Return: Upload firmware size, or zero on firmware fetch failure.
> + */
> +static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)



> +static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 
> guc_wopcm_size)
> +{
> +   u32 offset;
> +
> +   /*
> +* GuC WOPCM size shall be at least a dword larger than the offset 
> from
> +* WOPCM base (GuC WOPCM offset from WOPCM base + 
> GEN9_GUC_WOPCM_OFFSET)
> +* due to hardware limitation on Gen9.
> +*/
> +   offset = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET;
> +   if (offset > guc_wopcm_size ||
> +   (guc_wopcm_size - offset) < sizeof(u32)) {
> +   DRM_ERROR("GuC WOPCM size (%uKiB) is too small.%uKiB 
> needed.\n",

Drop the () and add " " after .

> + guc_wopcm_size / 1024,
> + (u32)(offset + sizeof(u32)) / 1024);
> +   return -E2BIG;
> +   }
> +
> +   return 0;
> +}
> +
> +static inline int check_hw_restriction(struct drm_i915_private *i915,
> +  u32 guc_wopcm_base, u32 guc_wopcm_size)
> +{
> +   int err = 0;
> +
> +   if (IS_GEN9(i915))
> +   err = gen9_check_dword_gap(guc_wopcm_base, guc_wopcm_size);
> +   if (err)
> +   return err;

You can just "return err;" and the extra checks would become:

if (!err && IS_FOO())
err = ...;
> +
> +   return 0;
> +}
> +
> +/**
> + * intel_wopcm_init() - 

Re: [Intel-gfx] [PATCH v12 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Yaodong Li]

On 03/02/2018 12:04 AM, Sagar Arun Kamble wrote:

 (GUC_WOPCM_RESERVED + GEN9_GUC_FW_RESERVED)

+
+/**
+ * intel_wopcm_init_early() - Early initialization of the WOPCM.
+ * @wopcm: pointer to intel_wopcm.
+ *
+ * Setup the size of WOPCM which will be used by later on WOPCM 
partitioning.

+ */
+void intel_wopcm_init_early(struct intel_wopcm *wopcm)
+{
+    wopcm->size = GEN9_WOPCM_SIZE;
I am not sure if you plan to do this later but initializing it with 
value from gem_init_stolen now seems more appropriate.
I've been asked this for several times already. Yes. I have a plan, but 
just cannot switch to that plan right now.;-)


Thanks,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v12 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Sagar Arun Kamble]

On 3/2/2018 5:46 AM, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM offset and size. On
Gen9, the value of the GuC WOPCM size register needs to be larger than the
value of GuC WOPCM offset register + a Gen9 specific offset (144KB) for
reserved GuC WOPCM. Fail to enforce such a restriction on GuC WOPCM size
will lead to GuC firmware execution failures. On the other hand, with
current static GuC WOPCM offset and size values (512KB for both offset and
size), the GuC WOPCM size verification will fail on Gen9 even if it can be
fixed by lowering the GuC WOPCM offset by calculating its value based on
HuC firmware size (which is likely less than 200KB on Gen9), so that we can
have a GuC WOPCM size value which is large enough to pass the GuC WOPCM
size check.

This patch updates the reserved GuC WOPCM size for RC6 context on Gen9 to
24KB to strictly align with the Gen9 GuC WOPCM layout. It also adds support
to verify the GuC WOPCM size aganist the Gen9 hardware restrictions. To
meet all above requirements, let's provide dynamic partitioning of the
WOPCM that will be based on platform specific HuC/GuC firmware sizes.

v2:
  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
  - Removed unnecessary function calls (Joonas)
  - Init GuC WOPCM partition as soon as firmware fetching is completed

v3:
  - Fixed indentation issues (Chris)
  - Removed layering violation code (Chris/Michal)
  - Created separat files for GuC wopcm code  (Michal)
  - Used inline function to avoid code duplication (Michal)

v4:
  - Preset the GuC WOPCM top during early GuC init (Chris)
  - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed

v5:
  - Moved GuC DMA WOPCM register updating code into intel_wopcm.c
  - Took care of the locking status before writing to GuC DMA
Write-Once registers. (Joonas)

v6:
  - Made sure the GuC WOPCM size to be multiple of 4K (4K aligned)

v8:
  - Updated comments and fixed naming issues (Sagar/Joonas)
  - Updated commit message to include more description about the hardware
restriction on GuC WOPCM size (Sagar)

v9:
  - Minor changes variable names and code comments (Sagar)
  - Added detailed GuC WOPCM layout drawing (Sagar/Michal)
  - Refined macro definitions to be reader friendly (Michal)
  - Removed redundent check to valid flag (Michal)
  - Unified first parameter for exported GuC WOPCM functions (Michal)
  - Refined the name and parameter list of hardware restriction checking
functions (Michal)

v10:
  - Used shorter function name for internal functions (Joonas)
  - Moved init-ealry function into c file (Joonas)
  - Consolidated and removed redundant size checks (Joonas/Michal)
  - Removed unnecessary unlikely() from code which is only called once
during boot (Joonas)
  - More fixes to kernel-doc format and content (Michal)
  - Avoided the use of PAGE_MASK for 4K pages (Michal)
  - Added error log messages to error paths (Michal)

v11:
  - Replaced intel_guc_wopcm with more generic intel_wopcm and attached
intel_wopcm to drm_i915_private instead intel_guc (Michal)
  - dynamic calculation of GuC non-wopcm memory start (a.k.a WOPCM Top
offset from GuC WOPCM base) (Michal)
  - Moved WOPCM marco definitions into .c source file (Michal)
  - Exported WOPCM layout diagram as kernel-doc (Michal)

v12:
  - Updated naming, function kernel-doc to align with new changes (Michal)

Bspec: 12690

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Reviewed-by: Sagar Arun Kamble  (v8)
Reviewed-by: Joonas Lahtinen  (v9)
Reviewed-by: Michal Wajdeczko  (v11)
Signed-off-by: Jackie Li 
---
  drivers/gpu/drm/i915/Makefile   |   3 +-
  drivers/gpu/drm/i915/i915_drv.c |   1 +
  drivers/gpu/drm/i915/i915_drv.h |   8 ++
  drivers/gpu/drm/i915/i915_gem.c |   4 +
  drivers/gpu/drm/i915/i915_gem_context.c |   5 +-
  drivers/gpu/drm/i915/intel_guc.c|  66 ---
  drivers/gpu/drm/i915/intel_guc.h|  16 ++-
  drivers/gpu/drm/i915/intel_guc_reg.h|   8 +-
  drivers/gpu/drm/i915/intel_huc.c|   2 +-
  drivers/gpu/drm/i915/intel_uc.c |   6 +-
  drivers/gpu/drm/i915/intel_uc_fw.c  |  13 +--
  drivers/gpu/drm/i915/intel_uc_fw.h  |  16 +++
  drivers/gpu/drm/i915/intel_wopcm.c  | 195 
  drivers/gpu/drm/i915/intel_wopcm.h  |  34 ++
  14 files changed, 337 insertions(+), 40 deletions(-)
  create mode 100644 drivers/gpu/drm/i915/intel_wopcm.c