[Intel-gfx] [PATCH v12 06/11] drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission

[Sagar Arun Kamble]

On 9/29/2017 5:57 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> During GuC load/enable, state is setup by driver that can be looked at
>> while disabling. So remove the check for i915.enable_guc_submission
>> parameter in those functions.
>>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> 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>
> <SNIP>
>
>> @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc)
>>   
>>   static void guc_ads_destroy(struct intel_guc *guc)
>>   {
>> -	i915_vma_unpin_and_release(&guc->ads_vma);
>> +	if (guc->ads_vma)
> GEM_BUG_ON(!guc->ads_vma);
This check was unnecessary. Suggestion from Chris was to make these 
destroys be self-check based instead of invoking
based on module parameters like enable_guc_submission/loading. In my new 
patch I have removed these checks.
https://patchwork.freedesktop.org/patch/179683/
>
>> +		i915_vma_unpin_and_release(&guc->ads_vma);
>>   }
>>   
>>   /*
>> @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>>   {
>>   	struct intel_guc *guc = &dev_priv->guc;
>>   
>> +	WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids));
>>   	ida_destroy(&guc->stage_ids);
>>   	guc_ads_destroy(guc);
>>   	intel_guc_log_destroy(guc);
>> -	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> -	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> +	if (guc->stage_desc_pool) {
> GEM_BUG_ON(!guc->stage_desc_pol) is the right thing.
stage_desc_pool check is used to enter submission_fini in my latest 
patch. Other than that there are no more checks needed.
https://patchwork.freedesktop.org/patch/179683/
>
>> +		i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> +		i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> +	}
> I'm generally against conditional teardown. If the _init did not fully
> succeed, the _fini is never supposed to be called.
Plan is to replace the module parameter based condition to driver state 
based condition.
>>   static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>> @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>>   {
>>   	struct intel_guc *guc = &dev_priv->guc;
>>   
> We may want document pre-requirements assert_lockdep_held() in
> enable/disable submission funcs, for a good measure. Then it'll be
> easier to convert away from struct_mutex when the time comes.
We removed lockdep assert as mutex is needed by internal functions which 
already have the asserts.
>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 6571d96..73333b6 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc)
>>   void intel_guc_log_destroy(struct intel_guc *guc)
>>   {
>>   	guc_log_runtime_destroy(guc);
>> -	i915_vma_unpin_and_release(&guc->log.vma);
>> +	if (guc->log.vma)
>> +		i915_vma_unpin_and_release(&guc->log.vma);
> Again, GEM_BUG_ON(!guc->log.vma);
This is unnecessary check.
>
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>   err_log_capture:
>>   	guc_capture_load_err_log(guc);
>>   err_submission:
>> -	if (i915_modparams.enable_guc_submission)
>> -		i915_guc_submission_fini(dev_priv);
>> +	i915_guc_submission_fini(dev_priv);
> No, no unconditional calling of _fini if the _init is not uncoditional
> too. You can drop both checks down to the submission_init/_fini funcs
> if you want to. For me it's more clear if they're here.
>
> Inside the funcs or right before calling them, when called just from
> one place (like I'd prefer here), but most importantly it has to be
> symmetric.
>
> Regards, Joonas
_fini is still conditional, but inside the function based on various 
states set.