Re: [Intel-gfx] [PATCH 5/9] drm/i915: Rename drm_gem_object_unreference in preparation for lockless free

[Chris Wilson]

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

[Dave Gordon]

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.


.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

[Joonas Lahtinen]

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

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