[Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
Sagar Arun Kamble
sagar.a.kamble at intel.com
Mon Nov 6 08:16:29 UTC 2017
Please update the subject to "Implement dynamic WOPCM partitioning"
Also, commit description should be written in present tense form.
On 11/4/2017 5:48 AM, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
>
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
Could you tell briefly here what is being done to wopcm offset and size
in this patch.
>
> Signed-off-by: Jackie Li <yaodong.li at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: John Spotswood <john.a.spotswood at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
> drivers/gpu/drm/i915/intel_guc.h | 18 +----
> drivers/gpu/drm/i915/intel_huc.c | 3 +-
> drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
> 9 files changed, 223 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e06..19509fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> WARN_ON(!list_empty(&dev_priv->contexts.list));
> }
>
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> + wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
> static int i915_load_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> + /*
> + * Get the wopcm memory info.
> + * NOTE: this need to be called before init FW.
> + */
> + i915_wopcm_init(dev_priv);
> +
I think this call might be better if done from
i915_driver_init_hw->i915_ggtt_probe_hw->*_gmch_probe after
gem_init_stolen as this is part of stolen memory. Might help performing
any validation w.r.t stolen memory alloc.
> intel_uc_init_fw(dev_priv);
>
> ret = i915_gem_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72bb5b5..61cd290 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
> u8 voltage_level;
> };
>
> +struct intel_wopcm_info {
> + u32 size;
> +};
> +
> +struct intel_wopcm_partition {
> + u32 guc_wopcm_offset;
> + u32 guc_wopcm_size;
> + u32 guc_wopcm_top;
> +};
> +
*_partition can become part of *_info. This can be named as
intel_guc_wopcm_partition and drop "guc_" prefix from variable names.
> struct drm_i915_private {
> struct drm_device drm;
>
> @@ -2258,6 +2268,9 @@ struct drm_i915_private {
> struct intel_huc huc;
> struct intel_guc guc;
>
> + struct intel_wopcm_info wopcm;
> + struct intel_wopcm_partition wopcm_partition;
> +
> struct intel_csr csr;
>
> struct intel_gmbus gmbus[GMBUS_NUM_PINS];
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb3..7347fd7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> ctx->desc_template =
> default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
>
> - /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not
> + /* GuC requires the ring to be placed above guc wopcm top. If GuC is not
> * present or not in use we still need a small bias as ring wraparound
> * at offset 0 sometimes hangs. No idea why.
> */
> if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> - ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> + ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 35cf991..d309884 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -67,17 +67,27 @@
> #define DMA_GUC_WOPCM_OFFSET _MMIO(0xc340)
> #define HUC_LOADING_AGENT_VCR (0<<1)
> #define HUC_LOADING_AGENT_GUC (1<<1)
> -#define GUC_WOPCM_OFFSET_VALUE 0x80000 /* 512KB */
> #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
>
> #define HUC_STATUS2 _MMIO(0xD3B0)
> #define HUC_FW_VERIFIED (1<<7)
>
> /* Defines WOPCM space available to GuC firmware */
WOPCM memory map here in the documentation will be helpful.
> +/* default WOPCM size 1MB */
> +#define WOPCM_DEFAULT_SIZE (0x1 << 20)
> +/* reserved WOPCM size 16KB */
> +#define WOPCM_RESERVED_SIZE (0x4000)
> +/* GUC WOPCM Offset need to be 16KB aligned */
> +#define WOPCM_OFFSET_ALIGNMENT (0x4000)
> +/* 8KB stack reserved for GuC FW*/
> +#define GUC_WOPCM_STACK_RESERVED (0x2000)
> +/* 24KB WOPCM reserved for RC6 CTX on BXT */
> +#define BXT_WOPCM_RC6_RESERVED (0x6000)
> +
> +#define GEN9_GUC_WOPCM_DELTA 4
> +#define GEN9_GUC_WOPCM_OFFSET (0x24000)
> +
> #define GUC_WOPCM_SIZE _MMIO(0xc050)
> -/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
> -#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */
> -#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */
>
> /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
> #define GUC_GGTT_TOP 0xFEE00000
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 9678630..0efcfb4 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> * This is a wrapper to create an object for use with the GuC. In order to
> * use it inside the GuC, an object needs to be pinned lifetime, so we allocate
> * both some backing storage and a range inside the Global GTT. We must pin
> - * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
> + * it in the GGTT somewhere other than than [0, guc wopcm_top) because that
> * range is reserved inside GuC.
> *
> * Return: A i915_vma if successful, otherwise an ERR_PTR.
> @@ -358,7 +358,8 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
> goto err;
>
> ret = i915_vma_pin(vma, 0, PAGE_SIZE,
> - PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_GLOBAL | PIN_OFFSET_BIAS |
> + intel_guc_wopcm_top(dev_priv));
> if (ret) {
> vma = ERR_PTR(ret);
> goto err;
> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
> {
> - u32 wopcm_size = GUC_WOPCM_TOP;
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_GEN9_LP(dev_priv))
> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> + GEM_BUG_ON(!wp->guc_wopcm_size);
>
> - return wopcm_size;
> + return wp->guc_wopcm_size;
> +}
> +
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
> +}
> +
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!wp->guc_wopcm_size);
> +
> + return wp->guc_wopcm_offset;
> +}
> +
> +/*
> + * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
change WOPCM_TOP
> + * which is reserved for Boot ROM, SRAM and WOPCM. All gfx objects
> + * used by GuC is pinned with PIN_OFFSET_BIAS along with top of WOPCM.
> + */
> +u32 guc_ggtt_offset(struct i915_vma *vma)
This move can be in a separate patch. Instead, I think all above
functions need to be moved to intel_guc.c.
> +{
> + struct drm_i915_private *dev_priv = vma->vm->i915;
> + u32 offset = i915_ggtt_offset(vma);
> +
> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> + return offset;
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 607e025..1493de0 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
> guc->notify(guc);
> }
>
> -/*
> - * 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.
> - */
> -static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> -{
> - u32 offset = i915_ggtt_offset(vma);
> -
> - GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> - GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> -
> - return offset;
> -}
>
> void intel_guc_init_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> @@ -127,5 +112,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv);
> int intel_guc_resume(struct drm_i915_private *dev_priv);
> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
> +u32 guc_ggtt_offset(struct i915_vma *vma);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 98d1725..a985aa5 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
> return;
>
> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_OFFSET_BIAS |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> DRM_ERROR("failed to pin huc fw object %d\n",
> (int)PTR_ERR(vma));
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index aec2954..83b2516 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> intel_guc_init_early(&dev_priv->guc);
> }
>
> +static u32 rc6_context_size(struct drm_i915_private *dev_priv)
> +{
> + /* On BXT, the top of WOPCM is reserved for RC6 context */
> + if (IS_GEN9_LP(dev_priv))
> + return BXT_WOPCM_RC6_RESERVED;
> +
> + return 0;
> +}
> +
> +static int do_wopcm_partition(struct drm_i915_private *dev_priv,
> + u32 offset, u32 reserved_size)
Alignment issue. parameters should match the open braces spacing above.
Can use checkpatch to spot such issues.
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
> +
> + if (offset >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if (reserved_size >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + wp->guc_wopcm_offset = aligned_offset;
> + wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
> + wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
> +
> + return 0;
> +}
> +
> +static int intel_uc_init_wopcm_partition(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> + size_t huc_size, guc_size;
> + u32 offset;
> + u32 reserved;
> + u32 wopcm_base;
> + u32 delta;
> + int err;
> +
> + /*Return if WOPCM partition has been initialized*/
> + if (wp->guc_wopcm_size)
> + return 0;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + /*No need to do partition if failed to fetch both GuC and HuC FW*/
> + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
> + huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + return -EIO;
> +
> + huc_size = 0;
> + guc_size = 0;
> + offset = WOPCM_RESERVED_SIZE;
> + reserved = rc6_context_size(dev_priv);
> +
> + if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> + huc_size = huc_fw->header_size + huc_fw->ucode_size;
> +
> + err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
> + if (err) {
> + if (!huc_size)
> + goto pt_done;
> +
> + /*partition failed with HuC FW, block HuC loading*/
> + huc_size = 0;
> +
> + /*partition without loading HuC FW*/
> + err = do_wopcm_partition(dev_priv, offset, reserved);
> + if (err)
> + goto pt_done;
> + }
> +
> + /*
> + * Check hardware restriction on Gen9
> + * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due
> + * to hardware limitation on Gen9.
> + */
> + if (IS_GEN9(dev_priv)) {
> + wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
Should this GEN9_GUC_WOPCM_OFFSET be added to "offset" prior to calling
do_wopcm_partition?
Then it will update wp->guc_wopcm_offset for Gen9 as well. Or that
update is not needed?
> + if (unlikely(wopcm_base > wp->guc_wopcm_size))
> + goto pt_done;
> +
> + delta = wp->guc_wopcm_size - wopcm_base;
> + if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
> + goto pt_done;
> + }
> +
> + if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
> + guc_size = guc_fw->header_size + guc_fw->ucode_size;
> + /*need 8K stack for GuC*/
> + guc_size += GUC_WOPCM_STACK_RESERVED;
> + }
> +
> + if (guc_size > wp->guc_wopcm_size)
> + guc_size = 0;
> +
> +pt_done:
> + if (!huc_size) {
> + DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(huc_fw);
> + }
> +
> + if (!guc_size) {
> + DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(guc_fw);
> + }
> +
> + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
> + wp->guc_wopcm_offset >> 10,
> + wp->guc_wopcm_size >> 10,
> + wp->guc_wopcm_top >> 10);
> +
> + return err;
> +}
> +
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> {
> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
> @@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> if (!i915_modparams.enable_guc_loading)
> return 0;
>
> + /*init WOPCM partition*/
> + ret = intel_uc_init_wopcm_partition(dev_priv);
> + if (ret)
> + goto err_wopcm;
> +
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
>
> @@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> /* init WOPCM */
> I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
> I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> - GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
> + intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
>
> /* WaEnableuKernelHeaderValidFix:skl */
> /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> @@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> i915_guc_submission_fini(dev_priv);
> err_guc:
> i915_ggtt_disable_guc(dev_priv);
> -
> +err_wopcm:
> if (i915_modparams.enable_guc_loading > 1 ||
> i915_modparams.enable_guc_submission > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 973888e..aefba13 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -95,9 +95,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
>
> - /* Header and uCode will be loaded to WOPCM */
> + /*
> + * Header and uCode will be loaded to WOPCM
> + * Only check the size against the overall available WOPCM here. Will
> + * continue to check the size during WOPCM partition calculation.
> + */
> size = uc_fw->header_size + uc_fw->ucode_size;
> - if (size > intel_guc_wopcm_size(dev_priv)) {
> + if (size > dev_priv->wopcm.size) {
> DRM_WARN("%s: Firmware is too large to fit in WOPCM\n",
> intel_uc_fw_type_repr(uc_fw->type));
> err = -E2BIG;
> @@ -207,6 +211,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> int (*xfer)(struct intel_uc_fw *uc_fw,
> struct i915_vma *vma))
> {
> + struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
> struct i915_vma *vma;
> int err;
>
> @@ -230,7 +235,8 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> }
>
> vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_OFFSET_BIAS |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
More information about the Intel-gfx
mailing list