Quoting Jackie Li (2018-02-09 01:03:51)
> 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. So we need add code to verify
> the GuC WOPCM offset and size to avoid any GuC 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 and add support to
> return CNL specific hardware reserved GuC WOPCM size. It also adds support
> to verify the GuC WOPCM size aganist the Gen9 hardware restrictions.
> Meanwhile, it provides a common way to calculate GuC WOPCM offset and size
> based on GuC and HuC firmware sizes for all GuC/HuC enabled platforms.
> Currently, GuC WOPCM offset is calculated based on HuC firmware size +
> reserved WOPCM size while GuC WOPCM size is set to total WOPCM size - GuC
> WOPCM offset - hardware reserved GuC WOPCM size. In this case, GuC WOPCM
> offset will be updated based on the size of HuC firmware while GuC WOPCM
> size will be set to use all the remaining WOPCM space.
>
> 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_guc_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)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: John Spotswood <john.a.spotswood@xxxxxxxxx>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> (v8)
> Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
<SNIP>
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> @@ -27,22 +27,115 @@
> #include "intel_guc_wopcm.h"
> #include "i915_drv.h"
>
> +static inline u32 guc_wopcm_context_reserved_size(struct intel_guc *guc)
This is intel_guc_wopcm file, so context_reserved_size() would be enough. But if
you feel like it is needed in the usage context, it's ok this way too.
> +int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
> + u32 huc_fw_size)
> {
> - struct drm_i915_private *i915 = guc_to_i915(guc);
> - u32 size = GUC_WOPCM_TOP;
> + struct intel_guc *guc =
> + container_of(guc_wopcm, struct intel_guc, wopcm);
> + u32 reserved = guc_wopcm_context_reserved_size(guc);
> + u32 offset, size, top;
> + int err;
>
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_GEN9_LP(i915))
> - size -= BXT_GUC_WOPCM_RC6_RESERVED;
> + if (!guc_fw_size)
> + return -EINVAL;
> +
> + if (reserved >= WOPCM_DEFAULT_SIZE)
> + return -E2BIG;
> +
> + offset = huc_fw_size + WOPCM_RESERVED_SIZE;
> + if (offset >= WOPCM_DEFAULT_SIZE)
> + return -E2BIG;
> +
> + /* Hardware requires GuC WOPCM offset to be 16K aligned. */
> + offset = ALIGN(offset, GUC_WOPCM_OFFSET_ALIGNMENT);
> + if ((offset + reserved) >= WOPCM_DEFAULT_SIZE)
> + return -E2BIG;
> +
Unless we're expecting a u32 wrap-around, could not we consolidate these
checks some? At least just checking offset against WOPCM_DEFAULT_SIZE
feels repetitive when in the next moment, we check ALIGN(offset) +
reserved vs. same limit.
> + top = WOPCM_DEFAULT_SIZE - offset;
> + size = top - reserved;
> +
> + /* GuC WOPCM size must be multiple of 4K pages */
> + size &= PAGE_MASK;
> +
> + /*
> + * GuC firmware size needs to be less than or equal to the size of the
> + * available GuC WOPCM (total available GuC WOPCM size - reserved size).
> + * Need extra 8K stack for GuC firmware.
> + */
> + reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> + if ((guc_fw_size + reserved) > size)
> + return -E2BIG;
> +
> + guc->wopcm.offset = offset;
> + guc->wopcm.size = size;
> + guc->wopcm.top = top;
> +
> + /* Check platform specific restrictions */
> + err = guc_wopcm_size_check(guc);
> + if (err)
> + return err;
> +
> + guc->wopcm.valid = 1;
> +
> + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
> + offset >> 10, size >> 10, top >> 10);
>
> - return size;
> + return 0;
> }
<SNIP>
> /*
> - * 512KB static GuC WOPCM size from GUC_WOPCM_OFFSET_VALUE to the end of GuC
> - * WOPCM. GuC addresses below GUC_WOPCM_TOP don't map through the GTT.
> + * The layout of the WOPCM will be determined by GuC WOPCM size and offset
> + * registers settings. Currently, GuC WOPCM code calculates the GuC WOPCM offset
> + * and size values based on a layout as shown below:
> + *
> + * +=====+==>+====================+<== GuC WOPCM top
> + * ^ ^ | HW contexts RSVD |
> + * | | +--------------------+<== GuC WOPCM size
> + * | | | |
> + * | GuC | |
> + * | WOPCM +--------------------+<== (Platform specific size)
> + * | | | GuC FW RSVD |
> + * WOPCM | +------------------- +<== (16KB)
> + * | v | RSVD GUC WOPCM |
> + * | +==>+====================+<== GuC WOPCM offset
> + * | | RSVD WOPCM |
> + * | +------------------- +
> + * v | HuC FW |
> + * +========>+====================+<== WOPCM Base
> + *
> + * GuC accessible WOPCM starts at GuC WOPCM offset and ends at GuC WOPCM size.
> + * The top part of the GuC WOPCM is reserved for hardware contexts (e.g. RC6
> + * context). We need to keep tracking the GuC WOPCM top since hardware requires
> + * the GGTT offset of a GuC accessible GEM buffer to be larger than the value of
> + * GuC WOPCM top. The values of GuC WOPCM size and top should be set to the
> + * length from GuC WOPCM offset in bytes.
> + */
> +
This diagram is a nice touch!
> +/* Default WOPCM size 1MB. */
> +#define WOPCM_DEFAULT_SIZE (0x1 << 20)
> +/* The initial 16KB WOPCM (RSVD WOPCM) is reserved. */
> +#define WOPCM_RESERVED_SIZE (16 << 10)
We might want to throw these to a more generic place, but they'll do
here for now as this is the only user.
> +static inline void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
> +{
> + guc_wopcm->top = WOPCM_DEFAULT_SIZE;
> +}
For header file brevity, move the implementation + comment to the .c file,
static inline header functions are an optimization for speed critical and often
used functions, which this definitely is not.
With the init_early function moved (and other changes at your
discretion) this is:
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
