[Intel-gfx] [PATCH v4 2/5] drm/i915/guc: Wait for ucode DMA transfer completion

[Chris Wilson]

Quoting Michal Wajdeczko (2017-11-08 21:17:35)
> On Wed, 08 Nov 2017 21:36:25 +0100, Chris Wilson  
> <chris at 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 at intel.com>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at 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