| Message ID | 20190730230743.19542-2-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] drm/i915/uc: Don't enable communication twice on resume | expand |
Quoting Daniele Ceraolo Spurio (2019-07-31 00:07:40) > 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 > other side is not uC-specific (although that is our main use-case), so > keep that separate. > > v2: move write_and_verify to uncore, fix log, re-use err_out tag, > add intel_wopcm_guc_base, fix log > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 47 ++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 3 +- > drivers/gpu/drm/i915/i915_gem.c | 8 +--- > drivers/gpu/drm/i915/intel_uncore.h | 12 +++++ > drivers/gpu/drm/i915/intel_wopcm.c | 68 --------------------------- > drivers/gpu/drm/i915/intel_wopcm.h | 18 +++++-- > 6 files changed, 76 insertions(+), 80 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 657fdcb70d00..7794a6a1f932 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -399,6 +399,49 @@ void intel_uc_sanitize(struct intel_uc *uc) > __uc_sanitize(uc); > } > > +/* Initialize and verify the uC regs related to uC positioning in WOPCM */ > +static int uc_init_wopcm(struct intel_uc *uc) > +{ > + struct intel_gt *gt = uc_to_gt(uc); > + struct intel_uncore *uncore = gt->uncore; > + u32 base = intel_wopcm_guc_base(>->i915->wopcm); > + u32 size = intel_wopcm_guc_size(>->i915->wopcm); > + u32 huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; > + u32 mask; > + int err; > + > + GEM_BUG_ON(!intel_uc_is_using_guc(uc)); > + GEM_BUG_ON(!(base & GUC_WOPCM_OFFSET_MASK)); > + GEM_BUG_ON(base & ~GUC_WOPCM_OFFSET_MASK); > + GEM_BUG_ON(!(size & GUC_WOPCM_SIZE_MASK)); > + GEM_BUG_ON(size & ~GUC_WOPCM_SIZE_MASK); Worth a GEM_BUG_ON(base + size < base) ? > + > + mask = GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED; > + err = intel_uncore_write_and_verify(uncore, GUC_WOPCM_SIZE, size, mask, > + size | GUC_WOPCM_SIZE_LOCKED); > + if (err) > + goto err_out; > + > + mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent; > + err = intel_uncore_write_and_verify(uncore, DMA_GUC_WOPCM_OFFSET, > + base | huc_agent, mask, > + base | huc_agent | > + GUC_WOPCM_OFFSET_VALID); > + if (err) > + goto err_out; > + > + return 0; > + > +err_out: > + DRM_ERROR("Failed to init uC 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; > +} Fair enough, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, 31 Jul 2019 01:07:40 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@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 > other side is not uC-specific (although that is our main use-case), so > keep that separate. > > v2: move write_and_verify to uncore, fix log, re-use err_out tag, > add intel_wopcm_guc_base, fix log > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > --- a/drivers/gpu/drm/i915/intel_uncore.h > +++ b/drivers/gpu/drm/i915/intel_uncore.h > @@ -393,6 +393,18 @@ static inline void intel_uncore_rmw_fw(struct > intel_uncore *uncore, > intel_uncore_write_fw(uncore, reg, val); > } > +static inline int intel_uncore_write_and_verify(struct intel_uncore > *uncore, > + i915_reg_t reg, u32 val, > + u32 mask, u32 expected_val) > +{ > + u32 reg_val; > + > + intel_uncore_write(uncore, reg, val); > + reg_val = intel_uncore_read(uncore, reg); > + > + return (reg_val & mask) != expected_val ? -EINVAL : 0; > +} nit: I'm not sure that -EINVAL is the best choice (not sure about -ENODATA or -ENOKEY either) that's why I wanted to use bool ;) Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Quoting Michal Wajdeczko (2019-07-31 10:00:57) > On Wed, 31 Jul 2019 01:07:40 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@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 > > other side is not uC-specific (although that is our main use-case), so > > keep that separate. > > > > v2: move write_and_verify to uncore, fix log, re-use err_out tag, > > add intel_wopcm_guc_base, fix log > > > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > > --- a/drivers/gpu/drm/i915/intel_uncore.h > > +++ b/drivers/gpu/drm/i915/intel_uncore.h > > @@ -393,6 +393,18 @@ static inline void intel_uncore_rmw_fw(struct > > intel_uncore *uncore, > > intel_uncore_write_fw(uncore, reg, val); > > } > > +static inline int intel_uncore_write_and_verify(struct intel_uncore > > *uncore, > > + i915_reg_t reg, u32 val, > > + u32 mask, u32 expected_val) > > +{ > > + u32 reg_val; > > + > > + intel_uncore_write(uncore, reg, val); > > + reg_val = intel_uncore_read(uncore, reg); > > + > > + return (reg_val & mask) != expected_val ? -EINVAL : 0; > > +} > > nit: I'm not sure that -EINVAL is the best choice (not sure about > -ENODATA or -ENOKEY either) that's why I wanted to use bool ;) ENXIO? It's a bridge we can cross later. -Chris
Quoting Daniele Ceraolo Spurio (2019-07-31 00:07:40) > 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 > other side is not uC-specific (although that is our main use-case), so > keep that separate. > > v2: move write_and_verify to uncore, fix log, re-use err_out tag, > add intel_wopcm_guc_base, fix log > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Pushed the first 2. Just the usual naming debate as a hold up for the completion. -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 657fdcb70d00..7794a6a1f932 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -399,6 +399,49 @@ void intel_uc_sanitize(struct intel_uc *uc) __uc_sanitize(uc); } +/* Initialize and verify the uC regs related to uC positioning in WOPCM */ +static int uc_init_wopcm(struct intel_uc *uc) +{ + struct intel_gt *gt = uc_to_gt(uc); + struct intel_uncore *uncore = gt->uncore; + u32 base = intel_wopcm_guc_base(>->i915->wopcm); + u32 size = intel_wopcm_guc_size(>->i915->wopcm); + u32 huc_agent = intel_uc_is_using_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; + u32 mask; + int err; + + GEM_BUG_ON(!intel_uc_is_using_guc(uc)); + GEM_BUG_ON(!(base & GUC_WOPCM_OFFSET_MASK)); + GEM_BUG_ON(base & ~GUC_WOPCM_OFFSET_MASK); + GEM_BUG_ON(!(size & GUC_WOPCM_SIZE_MASK)); + GEM_BUG_ON(size & ~GUC_WOPCM_SIZE_MASK); + + mask = GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED; + err = intel_uncore_write_and_verify(uncore, GUC_WOPCM_SIZE, size, mask, + size | GUC_WOPCM_SIZE_LOCKED); + if (err) + goto err_out; + + mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent; + err = intel_uncore_write_and_verify(uncore, DMA_GUC_WOPCM_OFFSET, + base | huc_agent, mask, + base | huc_agent | + GUC_WOPCM_OFFSET_VALID); + if (err) + goto err_out; + + return 0; + +err_out: + DRM_ERROR("Failed to init uC 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; +} + int intel_uc_init_hw(struct intel_uc *uc) { struct drm_i915_private *i915 = uc_to_gt(uc)->i915; @@ -411,6 +454,10 @@ int intel_uc_init_hw(struct intel_uc *uc) GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw)); + ret = uc_init_wopcm(uc); + if (ret) + goto err_out; + guc_reset_interrupts(guc); /* WaEnableuKernelHeaderValidFix:skl */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3eeb21ff04c2..bdede6325d87 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2274,10 +2274,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define HAS_GT_UC(dev_priv) (INTEL_INFO(dev_priv)->has_gt_uc) -/* Having GuC/HuC is not the same as using GuC/HuC */ +/* Having GuC is not the same as using GuC */ #define USES_GUC(dev_priv) intel_uc_is_using_guc(&(dev_priv)->gt.uc) #define USES_GUC_SUBMISSION(dev_priv) intel_uc_is_using_guc_submission(&(dev_priv)->gt.uc) -#define USES_HUC(dev_priv) intel_uc_is_using_huc(&(dev_priv)->gt.uc) #define HAS_POOLED_EU(dev_priv) (INTEL_INFO(dev_priv)->has_pooled_eu) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 65863e955f40..f681152d27fa 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1240,14 +1240,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_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 2f6ffa309669..e603d210a34d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -393,6 +393,18 @@ static inline void intel_uncore_rmw_fw(struct intel_uncore *uncore, intel_uncore_write_fw(uncore, reg, val); } +static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore, + i915_reg_t reg, u32 val, + u32 mask, u32 expected_val) +{ + u32 reg_val; + + intel_uncore_write(uncore, reg, val); + reg_val = intel_uncore_read(uncore, reg); + + return (reg_val & mask) != expected_val ? -EINVAL : 0; +} + #define raw_reg_read(base, reg) \ readl(base + i915_mmio_reg_offset(reg)) #define raw_reg_write(base, reg, value) \ 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..f9b603205bb1 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. @@ -26,6 +24,21 @@ struct intel_wopcm { } guc; }; +/** + * intel_wopcm_guc_base() + * @wopcm: intel_wopcm structure + * + * Returns the base of the WOPCM shadowed region. + * + * Returns: + * 0 if GuC is not present or not in use. + * Otherwise, the GuC WOPCM base. + */ +static inline u32 intel_wopcm_guc_base(struct intel_wopcm *wopcm) +{ + return wopcm->guc.base; +} + /** * intel_wopcm_guc_size() * @wopcm: intel_wopcm structure @@ -43,6 +56,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
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 other side is not uC-specific (although that is our main use-case), so keep that separate. v2: move write_and_verify to uncore, fix log, re-use err_out tag, add intel_wopcm_guc_base, fix log Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 47 ++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/i915_gem.c | 8 +--- drivers/gpu/drm/i915/intel_uncore.h | 12 +++++ drivers/gpu/drm/i915/intel_wopcm.c | 68 --------------------------- drivers/gpu/drm/i915/intel_wopcm.h | 18 +++++-- 6 files changed, 76 insertions(+), 80 deletions(-)