Re: [Intel-gfx] [PATCH 26/41] drm: Add reference counting to drm_atomic_state

2016-10-17 Thread Daniel Vetter 
On Fri, Oct 14, 2016 at 01:18:18PM +0100, Chris Wilson wrote:
> drm_atomic_state has a complicated single owner model that tracks the
> single reference from allocation through to destruction on another
> thread - or perhaps on a local error path. We can simplify this tracking
> by using reference counting (at a cost of a few more atomics). This is
> even more beneficial when the lifetime of the state becomes more
> convoluted than being passed to a single worker thread for the commit.
> 
> v2: Double check !intel atomic_commit functions for missing gets
> v3: Update kerneldocs
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: dri-de...@lists.freedesktop.org
> Reviewed-by: Eric Engestrom 
> Reviewed-by: Sean Paul 

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c |  3 +-
>  drivers/gpu/drm/drm_atomic.c | 25 +++
>  drivers/gpu/drm/drm_atomic_helper.c  | 98 
> +++-
>  drivers/gpu/drm/drm_fb_helper.c  |  9 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |  3 +-
>  drivers/gpu/drm/i915/i915_debugfs.c  |  5 +-
>  drivers/gpu/drm/i915/intel_display.c | 31 +
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  3 +-
>  drivers/gpu/drm/msm/msm_atomic.c |  3 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c   |  3 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c|  3 +-
>  drivers/gpu/drm/sti/sti_drv.c|  3 +-
>  drivers/gpu/drm/tegra/drm.c  |  3 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 -
>  drivers/gpu/drm/vc4/vc4_kms.c|  3 +-
>  include/drm/drm_atomic.h | 31 -
>  include/drm/drm_plane.h  |  1 -
>  18 files changed, 102 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 5f484310bee9..9f6222895212 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -464,7 +464,7 @@ atmel_hlcdc_dc_atomic_complete(struct 
> atmel_hlcdc_dc_commit *commit)
>  
>   drm_atomic_helper_cleanup_planes(dev, old_state);
>  
> - drm_atomic_state_free(old_state);
> + drm_atomic_state_put(old_state);
>  
>   /* Complete the commit, wake up any waiter. */
>   spin_lock(>commit.wait.lock);
> @@ -521,6 +521,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
> *dev,
>   /* Swap the state, this is the point of no return. */
>   drm_atomic_helper_swap_state(state, true);
>  
> + drm_atomic_state_get(state);
>   if (async)
>   queue_work(dc->wq, >work);
>   else
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 23739609427d..5dd70540219c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -74,6 +74,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
>  {
> + kref_init(>ref);
> +
>   /* TODO legacy paths should maybe do a better job about
>* setting this appropriately?
>*/
> @@ -215,22 +217,16 @@ void drm_atomic_state_clear(struct drm_atomic_state 
> *state)
>  EXPORT_SYMBOL(drm_atomic_state_clear);
>  
>  /**
> - * drm_atomic_state_free - free all memory for an atomic state
> - * @state: atomic state to deallocate
> + * __drm_atomic_state_free - free all memory for an atomic state
> + * @ref: This atomic state to deallocate
>   *
>   * This frees all memory associated with an atomic state, including all the
>   * per-object state for planes, crtcs and connectors.
>   */
> -void drm_atomic_state_free(struct drm_atomic_state *state)
> +void __drm_atomic_state_free(struct kref *ref)
>  {
> - struct drm_device *dev;
> - struct drm_mode_config *config;
> -
> - if (!state)
> - return;
> -
> - dev = state->dev;
> - config = >mode_config;
> + struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
> + struct drm_mode_config *config = >dev->mode_config;
>  
>   drm_atomic_state_clear(state);
>  
> @@ -243,7 +239,7 @@ void drm_atomic_state_free(struct drm_atomic_state *state)
>   kfree(state);
>   }
>  }
> -EXPORT_SYMBOL(drm_atomic_state_free);
> +EXPORT_SYMBOL(__drm_atomic_state_free);
>  
>  /**
>   * drm_atomic_get_crtc_state - get crtc state
> @@ -1742,7 +1738,7 @@ retry:
>   if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>   /*
>* Unlike commit, check_only does not clean up state.
> -  * Below we call drm_atomic_state_free for it.
> +  * Below we call drm_atomic_state_put for it.
>*/
>

