| Message ID | 1508309222-26406-7-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/10/2017 07:46, Sagar Arun Kamble wrote: > GuC log runtime/relay channel data is released during i915 unregister, > So only GuC log vma needs to be released during submission_fini. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index a2e8114..c360b37 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) > > ida_destroy(&guc->stage_ids); > guc_ads_destroy(guc); > - intel_guc_log_destroy(guc); > + i915_vma_unpin_and_release(&guc->log.vma); > i915_gem_object_unpin_map(guc->stage_desc_pool->obj); > i915_vma_unpin_and_release(&guc->stage_desc_pool); > } > Doesn't it make more sense to hide the logging implementation details from this call site? And I can't find the remaining caller of the intel_guc_log_destroy in the current codebase? Unless it was added in one of the previous patches? Regards, Tvrtko
On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 07:46, Sagar Arun Kamble wrote: >> GuC log runtime/relay channel data is released during i915 unregister, >> So only GuC log vma needs to be released during submission_fini. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index a2e8114..c360b37 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct >> drm_i915_private *dev_priv) >> ida_destroy(&guc->stage_ids); >> guc_ads_destroy(guc); >> - intel_guc_log_destroy(guc); >> + i915_vma_unpin_and_release(&guc->log.vma); >> i915_gem_object_unpin_map(guc->stage_desc_pool->obj); >> i915_vma_unpin_and_release(&guc->stage_desc_pool); >> } >> > > Doesn't it make more sense to hide the logging implementation details > from this call site? Yes right. Will need to separate logging from submission cleanup here. > > And I can't find the remaining caller of the intel_guc_log_destroy in > the current codebase? Unless it was added in one of the previous patches? It is called during submission_init on failure and that too will need to be changed as we separate logging from submission. Thanks > > Regards, > > Tvrtko
On 18/10/2017 17:04, Sagar Arun Kamble wrote: > On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote: >> >> On 18/10/2017 07:46, Sagar Arun Kamble wrote: >>> GuC log runtime/relay channel data is released during i915 unregister, >>> So only GuC log vma needs to be released during submission_fini. >>> >>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index a2e8114..c360b37 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct >>> drm_i915_private *dev_priv) >>> ida_destroy(&guc->stage_ids); >>> guc_ads_destroy(guc); >>> - intel_guc_log_destroy(guc); >>> + i915_vma_unpin_and_release(&guc->log.vma); >>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj); >>> i915_vma_unpin_and_release(&guc->stage_desc_pool); >>> } >>> >> >> Doesn't it make more sense to hide the logging implementation details >> from this call site? > Yes right. Will need to separate logging from submission cleanup here. >> >> And I can't find the remaining caller of the intel_guc_log_destroy in >> the current codebase? Unless it was added in one of the previous patches? > It is called during submission_init on failure and that too will need to > be changed as we separate logging from submission. But never gets to run on driver unload? By design or oversight? Regards, Tvrtko
On 10/19/2017 12:48 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 17:04, Sagar Arun Kamble wrote: >> On 10/18/2017 6:42 PM, Tvrtko Ursulin wrote: >>> >>> On 18/10/2017 07:46, Sagar Arun Kamble wrote: >>>> GuC log runtime/relay channel data is released during i915 unregister, >>>> So only GuC log vma needs to be released during submission_fini. >>>> >>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> index a2e8114..c360b37 100644 >>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct >>>> drm_i915_private *dev_priv) >>>> ida_destroy(&guc->stage_ids); >>>> guc_ads_destroy(guc); >>>> - intel_guc_log_destroy(guc); >>>> + i915_vma_unpin_and_release(&guc->log.vma); >>>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj); >>>> i915_vma_unpin_and_release(&guc->stage_desc_pool); >>>> } >>>> >>> >>> Doesn't it make more sense to hide the logging implementation >>> details from this call site? >> Yes right. Will need to separate logging from submission cleanup here. >>> >>> And I can't find the remaining caller of the intel_guc_log_destroy >>> in the current codebase? Unless it was added in one of the previous >>> patches? >> It is called during submission_init on failure and that too will need >> to be changed as we separate logging from submission. > > But never gets to run on driver unload? By design or oversight? Oversight. Never gets to run on driver unload. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index a2e8114..c360b37 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1021,7 +1021,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) ida_destroy(&guc->stage_ids); guc_ads_destroy(guc); - intel_guc_log_destroy(guc); + i915_vma_unpin_and_release(&guc->log.vma); i915_gem_object_unpin_map(guc->stage_desc_pool->obj); i915_vma_unpin_and_release(&guc->stage_desc_pool); }
GuC log runtime/relay channel data is released during i915 unregister, So only GuC log vma needs to be released during submission_fini. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)