Re: [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC
Date: Thu, 12 Oct 2017 12:20:38 +0530	[thread overview]
Message-ID: <688c5f2a-80e2-e67d-b490-57c14c48c665@intel.com> (raw)
In-Reply-To: <op.y7yfrj10xaggs7@mwajdecz-mobl1.ger.corp.intel.com>


On 10/11/2017 10:49 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:09 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> On resume from drm sleep/suspend, we have gem_init_hw path to reload
>> the GuC/HuC firmware. However, on resume from runtime suspend we needed
>> to add support to reload the GuC/HuC firmware and resume.
>> We can leverage intel_uc_init_hw for this based on skip_load_on_resume.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c |  2 +-
>>  drivers/gpu/drm/i915/intel_uc.c | 28 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uc.h |  1 +
>>  3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 7d1b7e1..9e257e2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2113,7 +2113,7 @@ void i915_gem_runtime_resume(struct 
>> drm_i915_private *dev_priv)
>>      i915_gem_init_swizzling(dev_priv);
>>      i915_gem_restore_fences(dev_priv);
>> -    intel_uc_resume(dev_priv);
>> +    intel_uc_runtime_resume(dev_priv);
>>     mutex_unlock(&dev_priv->drm.struct_mutex);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index f641872..25acf8f 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private 
>> *dev_priv)
>>          guc->skip_load_on_resume = false;
>>      }
>>  }
>> +
>> +/**
>> + * intel_uc_runtime_resume() - Resume uC operation.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function invokes intel_uc_suspend that will if GuC is loaded
>                             ^^^^^^^^^^^^^^^^
> Please focus on tasks rather than function names.
Sure.
>
>> + * enable communication with GuC, enable GuC interrupts, invoke GuC OS
>> + * resumption and enable GuC submission.
>> + * If GuC is not loaded, GuC needs to be loaded and do the entire setup
>> + * by leveraging intel_uc_init_hw.
>> + *
>> + */
>> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>> +{
>> +    struct intel_guc *guc = &dev_priv->guc;
>> +
>> +    if (!guc->suspended)
>> +        return;
>> +
>> +    intel_uc_resume(dev_priv);
>> +
>> +    if (guc->skip_load_on_resume)
>
> Hmm, I may be lost, but I feel that some changes from 13/21 done
> in intel_uc_resume() looks like good candidate for this function.
>
> What I'm missing is clear distinction what each function will do,
> due to lot of conditions and cross calls.
Have introduced separate runtime and drm uc_suspend/resume functions in 
v14 and will help understand this better.
>
>> +        return;
>> +
>> +    WARN_ON(guc_wopcm_locked(guc));
>
> Why here?
will remove.
>
>> +
>> +    intel_uc_init_hw(dev_priv);
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 7d9dd9c..f741ccc 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -36,5 +36,6 @@
>>  void intel_uc_cleanup(struct drm_i915_private *dev_priv);
>>  int intel_uc_suspend(struct drm_i915_private *dev_priv);
>>  void intel_uc_resume(struct drm_i915_private *dev_priv);
>> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
>> #endif

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx