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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@chris-wilson.co.uk>
> 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>

<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);

> +		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.

> +		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.

>  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.

> 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); 

> +++ 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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx