| Message ID | 1507712056-25030-20-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble <sagar.a.kamble@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@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@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 ? > + i915_ggtt_disable_guc(dev_priv); > + > guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; > huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; > } Btw, what should we do with "suspended" flag during sanitize ?
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@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@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@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 don't want to ggtt_disable_guc. if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in it. > >> + i915_ggtt_disable_guc(dev_priv); >> + >> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >> } > > Btw, what should we do with "suspended" flag during sanitize ? suspended flag is set to true on suspend and false on resume. sanitize is done post suspend and before resume so we should not touch it. initializing it to false during guc_init_early should take care of reload (during unload we are suspending gem but we wont resume)
On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble <sagar.a.kamble@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@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@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@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. > don't want to ggtt_disable_guc. > if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in > it. Hmm, this is the way how we run CI tests ;) >> >>> + i915_ggtt_disable_guc(dev_priv); >>> + >>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>> } >> >> Btw, what should we do with "suspended" flag during sanitize ? > suspended flag is set to true on suspend and false on resume. > sanitize is done post suspend and before resume so we should not touch > it. > initializing it to false during guc_init_early should take care of > reload (during unload we are suspending gem but we wont resume)
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@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@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@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Joonas Lahtinen <joonas.lahtinen@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 so enable_guc_loading will have non-zero value for some platforms. So I think it makes sense to limit this to only load_status based. > >> don't want to ggtt_disable_guc. >> if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON >> in it. > > Hmm, this is the way how we run CI tests ;) yes ... from long time :) > >>> >>>> + i915_ggtt_disable_guc(dev_priv); >>>> + >>>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>>> } >>> >>> Btw, what should we do with "suspended" flag during sanitize ? >> suspended flag is set to true on suspend and false on resume. >> sanitize is done post suspend and before resume so we should not >> touch it. >> initializing it to false during guc_init_early should take care of >> reload (during unload we are suspending gem but we wont resume)
On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble <sagar.a.kamble@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@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@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@intel.com> >>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Joonas Lahtinen <joonas.lahtinen@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? > so enable_guc_loading will have > non-zero value for some platforms. So I think it makes sense to limit > this to only load_status based. >> >>> don't want to ggtt_disable_guc. >>> if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in >>> it. >> >> Hmm, this is the way how we run CI tests ;) > yes ... from long time :) >> >>>> >>>>> + i915_ggtt_disable_guc(dev_priv); >>>>> + >>>>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>>>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; >>>>> } >>>> >>>> Btw, what should we do with "suspended" flag during sanitize ? >>> suspended flag is set to true on suspend and false on resume. >>> sanitize is done post suspend and before resume so we should not touch >>> it. >>> initializing it to false during guc_init_early should take care of >>> reload (during unload we are suspending gem but we wont resume)
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@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@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@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@intel.com> > > > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > > > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@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
On 10/12/2017 2:38 PM, Joonas Lahtinen wrote: > 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@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@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@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@intel.com> >>>>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@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. Sorry. I thought the first reorg series that was merged last time was the only gate for GEM/GuC fixes series. There are not a lot conflicts with current reorg series. But I now understand that I should have held onto the new revision. Will wait for these changes to go in. > 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
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) + i915_ggtt_disable_guc(dev_priv); + guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; huc_fw->load_status = INTEL_UC_FIRMWARE_NONE; }
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@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@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(-)