| Message ID | 20171103151816.62048-2-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-11-03 15:18:13) > We silently assumed that DMA transfer will be completed > within assumed timeout and thus we were waiting only at > last step for GuC to become ready. Add intermediate wait > to catch unexpected delays in DMA transfer. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c > index c4f4526..74a61fe 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fw.c > +++ b/drivers/gpu/drm/i915/intel_guc_fw.c > @@ -160,6 +160,8 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct intel_uc_fw *guc_fw = &guc->fw; > unsigned long offset; > + u32 status; > + int ret; > > /* > * The header plus uCode will be copied to WOPCM via DMA, excluding any > @@ -182,7 +184,12 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) > /* Finally start the DMA */ > I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA)); > > - return 0; > + /* Wait for DMA to finish */ > + ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, START_DMA, 0, > + 2, 100, &status); To use _fw, you need to either hold forcewake or be sure you don't need it, and you also need to be sure that either you have serialisation around the routine or that it is not subject to the various concurrent mmio access bugs. I haven't seen anything that suggests they have been taken into account. -Chris
On Wed, 08 Nov 2017 21:36:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-11-03 15:18:13) >> We silently assumed that DMA transfer will be completed >> within assumed timeout and thus we were waiting only at >> last step for GuC to become ready. Add intermediate wait >> to catch unexpected delays in DMA transfer. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c >> b/drivers/gpu/drm/i915/intel_guc_fw.c >> index c4f4526..74a61fe 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c >> @@ -160,6 +160,8 @@ static int guc_xfer_ucode(struct intel_guc *guc, >> struct i915_vma *vma) >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> struct intel_uc_fw *guc_fw = &guc->fw; >> unsigned long offset; >> + u32 status; >> + int ret; >> >> /* >> * The header plus uCode will be copied to WOPCM via DMA, >> excluding any >> @@ -182,7 +184,12 @@ static int guc_xfer_ucode(struct intel_guc *guc, >> struct i915_vma *vma) >> /* Finally start the DMA */ >> I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA)); >> >> - return 0; >> + /* Wait for DMA to finish */ >> + ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, >> START_DMA, 0, >> + 2, 100, &status); > > To use _fw, you need to either hold forcewake or be sure you don't need > it, and you also need to be sure that either you have serialisation > around the routine or that it is not subject to the various concurrent > mmio access bugs. I haven't seen anything that suggests they have been > taken into account. Note that this function is called by guc_fw_xfer() inside intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); ... intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); and for serialization (with similar code in huc_ucode_xfer) we just rely on code sequence in intel_uc_init_hw ... is this not enough ? Michal
Quoting Michal Wajdeczko (2017-11-08 21:17:35) > On Wed, 08 Nov 2017 21:36:25 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2017-11-03 15:18:13) > >> We silently assumed that DMA transfer will be completed > >> within assumed timeout and thus we were waiting only at > >> last step for GuC to become ready. Add intermediate wait > >> to catch unexpected delays in DMA transfer. > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c > >> b/drivers/gpu/drm/i915/intel_guc_fw.c > >> index c4f4526..74a61fe 100644 > >> --- a/drivers/gpu/drm/i915/intel_guc_fw.c > >> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c > >> @@ -160,6 +160,8 @@ static int guc_xfer_ucode(struct intel_guc *guc, > >> struct i915_vma *vma) > >> struct drm_i915_private *dev_priv = guc_to_i915(guc); > >> struct intel_uc_fw *guc_fw = &guc->fw; > >> unsigned long offset; > >> + u32 status; > >> + int ret; > >> > >> /* > >> * The header plus uCode will be copied to WOPCM via DMA, > >> excluding any > >> @@ -182,7 +184,12 @@ static int guc_xfer_ucode(struct intel_guc *guc, > >> struct i915_vma *vma) > >> /* Finally start the DMA */ > >> I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA)); > >> > >> - return 0; > >> + /* Wait for DMA to finish */ > >> + ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, > >> START_DMA, 0, > >> + 2, 100, &status); > > > > To use _fw, you need to either hold forcewake or be sure you don't need > > it, and you also need to be sure that either you have serialisation > > around the routine or that it is not subject to the various concurrent > > mmio access bugs. I haven't seen anything that suggests they have been > > taken into account. > > Note that this function is called by guc_fw_xfer() inside > > intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > ... > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > > and for serialization (with similar code in huc_ucode_xfer) we just > rely on code sequence in intel_uc_init_hw ... is this not enough ? That's all that is required. It was odd to see the raw _fw version as the first use of _FW, hence the question. You are using that version because it's the only supplier of out_status. Less eyebrows will be raised if we made it available via intel_wait_for_register(). -Chris
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c index c4f4526..74a61fe 100644 --- a/drivers/gpu/drm/i915/intel_guc_fw.c +++ b/drivers/gpu/drm/i915/intel_guc_fw.c @@ -160,6 +160,8 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) struct drm_i915_private *dev_priv = guc_to_i915(guc); struct intel_uc_fw *guc_fw = &guc->fw; unsigned long offset; + u32 status; + int ret; /* * The header plus uCode will be copied to WOPCM via DMA, excluding any @@ -182,7 +184,12 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) /* Finally start the DMA */ I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA)); - return 0; + /* Wait for DMA to finish */ + ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, START_DMA, 0, + 2, 100, &status); + DRM_DEBUG_DRIVER("GuC DMA status %#x\n", status); + + return ret; } /*