[Intel-gfx] [PATCH 26/41] drm: Add reference counting to drm_atomic_state

2016-10-14 Thread Chris Wilson 
drm_atomic_state has a complicated single owner model that tracks the
single reference from allocation through to destruction on another
thread - or perhaps on a local error path. We can simplify this tracking
by using reference counting (at a cost of a few more atomics). This is
even more beneficial when the lifetime of the state becomes more
convoluted than being passed to a single worker thread for the commit.

v2: Double check !intel atomic_commit functions for missing gets
v3: Update kerneldocs

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-de...@lists.freedesktop.org
Reviewed-by: Eric Engestrom 
Reviewed-by: Sean Paul 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c |  3 +-
 drivers/gpu/drm/drm_atomic.c | 25 +++
 drivers/gpu/drm/drm_atomic_helper.c  | 98 +++-
 drivers/gpu/drm/drm_fb_helper.c  |  9 +--
 drivers/gpu/drm/exynos/exynos_drm_drv.c  |  3 +-
 drivers/gpu/drm/i915/i915_debugfs.c  |  5 +-
 drivers/gpu/drm/i915/intel_display.c | 31 +
 drivers/gpu/drm/i915/intel_sprite.c  |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  3 +-
 drivers/gpu/drm/msm/msm_atomic.c |  3 +-
 drivers/gpu/drm/omapdrm/omap_drv.c   |  3 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c|  3 +-
 drivers/gpu/drm/sti/sti_drv.c|  3 +-
 drivers/gpu/drm/tegra/drm.c  |  3 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 -
 drivers/gpu/drm/vc4/vc4_kms.c|  3 +-
 include/drm/drm_atomic.h | 31 -
 include/drm/drm_plane.h  |  1 -
 18 files changed, 102 insertions(+), 131 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 5f484310bee9..9f6222895212 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -464,7 +464,7 @@ atmel_hlcdc_dc_atomic_complete(struct atmel_hlcdc_dc_commit 
*commit)
 
drm_atomic_helper_cleanup_planes(dev, old_state);
 
-   drm_atomic_state_free(old_state);
+   drm_atomic_state_put(old_state);
 
/* Complete the commit, wake up any waiter. */
spin_lock(>commit.wait.lock);
@@ -521,6 +521,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
*dev,
/* Swap the state, this is the point of no return. */
drm_atomic_helper_swap_state(state, true);
 
+   drm_atomic_state_get(state);
if (async)
queue_work(dc->wq, >work);
else
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 23739609427d..5dd70540219c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -74,6 +74,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
 int
 drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
 {
+   kref_init(>ref);
+
/* TODO legacy paths should maybe do a better job about
 * setting this appropriately?
 */
@@ -215,22 +217,16 @@ void drm_atomic_state_clear(struct drm_atomic_state 
*state)
 EXPORT_SYMBOL(drm_atomic_state_clear);
 
 /**
- * drm_atomic_state_free - free all memory for an atomic state
- * @state: atomic state to deallocate
+ * __drm_atomic_state_free - free all memory for an atomic state
+ * @ref: This atomic state to deallocate
  *
  * This frees all memory associated with an atomic state, including all the
  * per-object state for planes, crtcs and connectors.
  */
-void drm_atomic_state_free(struct drm_atomic_state *state)
+void __drm_atomic_state_free(struct kref *ref)
 {
-   struct drm_device *dev;
-   struct drm_mode_config *config;
-
-   if (!state)
-   return;
-
-   dev = state->dev;
-   config = >mode_config;
+   struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
+   struct drm_mode_config *config = >dev->mode_config;
 
drm_atomic_state_clear(state);
 
@@ -243,7 +239,7 @@ void drm_atomic_state_free(struct drm_atomic_state *state)
kfree(state);
}
 }
-EXPORT_SYMBOL(drm_atomic_state_free);
+EXPORT_SYMBOL(__drm_atomic_state_free);
 
 /**
  * drm_atomic_get_crtc_state - get crtc state
@@ -1742,7 +1738,7 @@ retry:
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
/*
 * Unlike commit, check_only does not clean up state.
-* Below we call drm_atomic_state_free for it.
+* Below we call drm_atomic_state_put for it.
 */
ret = drm_atomic_check_only(state);
} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
@@ -1775,8 +1771,7 @@ out:
goto retry;
}
 
-   if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
-   drm_atomic_state_free(state);