Quoting Michal Wajdeczko (2017-11-08 21:17:35) > On Wed, 08 Nov 2017 21:36:25 +0100, Chris Wilson > <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> > >> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > >> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > >> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> > >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> > >> --- > >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
