Re: [PATCH v12 05/11] drm/i915/guc: Introduce intel_uc_sanitize

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

On 9/29/2017 5:30 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> Currently GPU is reset at the end of suspend via i915_gem_sanitize.
>> On resume, GuC will not be loaded until intel_uc_init_hw happens
>> during GEM resume flow but action to exit sleep can be sent to GuC
>> considering the FW load status. To make sure we don't invoke that
>> action update GuC FW load status at the end of GPU reset as NONE.
>> load_status indicates HW state and it is sanitized through this new
>> function intel_uc_sanitize.
>>
>> v2: Rebase.
>>
>> v3: Removed intel_guc_sanitize. Marking load status as NONE at the
>> GPU reset point. (Chris/Michal)
>>
>> v4: Reinstated the uC function intel_uc_sanitize. (Michal Wajdeczko)
>>
>> 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>
> <SNIP>
>
>> @@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
>>   	return intel_guc_resume(dev_priv);
>>   }
>>   
>> +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
>> +{
>> +	/*
>> +	 * FIXME: intel_uc_resume currently depends on load_status to resume
>> +	 * GuC. Since we are resetting Full GPU at the end of suspend, let us
>> +	 * mark the load status as NONE. Once intel_uc_resume is updated to take
>> +	 * into consideration GuC load state based on WOPCM, we can skip this
>> +	 * state update.
>> +	 */
>> +	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
> With what I suggested to Michal, this would be call to
> intel_guc_sanitize() (and in future also intel_huc_sanitize()
> intel_dmc_sanitize()).
Yes.
>
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
>>   	}
>>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>   
>> +	if (engine_mask == ALL_ENGINES)
>> +		intel_uc_sanitize(dev_priv);
> We could propagate engine_mask to intel_uc_sanitize and let it decide
> what it does to keep a clear top level code flow. This also doesn't
> seem to depend on if GuC submission is enabled or not.
Sure. will make this change.
> If we want to be unconditional, wouldn't intel_guc_select_fw would not
> be more appropriate in intel_uc_sanitize?
Do we want to select different fw across resets? That would mean 
changing i915.guc_firmware_path at runtime which I guess we don't want 
do right?
> Regards, Joonas

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