[Intel-gfx] [PATCH 1/6] drm/i915/uc: move uC WOPCM setup in uc_init_hw
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Jul 30 14:39:28 UTC 2019
On Tue, 30 Jul 2019 01:47:22 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> The register we write are not WOPCM regs but uC ones related to how
> GuC and HuC are going to use the WOPCM, so it makes logical sense
> for them to be programmed as part of uc_init_hw. The wopcm map on the
s/wopcm/WOPCM
> other side is not uC-specific (although that is our main use-case), so
> keep that separate.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 62 ++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem.c | 8 +---
> drivers/gpu/drm/i915/intel_wopcm.c | 68 ---------------------------
> drivers/gpu/drm/i915/intel_wopcm.h | 3 --
> 4 files changed, 63 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index fafa9be1e12a..2f71f3704c46 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -390,6 +390,63 @@ void intel_uc_sanitize(struct intel_uc *uc)
> __uc_sanitize(uc);
> }
> +static int
> +write_and_verify(struct intel_gt *gt,
> + i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
as this function is more 'uncore' than 'gt' I would define it as:
static inline bool
intel_uncore_write_and_verify(struct intel_uncore *uncore,
i915_reg_t reg, u32 value,
u32 expected_value, u32 mask)
in intel_uncore.h
> +{
> + struct intel_uncore *uncore = gt->uncore;
> + u32 reg_val;
> +
> + GEM_BUG_ON(val & ~mask);
> +
> + intel_uncore_write(uncore, reg, val);
> +
> + reg_val = intel_uncore_read(uncore, reg);
> +
> + return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
> +}
> +
> +/* Initialize and verify the uC regs related to uC positioning in WOPCM
> */
> +int uc_wopcm_init_hw(struct intel_uc *uc)
static int uc_init_wopcm()
> +{
> + struct intel_gt *gt = uc_to_gt(uc);
> + struct intel_wopcm *wopcm = >->i915->wopcm;
> + struct intel_uncore *uncore = gt->uncore;
> + u32 huc_agent;
> + u32 mask;
> + int err;
> +
> + GEM_BUG_ON(!HAS_GT_UC(gt->i915));
> + GEM_BUG_ON(!intel_uc_is_using_guc(uc));
> + GEM_BUG_ON(!wopcm->guc.size);
on one hand there is intel_wopcm_guc_size() that can be used here,
but on other hand there is no intel_wopcm_guc_base ;(
> + GEM_BUG_ON(!wopcm->guc.base);
> +
> + err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
> + GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> + GUC_WOPCM_SIZE_LOCKED);
hmm, as these are write-once registers, maybe we should write only once
(not here) and only verify every time in uc_init_hw ?
> + if (err)
> + goto err_out;
> +
> + huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
> + mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
> + err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
> + wopcm->guc.base | huc_agent, mask,
> + GUC_WOPCM_OFFSET_VALID);
> + if (err)
> + goto err_out;
> +
> + return 0;
> +
> +err_out:
> + DRM_ERROR("Failed to init WOPCM registers:\n");
In commit msg you said that these are not WOPCM registers ;)
> + DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> + intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
> + DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
> + intel_uncore_read(uncore, GUC_WOPCM_SIZE));
btw, we can avoid extra read by reporting already failed write
in intel_uncore_write_and_verify()
> +
> + return err;
> +}
> +
> int intel_uc_init_hw(struct intel_uc *uc)
> {
> struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
> @@ -402,6 +459,10 @@ int intel_uc_init_hw(struct intel_uc *uc)
> GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> + ret = uc_wopcm_init_hw(uc);
> + if (ret)
> + goto out;
it should be harmless to reuse existing err_out label
> +
> guc_reset_interrupts(guc);
> /* WaEnableuKernelHeaderValidFix:skl */
> @@ -486,6 +547,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
> if (GEM_WARN_ON(ret == -EIO))
> ret = -EINVAL;
> +out:
> dev_err(i915->drm.dev, "GuC initialization failed %d\n", ret);
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 01dd0d1d9bf6..ae4e7cc3e3f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1239,14 +1239,8 @@ int i915_gem_init_hw(struct drm_i915_private
> *i915)
> goto out;
> }
> - ret = intel_wopcm_init_hw(&i915->wopcm, gt);
> - if (ret) {
> - DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
> - goto out;
> - }
> -
> /* We can't enable contexts until all firmware is loaded */
> - ret = intel_uc_init_hw(&i915->gt.uc);
> + ret = intel_uc_init_hw(>->uc);
> if (ret) {
> DRM_ERROR("Enabling uc failed (%d)\n", ret);
> goto out;
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index 0e86a9e85b49..d9973c0b0384 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -224,71 +224,3 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> return 0;
> }
> -
> -static int
> -write_and_verify(struct intel_gt *gt,
> - i915_reg_t reg, u32 val, u32 mask, u32 locked_bit)
> -{
> - struct intel_uncore *uncore = gt->uncore;
> - u32 reg_val;
> -
> - GEM_BUG_ON(val & ~mask);
> -
> - intel_uncore_write(uncore, reg, val);
> -
> - reg_val = intel_uncore_read(uncore, reg);
> -
> - return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
> -}
> -
> -/**
> - * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
> - * @wopcm: pointer to intel_wopcm.
> - * @gt: pointer to the containing GT
> - *
> - * Setup the GuC WOPCM size and offset registers with the calculated
> values. It
> - * will verify the register values to make sure the registers are
> locked with
> - * correct values.
> - *
> - * Return: 0 on success. -EIO if registers were locked with incorrect
> values.
> - */
> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt)
> -{
> - struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> - struct intel_uncore *uncore = gt->uncore;
> - u32 huc_agent;
> - u32 mask;
> - int err;
> -
> - if (!USES_GUC(i915))
> - return 0;
> -
> - GEM_BUG_ON(!HAS_GT_UC(i915));
> - GEM_BUG_ON(!wopcm->guc.size);
> - GEM_BUG_ON(!wopcm->guc.base);
> -
> - err = write_and_verify(gt, GUC_WOPCM_SIZE, wopcm->guc.size,
> - GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> - GUC_WOPCM_SIZE_LOCKED);
> - if (err)
> - goto err_out;
> -
> - huc_agent = USES_HUC(i915) ? HUC_LOADING_AGENT_GUC : 0;
> - mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
> - err = write_and_verify(gt, DMA_GUC_WOPCM_OFFSET,
> - wopcm->guc.base | huc_agent, mask,
> - GUC_WOPCM_OFFSET_VALID);
> - if (err)
> - goto err_out;
> -
> - return 0;
> -
> -err_out:
> - DRM_ERROR("Failed to init WOPCM registers:\n");
> - DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> - intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET));
> - DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
> - intel_uncore_read(uncore, GUC_WOPCM_SIZE));
> -
> - return err;
> -}
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h
> b/drivers/gpu/drm/i915/intel_wopcm.h
> index 56aaed4d64ff..e1f0f66aaa44 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
> @@ -9,8 +9,6 @@
> #include <linux/types.h>
> -struct intel_gt;
> -
> /**
> * struct intel_wopcm - Overall WOPCM info and WOPCM regions.
> * @size: Size of overall WOPCM.
> @@ -43,6 +41,5 @@ static inline u32 intel_wopcm_guc_size(struct
> intel_wopcm *wopcm)
> void intel_wopcm_init_early(struct intel_wopcm *wopcm);
> int intel_wopcm_init(struct intel_wopcm *wopcm);
> -int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt);
> #endif
More information about the Intel-gfx
mailing list