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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
<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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
