Re: [Intel-gfx] [PATCH 5/9] drm/i915: Rename drm_gem_object_unreference in preparation for lockless free
On Wed, Jul 20, 2016 at 12:51:31PM +0100, Dave Gordon wrote: > On 20/07/16 12:43, Joonas Lahtinen wrote: > >On ke, 2016-07-20 at 09:59 +0100, Chris Wilson wrote: > > > >>diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > >>b/drivers/gpu/drm/i915/intel_guc_loader.c > >>index 605c69658d2c..75a1496ceb6f 100644 > >>--- a/drivers/gpu/drm/i915/intel_guc_loader.c > >>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c > >>@@ -737,13 +737,15 @@ void intel_guc_fini(struct drm_device *dev) > >>struct drm_i915_private *dev_priv = to_i915(dev); > >>struct intel_guc_fw *guc_fw = _priv->guc.guc_fw; > >> > >>+ if (!guc_fw->guc_fw_obj) > >>+ return; > >>+ > > > >This is definitely not a mechanical change, should be split to separate > >patch (maybe outside of this series too). > > > >With that split out, > > > >Reviewed-by: Joonas Lahtinen> > > >Regards, Joonas > > > >>mutex_lock(>struct_mutex); > >>direct_interrupts_to_host(dev_priv); > >>i915_guc_submission_disable(dev_priv); > >>i915_guc_submission_fini(dev_priv); > >> > >>- if (guc_fw->guc_fw_obj) > >>- drm_gem_object_unreference(_fw->guc_fw_obj->base); > >>+ i915_gem_object_put(guc_fw->guc_fw_obj); > >>guc_fw->guc_fw_obj = NULL; > >>mutex_unlock(>struct_mutex); > > In my version of this set of transformations, I allowed the > reference-releasing function (equivalent to i915_gem_object_put() > here) to be called with NULL as a parameter; > drm_gem_object_unreference() can already handle NULL, and since we > now guarantee gem-obj == NULL <=> i915-obj == NULL, it's safe to > pass it though. That allows quite a few simplifications in various > bits of cleanup code. There are 3 places where this applies, 2 of which are inside the guc. It is the guc that is where the coventions breakdown. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Rename drm_gem_object_unreference in preparation for lockless free
On 20/07/16 12:43, Joonas Lahtinen wrote: On ke, 2016-07-20 at 09:59 +0100, Chris Wilson wrote: diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 605c69658d2c..75a1496ceb6f 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -737,13 +737,15 @@ void intel_guc_fini(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct intel_guc_fw *guc_fw = _priv->guc.guc_fw; + if (!guc_fw->guc_fw_obj) + return; + This is definitely not a mechanical change, should be split to separate patch (maybe outside of this series too). With that split out, Reviewed-by: Joonas LahtinenRegards, Joonas mutex_lock(>struct_mutex); direct_interrupts_to_host(dev_priv); i915_guc_submission_disable(dev_priv); i915_guc_submission_fini(dev_priv); - if (guc_fw->guc_fw_obj) - drm_gem_object_unreference(_fw->guc_fw_obj->base); + i915_gem_object_put(guc_fw->guc_fw_obj); guc_fw->guc_fw_obj = NULL; mutex_unlock(>struct_mutex); In my version of this set of transformations, I allowed the reference-releasing function (equivalent to i915_gem_object_put() here) to be called with NULL as a parameter; drm_gem_object_unreference() can already handle NULL, and since we now guarantee gem-obj == NULL <=> i915-obj == NULL, it's safe to pass it though. That allows quite a few simplifications in various bits of cleanup code. .Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Rename drm_gem_object_unreference in preparation for lockless free
On ke, 2016-07-20 at 09:59 +0100, Chris Wilson wrote: > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index 605c69658d2c..75a1496ceb6f 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -737,13 +737,15 @@ void intel_guc_fini(struct drm_device *dev) > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_guc_fw *guc_fw = _priv->guc.guc_fw; > > + if (!guc_fw->guc_fw_obj) > + return; > + This is definitely not a mechanical change, should be split to separate patch (maybe outside of this series too). With that split out, Reviewed-by: Joonas LahtinenRegards, Joonas > mutex_lock(>struct_mutex); > direct_interrupts_to_host(dev_priv); > i915_guc_submission_disable(dev_priv); > i915_guc_submission_fini(dev_priv); > > - if (guc_fw->guc_fw_obj) > - drm_gem_object_unreference(_fw->guc_fw_obj->base); > + i915_gem_object_put(guc_fw->guc_fw_obj); > guc_fw->guc_fw_obj = NULL; > mutex_unlock(>struct_mutex); > -- 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