convert dc to using krefs for object reference counts

PATCH In review Dave Airlie 3 participants 10 messages Oct 3, 2017 2 messages not in the archive

Subsystems (3)

DRM drivers Radeon and amdgpu DRM drivers AMD display core

lore

Share & tools

lore

Dave Airlie

author

2017-10-03 02:38 UTC #0 lore raw

This series might be a bit of a too big step :-)

So in the kernel we use krefs for reference counts for lots of good reasons, the main one, is no bugs with roll your own atomic reference counting for structs, which lots of this code is.

I've ported all the structs I could find using hand rolled atomic reference counters to use krefs.

Dave.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 20171003023902.5036-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Dave Airlie

author

2017-10-03 02:38 UTC #2 lore raw

PATCH 1/6 amdgpu/dc: convert dc_transfer to use a kref.

From: Dave Airlie <[email protected]>

Rolling your own atomic ref counts is frowned upon.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc_surface.c | 17 +++++++++--------
 drivers/gpu/drm/amd/display/dc/dc.h              |  2 +-
 drivers/gpu/drm/amd/display/dc/os_types.h        |  2 ++
 3 files changed, 12 insertions(+), 9 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +9 -8

index ff07105..c2168df 100644
@@ -161,17 +161,18 @@ struct dc_gamma *dc_create_gamma()
 
 void dc_transfer_func_retain(struct dc_transfer_func *tf)
 {
-	ASSERT(atomic_read(&tf->ref_count) > 0);
-	atomic_inc(&tf->ref_count);
+	kref_get(&tf->refcount);
 }
 
-void dc_transfer_func_release(struct dc_transfer_func *tf)
+static void dc_transfer_func_free(struct kref *kref)
 {
-	ASSERT(atomic_read(&tf->ref_count) > 0);
-	atomic_dec(&tf->ref_count);
+	struct dc_transfer_func *tf = container_of(kref, struct dc_transfer_func, refcount);
+	kfree(tf);
+}
 
-	if (atomic_read(&tf->ref_count) == 0)
-		kfree(tf);
+void dc_transfer_func_release(struct dc_transfer_func *tf)
+{
+	kref_put(&tf->refcount, dc_transfer_func_free);
 }
 
 struct dc_transfer_func *dc_create_transfer_func()

@@ -181,7 +182,7 @@ struct dc_transfer_func *dc_create_transfer_func()
 	if (tf == NULL)
 		goto alloc_fail;
 
-	atomic_inc(&tf->ref_count);
+	kref_init(&tf->refcount);
 
 	return tf;

drivers/gpu/drm/amd/display/dc/dc.h +1 -1

