[Intel-gfx] [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup

[Oscar Mateo]

On 03/23/2017 03:57 PM, Chris Wilson wrote:
> On Wed, Mar 22, 2017 at 10:39:46AM -0700, Oscar Mateo wrote:
>> Starting with intel_guc_loader, down to intel_guc_submission
>> and finally to intel_guc_log.
>>
>> v2:
>>    - Null execbuf client outside guc_client_free (Daniele)
>>    - Assert if things try to get allocated twice (Daniele/Joonas)
>>    - Null guc->log.buf_addr when destroyed (Daniele)
>>    - Newline between returning success and error labels (Joonas)
>>    - Remove some unnecessary comments (Joonas)
>>    - Keep guc_log_create_extras naming convention (Joonas)
>>    - Helper function guc_log_has_extras (Joonas)
>>    - No need for separate relay_channel create/destroy. It's just another extra.
>>    - No need to nullify guc->log.flush_wq when destroyed (Joonas)
>>    - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
>>    - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
>>    - Make sure initel_guc_fini is not called before init is ever called (Daniele)
>>
>> v3:
>>    - Remove unnecessary parenthesis (Joonas)
>>    - Check for logs enabled on debugfs registration
>>    - Rebase on top of Tvrtko's "Fix request re-submission after reset"
>>
>> v4:
>>    - Rebased
>>    - Comment around enabling/disabling interrupts inside GuC logging (Joonas)
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c            |  11 +-
>>   drivers/gpu/drm/i915/i915_gem.c            |  10 +-
>>   drivers/gpu/drm/i915/i915_guc_submission.c |  97 ++++----
>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  21 --
>>   drivers/gpu/drm/i915/intel_guc_log.c       | 364 ++++++++++++++---------------
>>   drivers/gpu/drm/i915/intel_uc.c            |  55 +++--
>>   drivers/gpu/drm/i915/intel_uc.h            |   8 +-
>>   7 files changed, 297 insertions(+), 269 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 03d9e45..6d9944a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -549,6 +549,8 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
>>   static void i915_gem_fini(struct drm_i915_private *dev_priv)
>>   {
>>   	mutex_lock(&dev_priv->drm.struct_mutex);
>> +	if (i915.enable_guc_loading)
>> +		intel_uc_fini_hw(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index fd611b4..4eb46e4 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4596,10 +4596,12 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>>   
>>   	intel_mocs_init_l3cc_table(dev_priv);
>>   
>> -	/* We can't enable contexts until all firmware is loaded */
>> -	ret = intel_uc_init_hw(dev_priv);
>> -	if (ret)
>> -		goto out;
>> +	if (i915.enable_guc_loading) {
>> +		/* We can't enable contexts until all firmware is loaded */
>> +		ret = intel_uc_init_hw(dev_priv);
>> +		if (ret)
>> +			goto out;
>> +	}
>>   
>>   out:
> I'm not happy with moving subfeature detection logic into the core GEM
> code. if (i915.enable_guc_loading) firstly should never be a module
> parameter (it's derived state!) and secondly it should reside next to
> the dependent logic and not be interrupting the central control flow.
> -Chris
What do you mean it's derived state? from what?
As for interrupting the central control flow, you are probably right, I 
can change it back (the code was refactored so many times that I cannot 
remember my logic behind that change anymore)
- Oscar