Re: [PATCH v12 04/11] drm/i915: Create uC runtime and system suspend/resume helpers

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
> + Michal,
>
> On the principle of code motion first, changes second, I'd like to see
> the clean split-up from Michal before touching the files much. That way
>   git history will be easier to examine.
I think we wanted to get these fixes in prior to restructuring as that 
might take time.
Currently this v12 series has some bit of restructuring included along 
with fixes which has increased patches.
I have separated that today with new series. Will get that in first and 
then follow with fixes series or fixes series based on Michal's split-up 
series.
This is the minor restructuring series that is required for fixes 
series: https://patchwork.freedesktop.org/series/31140/

Michal, please suggest.

>
> Few comments below.
>
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> Prepared generic helpers intel_uc_suspend, intel_uc_resume,
>> intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
>> called from respective GEM functions.
>> Only exception is intel_uc_resume that needs to be called
>> w/ or w/o GuC loaded in i915_drm_resume path. Changes to
>> add WOPCM condition check to load GuC during resume will be added
>> in later patches.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> v3: Calling intel_uc_resume from i915_gem_resume post resuming
>> i915 gem setup. This is symmetrical with i915_gem_suspend.
>> Removed error messages from i915 suspend/resume routines as
>> uC suspend/resume routines will have those. (Michal Wajdeczko)
>> Declare wedged on uc_suspend failure and uc_resume failure.
>> (Michał Winiarski)
>> Keeping the uC suspend/resume function definitions close to other
>> uC functions.
>>
>> v4: Added implementation to intel_uc_resume as GuC resume is
>> needed to be triggered post reloading the firmware as well. Added
>> comments about semantics of GuC resume with the firmware reload.
>>
>> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
>> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
>> Updated comments as FIXME.
>>
>> 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>
> <SNIP>
>
>> @@ -1698,6 +1698,18 @@ static int i915_drm_resume(struct drm_device *dev)
>>   	}
>>   	mutex_unlock(&dev->struct_mutex);
>>   
>> +	/*
>> +	 * FIXME: Currently we know that at the end of suspend we have done Full
> I think "beginning of resume" is the one that bites us more.
Yes. I have actually rephrased these comments in my next rev but its 
held up before below series is merged:
https://patchwork.freedesktop.org/series/31140/
>
>> +	 * GPU reset and GuC is loaded again during i915_gem_init_hw.
>> +	 * Now, send action to GuC to resume back again as earlier call to
>> +	 * intel_uc_resume from i915_gem_resume would have done nothing.
>> +	 * This needs to be skipped if GuC was not loaded during resume as
>> +	 * intel_uc_resume would have been already called from i915_gem_resume.
>> +	 */
>> +	ret = intel_uc_resume(dev_priv);
>> +	if (ret)
>> +		i915_gem_set_wedged(dev_priv);
> I'm very sure we want to bring the system up with a backup of ELSP only
> submission *even* if we used GuC when going to sleep.
Yes. In the next rev I am removing this wedged.
>
>> @@ -4571,7 +4573,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>>   	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>>   		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>>   
>> -	intel_guc_suspend(dev_priv);
>> +	ret = intel_uc_suspend(dev_priv);
>> +	if (ret)
>> +		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
> Isn't that an exxaragation when we still may have functional ELSP?
>
> It may not be a bad idea to first bring up the ELSP submission and when
> GuC has loaded, switch over. To get rid of the latency.
Yes. will be removing this wedged.
>
>> @@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
>>   	 */
>>   	dev_priv->gt.resume(dev_priv);
>>   
>> -	intel_guc_resume(dev_priv);
>> +	/*
>> +	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
>> +	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
>> +	 * status here to see if GuC is still loaded and just do GuC resume
>> +	 * without reloading the firmware back.
>> +	 */
> Again, we're resetting at both directions, going to suspend and on
> resume too. I'd classify these bot as more of a TODOs, because we only
> get some latency that can be improved on, no broken behaviour to fix.
Yes. Will mark these as TODO.
Thanks for the review.
> Regards, Joonas

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