index 9c0e000..17f0c73 100644
@@ -334,11 +334,11 @@ enum dc_transfer_func_predefined {
 };
 
 struct dc_transfer_func {
+	struct kref refcount;
 	struct dc_transfer_func_distributed_points tf_pts;
 	enum dc_transfer_func_type type;
 	enum dc_transfer_func_predefined tf;
 	struct dc_context *ctx;
-	atomic_t ref_count;
 };
 
 /*

drivers/gpu/drm/amd/display/dc/os_types.h +2 -0

index 27ed2a6..e0cd527 100644
@@ -32,6 +32,8 @@
 #include <linux/types.h>
 #include <drm/drmP.h>
 
+#include <linux/kref.h>
+
 #include "cgs_linux.h"
 
 #if defined(__BIG_ENDIAN) && !defined(BIGENDIAN_CPU)

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 20171003023902.5036-2-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Christian König

2017-10-03 07:48 UTC #4 lore raw

Re: [PATCH 1/6] amdgpu/dc: convert dc_transfer to use a kref.

Am 03.10.2017 um 04:38 schrieb Dave Airlie:

Quoted 21 lines

From: Dave Airlie <[email protected]>

Rolling your own atomic ref counts is frowned upon.

Signed-off-by Dave Airlie <[email protected]>

---
  drivers/gpu/drm/amd/display/dc/core/dc_surface.c | 17 +++++++++--------
  drivers/gpu/drm/amd/display/dc/dc.h              |  2 +-
  drivers/gpu/drm/amd/display/dc/os_types.h        |  2 ++
  3 files changed, 12 insertions(+), 9 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +0 -2

index ff07105..c2168df 100644
@@ -161,17 +161,18 @@ struct dc_gamma *dc_create_gamma()
 
  void dc_transfer_func_retain(struct dc_transfer_func *tf)
  {
-	ASSERT(atomic_read(&tf->ref_count) > 0);
-	atomic_inc(&tf->ref_count);

drivers/gpu/drm/amd/display/dc/core/dc_surface.c struct dc_gamma *dc_create_gamma() · line 170

Quoted 10 lines

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +4 -3

+	kref_get(&tf->refcount);
  }
 
-void dc_transfer_func_release(struct dc_transfer_func *tf)
+static void dc_transfer_func_free(struct kref *kref)
  {
-	ASSERT(atomic_read(&tf->ref_count) > 0);
-	atomic_dec(&tf->ref_count);
+	struct dc_transfer_func *tf = container_of(kref, struct dc_transfer_func, refcount);
+	kfree(tf);

Minor style nit pick, but we should have a new line between declaration and code. Alternative I could also like with "kfree(container....)".

Not that I care to much, but I had to spend a good part of my time beating people at AMD to come up with good kernel coding style :)

Christian.

Quoted 41 lines

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +5 -3

+}
 
-	if (atomic_read(&tf->ref_count) == 0)
-		kfree(tf);
+void dc_transfer_func_release(struct dc_transfer_func *tf)
+{
+	kref_put(&tf->refcount, dc_transfer_func_free);
  }
 
  struct dc_transfer_func *dc_create_transfer_func()

@@ -181,7 +182,7 @@ struct dc_transfer_func *dc_create_transfer_func()
  	if (tf == NULL)
  		goto alloc_fail;
 
-	atomic_inc(&tf->ref_count);
+	kref_init(&tf->refcount);
 
  	return tf;

drivers/gpu/drm/amd/display/dc/dc.h +1 -1

index 9c0e000..17f0c73 100644
@@ -334,11 +334,11 @@ enum dc_transfer_func_predefined {
  };
 
  struct dc_transfer_func {
+	struct kref refcount;
  	struct dc_transfer_func_distributed_points tf_pts;
  	enum dc_transfer_func_type type;
  	enum dc_transfer_func_predefined tf;
  	struct dc_context *ctx;
-	atomic_t ref_count;
  };
 
  /*

drivers/gpu/drm/amd/display/dc/os_types.h +0 -0

index 27ed2a6..e0cd527 100644

drivers/gpu/drm/amd/display/dc/os_types.h line 39

Quoted 8 lines

drivers/gpu/drm/amd/display/dc/os_types.h +2 -0

@@ -32,6 +32,8 @@
  #include <linux/types.h>
  #include <drm/drmP.h>
 
+#include <linux/kref.h>
+
  #include "cgs_linux.h"
 
  #if defined(__BIG_ENDIAN) && !defined(BIGENDIAN_CPU)

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dave Airlie

author

2017-10-03 02:38 UTC #5 lore raw

PATCH 2/6 amdgpu/dc: convert dc_gamma to kref reference counting.

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

Rolling your own reference counting is frowned upon.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc_surface.c | 19 +++++++++----------
 drivers/gpu/drm/amd/display/dc/dc_hw_types.h     |  4 +---
 2 files changed, 10 insertions(+), 13 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +9 ~~-10~~

index c2168df..0950075 100644
@@ -129,18 +129,18 @@ void dc_plane_state_release(struct dc_plane_state *plane_state)
 
 void dc_gamma_retain(struct dc_gamma *gamma)
 {
-	ASSERT(atomic_read(&gamma->ref_count) > 0);
-	atomic_inc(&gamma->ref_count);
+	kref_get(&gamma->refcount);
 }
 
-void dc_gamma_release(struct dc_gamma **gamma)
+static void dc_gamma_free(struct kref *kref)
 {
-	ASSERT(atomic_read(&(*gamma)->ref_count) > 0);
-	atomic_dec(&(*gamma)->ref_count);
-
-	if (atomic_read(&(*gamma)->ref_count) == 0)
-		kfree((*gamma));
+	struct dc_gamma *gamma = container_of(kref, struct dc_gamma, refcount);
+	kfree(gamma);
+}
 
+void dc_gamma_release(struct dc_gamma **gamma)
+{
+	kref_put(&(*gamma)->refcount, dc_gamma_free);
 	*gamma = NULL;
 }

@@ -151,8 +151,7 @@ struct dc_gamma *dc_create_gamma()
 	if (gamma == NULL)
 		goto alloc_fail;
 
-	atomic_inc(&gamma->ref_count);
-
+	kref_init(&gamma->refcount);
 	return gamma;
 
 alloc_fail:

drivers/gpu/drm/amd/display/dc/dc_hw_types.h +1 -3

index 94f83cd..4ab1093 100644
@@ -420,6 +420,7 @@ enum dc_gamma_type {
 };
 
 struct dc_gamma {
+	struct kref refcount;
 	enum dc_gamma_type type;
 	unsigned int num_entries;

@@ -431,9 +432,6 @@ struct dc_gamma {
 
 	/* private to DC core */
 	struct dc_context *ctx;
-
-	/* private to dc_surface.c */
-	atomic_t ref_count;
 };
 
 /* Used by both ipp amd opp functions*/

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dave Airlie

author

2017-10-03 02:38 UTC #6 lore raw

PATCH 3/6 amdgpu/dc: use kref for dc_plane_state.

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc_surface.c | 20 ++++++++++----------
 drivers/gpu/drm/amd/display/dc/dc.h              |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_surface.c +10 ~~-10~~

index 0950075..d43783a 100644
@@ -71,8 +71,8 @@ struct dc_plane_state *dc_create_plane_state(struct dc *dc)
 	if (NULL == plane_state)
 		return NULL;
 
+	kref_init(&plane_state->refcount);
 	construct(core_dc->ctx, plane_state);
-	atomic_inc(&plane_state->ref_count);
 
 	return plane_state;
 }

@@ -112,19 +112,19 @@ const struct dc_plane_status *dc_plane_get_status(
 
 void dc_plane_state_retain(struct dc_plane_state *plane_state)
 {
-	ASSERT(atomic_read(&plane_state->ref_count) > 0);
-	atomic_inc(&plane_state->ref_count);
+	kref_get(&plane_state->refcount);
 }
 
-void dc_plane_state_release(struct dc_plane_state *plane_state)
+static void dc_plane_state_free(struct kref *kref)
 {
-	ASSERT(atomic_read(&plane_state->ref_count) > 0);
-	atomic_dec(&plane_state->ref_count);
+	struct dc_plane_state *plane_state = container_of(kref, struct dc_plane_state, refcount);
+	destruct(plane_state);
+	kfree(plane_state);
+}
 
-	if (atomic_read(&plane_state->ref_count) == 0) {
-		destruct(plane_state);
-		kfree(plane_state);
-	}
+void dc_plane_state_release(struct dc_plane_state *plane_state)
+{
+	kref_put(&plane_state->refcount, dc_plane_state_free);
 }
 
 void dc_gamma_retain(struct dc_gamma *gamma)

drivers/gpu/drm/amd/display/dc/dc.h +1 -1

index 17f0c73..c4d89d2 100644
@@ -385,7 +385,7 @@ struct dc_plane_state {
 
 	/* private to dc_surface.c */
 	enum dc_irq_source irq_source;
-	atomic_t ref_count;
+	struct kref refcount;
 };
 
 struct dc_plane_info {

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dave Airlie

author

2017-10-03 02:39 UTC #7 lore raw

PATCH 4/6 amdgpu/dc: convert dc_stream_state to kref.

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc_stream.c | 22 +++++++++++-----------
 drivers/gpu/drm/amd/display/dc/dc.h             |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_stream.c +11 ~~-11~~

index a9919641..23df7bc 100644
@@ -118,20 +118,21 @@ static void destruct(struct dc_stream_state *stream)
 
 void dc_stream_retain(struct dc_stream_state *stream)
 {
-	ASSERT(atomic_read(&stream->ref_count) > 0);
-	atomic_inc(&stream->ref_count);
+	kref_get(&stream->refcount);
+}
+
+static void dc_stream_free(struct kref *kref)
+{
+	struct dc_stream_state *stream = container_of(kref, struct dc_stream_state, refcount);
+
+	destruct(stream);
+	kfree(stream);
 }
 
 void dc_stream_release(struct dc_stream_state *stream)
 {
 	if (stream != NULL) {
-		ASSERT(atomic_read(&stream->ref_count) > 0);
-		atomic_dec(&stream->ref_count);
-
-		if (atomic_read(&stream->ref_count) == 0) {
-			destruct(stream);
-			kfree(stream);
-		}
+		kref_put(&stream->refcount, dc_stream_free);
 	}
 }

@@ -149,10 +150,9 @@ struct dc_stream_state *dc_create_stream_for_sink(
 
 	construct(stream, sink);
 
-	atomic_inc(&stream->ref_count);
+	kref_init(&stream->refcount);
 
 	return stream;
-
 }
 
 struct dc_stream_status *dc_stream_get_status(

drivers/gpu/drm/amd/display/dc/dc.h +1 -1

index c4d89d2..abba134 100644
@@ -562,7 +562,7 @@ struct dc_stream_state {
 	struct dc_stream_status status;
 
 	/* from stream struct */
-	atomic_t ref_count;
+	struct kref refcount;
 };
 
 struct dc_stream_update {

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dave Airlie

author

2017-10-03 02:39 UTC #8 lore raw

PATCH 5/6 amdgpu/dc: convert dc_sink to kref.

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

Refcounts use krefs.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc_sink.c | 20 ++++++++++----------
 drivers/gpu/drm/amd/display/dc/dc.h           |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc_sink.c +10 ~~-10~~

index b3bbafc..f2b2e82 100644
@@ -63,19 +63,19 @@ static bool construct(struct dc_sink *sink, const struct dc_sink_init_data *init
 
 void dc_sink_retain(struct dc_sink *sink)
 {
-	ASSERT(atomic_read(&sink->ref_count) > 0);
-	atomic_inc(&sink->ref_count);
+	kref_get(&sink->refcount);
 }
 
-void dc_sink_release(struct dc_sink *sink)
+static void dc_sink_free(struct kref *kref)
 {
-	ASSERT(atomic_read(&sink->ref_count) > 0);
-	atomic_dec(&sink->ref_count);
+	struct dc_sink *sink = container_of(kref, struct dc_sink, refcount);
+	destruct(sink);
+	kfree(sink);
+}
 
-	if (atomic_read(&sink->ref_count) == 0) {
-		destruct(sink);
-		kfree(sink);
-	}
+void dc_sink_release(struct dc_sink *sink)
+{
+	kref_put(&sink->refcount, dc_sink_free);
 }
 
 struct dc_sink *dc_sink_create(const struct dc_sink_init_data *init_params)

@@ -88,7 +88,7 @@ struct dc_sink *dc_sink_create(const struct dc_sink_init_data *init_params)
 	if (false == construct(sink, init_params))
 		goto construct_fail;
 
-	atomic_inc(&sink->ref_count);
+	kref_init(&sink->refcount);
 
 	return sink;

drivers/gpu/drm/amd/display/dc/dc.h +1 -1

index abba134..31952b9 100644
@@ -972,7 +972,7 @@ struct dc_sink {
 	struct dc_context *ctx;
 
 	/* private to dc_sink.c */
-	atomic_t ref_count;
+	struct kref refcount;
 };
 
 void dc_sink_retain(struct dc_sink *sink);

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Dave Airlie

author

2017-10-03 02:39 UTC #9 lore raw

PATCH 6/6 amdgpu/dc: use kref for dc_state.

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

I'm not a huge fan of those copying around refcounts bits, might want to consider alternates, but this should work for now.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/dc/core/dc.c          | 26 +++++++++++------------
 drivers/gpu/drm/amd/display/dc/core/dc_resource.c |  4 ++--
 drivers/gpu/drm/amd/display/dc/inc/core_types.h   |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

drivers/gpu/drm/amd/display/dc/core/dc.c +13 ~~-13~~

index c8235b0..e8634b8 100644
@@ -964,25 +964,25 @@ struct dc_state *dc_create_state(void)
 	if (!context)
 		return NULL;
 
-	atomic_inc(&context->ref_count);
+	kref_init(&context->refcount);
 	return context;
 }
 
 void dc_retain_state(struct dc_state *context)
 {
-	ASSERT(atomic_read(&context->ref_count) > 0);
-	atomic_inc(&context->ref_count);
+	kref_get(&context->refcount);
 }
 
-void dc_release_state(struct dc_state *context)
+static void dc_state_free(struct kref *kref)
 {
-	ASSERT(atomic_read(&context->ref_count) > 0);
-	atomic_dec(&context->ref_count);
+	struct dc_state *context = container_of(kref, struct dc_state, refcount);
+	dc_resource_state_destruct(context);
+	kfree(context);
+}
 
-	if (atomic_read(&context->ref_count) == 0) {
-		dc_resource_state_destruct(context);
-		kfree(context);
-	}
+void dc_release_state(struct dc_state *context)
+{
+	kref_put(&context->refcount, dc_state_free);
 }
 
 static bool is_surface_in_context(

@@ -1566,7 +1566,7 @@ void dc_set_power_state(
 	struct dc *dc,
 	enum dc_acpi_cm_power_state power_state)
 {
-	atomic_t ref_count;
+	struct kref refcount;
 
 	switch (power_state) {
 	case DC_ACPI_CM_POWER_STATE_D0:

@@ -1584,12 +1584,12 @@ void dc_set_power_state(
 		 */
 
 		/* Preserve refcount */
-		ref_count = dc->current_state->ref_count;
+		refcount = dc->current_state->refcount;
 		dc_resource_state_destruct(dc->current_state);
 		memset(dc->current_state, 0,
 				sizeof(*dc->current_state));
 
-		dc->current_state->ref_count = ref_count;
+		dc->current_state->refcount = refcount;
 
 		break;
 	}

drivers/gpu/drm/amd/display/dc/core/dc_resource.c +2 -2

index 1832f25..057bfe6 100644
@@ -2433,7 +2433,7 @@ void dc_resource_state_copy_construct(
 		struct dc_state *dst_ctx)
 {
 	int i, j;
-	atomic_t ref_count = dst_ctx->ref_count;
+	struct kref refcount = dst_ctx->refcount;
 
 	*dst_ctx = *src_ctx;

@@ -2456,7 +2456,7 @@ void dc_resource_state_copy_construct(
 	}
 
 	/* context refcount should not be overridden */
-	dst_ctx->ref_count = ref_count;
+	dst_ctx->refcount = refcount;
 
 }

drivers/gpu/drm/amd/display/dc/inc/core_types.h +1 -1

index 1a1d322..d786ec4 100644
@@ -268,7 +268,7 @@ struct dc_state {
 
 	struct display_clock *dis_clk;
 
-	atomic_t ref_count;
+	struct kref refcount;
 };
 
 #endif /* _CORE_TYPES_H_ */

Signature · 6 lines

2.9.5

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Christian König

2017-10-03 07:50 UTC #10 lore raw

In reply to a message not in the archive

Apart from a minor style nit pick which was repeated a few times the series is Reviewed-by: Christian König <[email protected]>.

Regards,
Christian.

Am 03.10.2017 um 04:38 schrieb Dave Airlie:

Quoted 9 lines

This series might be a bit of a too big step :-)
So in the kernel we use krefs for reference counts for lots of good reasons, the main one, is no bugs with roll your own atomic reference counting for structs, which lots of this code is.
I've ported all the structs I could find using hand rolled atomic reference counters to use krefs.

Quoted 6 lines

Dave.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Harry Wentland

Reviewed-by

2017-10-03 21:14 UTC #11 lore raw

In reply to a message not in the archive

Quoted 2 lines · Dave Airlie

This series might be a bit of a too big step :-)

Should be okay since ref counts are pretty self-contained.

Quoted 7 lines

So in the kernel we use krefs for reference counts for lots of good reasons, the main one, is no bugs with roll your own atomic reference counting for structs, which lots of this code is.
I've ported all the structs I could find using hand rolled atomic reference counters to use krefs.

I'd still echo Christian's nitpick but knowing the state of DC code style this series is

Reviewed-by Harry Wentland <[email protected]>

Harry

Quoted 7 lines

Dave.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx