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

[Sagar Arun Kamble]

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 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.
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