[Intel-gfx] [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions

[Joonas Lahtinen]

On Wed, 2017-10-11 at 20:20 +0200, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble  
> <sagar.a.kamble at intel.com> wrote:
> 
> > 
> > 
> > On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
> > > On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble  
> > > <sagar.a.kamble at intel.com> wrote:
> > > 
> > > > 
> > > > 
> > > > On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
> > > > > On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble  
> > > > > <sagar.a.kamble at intel.com> wrote:
> > > > > 
> > > > > > i915_ggtt_enable_guc has to happen first during i915_gem_resume
> > > > > > if GuC loading is enabled before GTT restore. In case GuC is not
> > > > > > loaded this enabling happening during intel_uc_init_hw need to
> > > > > > skipped. (avoid the GEM_BUG_ON)
> > > > > > i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
> > > > > > post GGTT suspend operations. Calling it during uc_sanitize covers
> > > > > > all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
> > > > > > needto be protected by struct_mutex. Hence struct_mutex locking is
> > > > > > added in i915_gem_sanitize while sanitizing uC. struct_mutex is  
> > > > > > already
> > > > > > held during i915_gem_reset_prepare.
> > > > > > 
> > > > > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > > > > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > > > > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > > > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_gem.c |  4 ++++
> > > > > >  drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
> > > > > >  2 files changed, 15 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> > > > > > b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > index a4bbf6c..77a0746 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private  
> > > > > > *dev_priv)
> > > > > >      WARN_ON(dev_priv->gt.awake);
> > > > > >     mutex_lock(&dev->struct_mutex);
> > > > > > +    /* We need to notify the guc whenever we change the GGTT */
> > > > > > +    if (i915_modparams.enable_guc_loading)
> > > > > > +        i915_ggtt_enable_guc(dev_priv);
> > > > > > +
> > > > > >      i915_gem_restore_gtt_mappings(dev_priv);
> > > > > >      i915_gem_restore_fences(dev_priv);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> > > > > > b/drivers/gpu/drm/i915/intel_uc.c
> > > > > > index 9010ab5..0b799fe 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > > > > @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private  
> > > > > > *dev_priv)
> > > > > >      guc_disable_communication(guc);
> > > > > >      gen9_reset_guc_interrupts(dev_priv);
> > > > > > -    /* We need to notify the guc whenever we change the GGTT */
> > > > > > -    i915_ggtt_enable_guc(dev_priv);
> > > > > > +    /*
> > > > > > +     * We need to notify the guc whenever we change the GGTT.
> > > > > > +     * During resume from sleep we would have already updated the
> > > > > > +     * GGTT invalidate function for GuC during i915_gem_resume so
> > > > > > +     * we need to skip here. Will enable here on driver load/reset.
> > > > > > +     */
> > > > > > +    if (!guc->suspended)
> > > > > > +        i915_ggtt_enable_guc(dev_priv);
> > > > > >     if (i915_modparams.enable_guc_submission) {
> > > > > >          /*
> > > > > > @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private  
> > > > > > *dev_priv)
> > > > > >      guc_free_load_err_log(guc);
> > > > > >     i915_guc_submission_cleanup(dev_priv);
> > > > > > -
> > > > > > -    if (i915_modparams.enable_guc_loading)
> > > > > > -        i915_ggtt_disable_guc(dev_priv);
> > > > > >  }
> > > > > > /**
> > > > > > @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private  
> > > > > > *dev_priv)
> > > > > >      struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> > > > > >     if (i915_modparams.enable_guc_loading) {
> > > > > > +        if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
> > > > > 
> > > > > Hmm, isn't that check redundant ?
> > > > 
> > > > uc_sanitize can happen without firmware loaded too in which case we
> > > 
> > > If uc_sanitize can be loaded without firmware loaded, then I assume
> > > i915_modparams.enable_guc_loading will be cleared too, right ?
> > > 
> > > I'm just wondering if we need to check both modparam and fw status.
> > 
> > actually load time uc_sanitize is happening before uc_sanitize_options
> 
> Hmm, so maybe we should call intel_sanitize_options() from or right after
> i915_driver_init_early() ? It looks that all 'sanitize-options' are using
> only device info flags, there is no MMIO access. Chris/Joonas?

Will be hard to answer any question on patch 19/21, when the preceding
patches are still under debate.

What I meant with focusing on single series at a time is that we first
discuss on any changes to get the first patches merged in reasonable
sized chunks of few patches per merge. During that discussion, lets
hold the dependent series from the mailing list for a while, because
the existing patches will cause changes and invalidate the reviews.

Now I have multiple dozens of e-mails in my inbox, and we're not going
to make progress that way. My understanding is that Michal's patches on
better organizing the GuC code are the first ones, so lets focus on
those first. So lets get back to this series after we're done with the
code organization sanitization.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation