Skip to contentSkip to conversation

drm: make drm_get_{connector, encoder}_name thread safe

RFC PATCH In review Jani Nikula 4 participants 10 messages May 14, 2014 – May 27, 2014

Subsystems (2)
DRM drivers DRM misc
lore
Share & tools
lore

Jani Nikula

author
#0 lore raw

cover drm: make drm_get_{connector, encoder}_name thread safe

Hi all -

This series stores connector/encoder names in the relevant structs to make the name getters thread safe.

What say you, is the wasted memory too high a price to pay for the thread safety and implementation simplicity of this approach? I think making drm_get_connector_name and drm_get_encoder_name return allocated buffers makes a lot of code really ugly and error prone.

I am assuming connector_type, connector_type_id, encoder_type, and encoder.base.id remain unchanged for the lifetime of the connector/encoder - is that a correct assumption?

BR,
Jani.

N.B. I did not actually test this...

Jani Nikula (2):
drm: store connector name in connector struct
drm: store encoder name in encoder struct

 drivers/gpu/drm/drm_crtc.c | 67 +++++++++++++++++++++++++---------------------
 include/drm/drm_crtc.h     |  4 +++
 2 files changed, 41 insertions(+), 30 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

RFC PATCH 1/2 drm: store connector name in connector struct

This makes drm_get_connector_name() thread safe.

Reference: http://lkml.kernel.org/r/[email protected]

Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++----------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 22 insertions(+), 16 deletions(-)
drivers/gpu/drm/drm_crtc.c +20 -16
index 461d19bd14ee..5781130b4126 100644
@@ -277,21 +277,11 @@ EXPORT_SYMBOL(drm_get_encoder_name);
 
 /**
  * drm_get_connector_name - return a string for connector
- * @connector: connector to compute name of
- *
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * @connector: the connector to get name for
  */
 const char *drm_get_connector_name(const struct drm_connector *connector)
 {
-	static char buf[32];
-
-	snprintf(buf, 32, "%s-%d",
-		 drm_connector_enum_list[connector->connector_type].name,
-		 connector->connector_type_id);
-	return buf;
+	return connector->name;
 }
 EXPORT_SYMBOL(drm_get_connector_name);
 

@@ -824,7 +814,7 @@ int drm_connector_init(struct drm_device *dev,
 
 	ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
 	if (ret)
-		goto out;
+		goto out_unlock;
 
 	connector->base.properties = &connector->properties;
 	connector->dev = dev;

@@ -834,9 +824,17 @@ int drm_connector_init(struct drm_device *dev,
 		ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
 	if (connector->connector_type_id < 0) {
 		ret = connector->connector_type_id;
-		drm_mode_object_put(dev, &connector->base);
-		goto out;
+		goto out_out;
 	}
+	connector->name =
+		kasprintf(GFP_KERNEL, "%s-%d",
+			  drm_connector_enum_list[connector_type].name,
+			  connector->connector_type_id);
+	if (!connector->name) {
+		ret = -ENOMEM;
+		goto out_put;
+	}
+
 	INIT_LIST_HEAD(&connector->probed_modes);
 	INIT_LIST_HEAD(&connector->modes);
 	connector->edid_blob_ptr = NULL;

@@ -853,7 +851,11 @@ int drm_connector_init(struct drm_device *dev,
 	drm_object_attach_property(&connector->base,
 				      dev->mode_config.dpms_property, 0);
 
- out:
+out_put:
+	if (ret)
+		drm_mode_object_put(dev, &connector->base);
+
+out_unlock:
 	drm_modeset_unlock_all(dev);
 
 	return ret;

@@ -881,6 +883,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		   connector->connector_type_id);
 
 	drm_mode_object_put(dev, &connector->base);
+	kfree(connector->name);
+	connector->name = NULL;
 	list_del(&connector->head);
 	dev->mode_config.num_connector--;
 }
include/drm/drm_crtc.h +2 -0
index c061bb372199..d4cd7e513280 100644
@@ -444,6 +444,7 @@ struct drm_encoder {
  * @attr: sysfs attributes
  * @head: list management
  * @base: base KMS object
+ * @name: connector name
  * @connector_type: one of the %DRM_MODE_CONNECTOR_<foo> types from drm_mode.h
  * @connector_type_id: index into connector type enum
  * @interlace_allowed: can this connector handle interlaced modes?

@@ -482,6 +483,7 @@ struct drm_connector {
 
 	struct drm_mode_object base;
 
+	char *name;
 	int connector_type;
 	int connector_type_id;
 	bool interlace_allowed;
Signature · 1 line
1.9.1
author
#5 lore raw

RFC PATCH 2/2 drm: store encoder name in encoder struct

In reply to Jani Nikula

This makes drm_get_encoder_name() thread safe.

Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01\
.ptsecurity.ru

Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++--------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 19 insertions(+), 14 deletions(-)
drivers/gpu/drm/drm_crtc.c +17 -14
index 5781130b4126..1c4cb74ede77 100644
@@ -257,21 +257,11 @@ void drm_connector_ida_destroy(void)
 
 /**
  * drm_get_encoder_name - return a string for encoder
- * @encoder: encoder to compute name of
- *
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * @encoder: the encoder to get name for
  */
 const char *drm_get_encoder_name(const struct drm_encoder *encoder)
 {
-	static char buf[32];
-
-	snprintf(buf, 32, "%s-%d",
-		 drm_encoder_enum_list[encoder->encoder_type].name,
-		 encoder->base.id);
-	return buf;
+	return encoder->name;
 }
 EXPORT_SYMBOL(drm_get_encoder_name);
 

@@ -986,16 +976,27 @@ int drm_encoder_init(struct drm_device *dev,
 
 	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
 	if (ret)
-		goto out;
+		goto out_unlock;
 
 	encoder->dev = dev;
 	encoder->encoder_type = encoder_type;
 	encoder->funcs = funcs;
+	encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
+				  drm_encoder_enum_list[encoder_type].name,
+				  encoder->base.id);
+	if (!encoder->name) {
+		ret = -ENOMEM;
+		goto out_put;
+	}
 
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	dev->mode_config.num_encoder++;
 
- out:
+out_put:
+	if (ret)
+		drm_mode_object_put(dev, &encoder->base);
+
+out_unlock:
 	drm_modeset_unlock_all(dev);
 
 	return ret;

@@ -1013,6 +1014,8 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	struct drm_device *dev = encoder->dev;
 	drm_modeset_lock_all(dev);
 	drm_mode_object_put(dev, &encoder->base);
+	kfree(encoder->name);
+	encoder->name = NULL;
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
 	drm_modeset_unlock_all(dev);
include/drm/drm_crtc.h +2 -0
index d4cd7e513280..219b533a2c15 100644
@@ -400,6 +400,7 @@ struct drm_encoder_funcs {
  * @dev: parent DRM device
  * @head: list management
  * @base: base KMS object
+ * @name: encoder name
  * @encoder_type: one of the %DRM_MODE_ENCODER_<foo> types in drm_mode.h
  * @possible_crtcs: bitmask of potential CRTC bindings
  * @possible_clones: bitmask of potential sibling encoders for cloning

@@ -416,6 +417,7 @@ struct drm_encoder {
 	struct list_head head;
 
 	struct drm_mode_object base;
+	char *name;
 	int encoder_type;
 	uint32_t possible_crtcs;
 	uint32_t possible_clones;
Signature · 1 line
1.9.1
#7 lore raw
In reply to Jani Nikula
Quoted 9 lines · Jani Nikula

Hi all -

This series stores connector/encoder names in the relevant structs to make the name getters thread safe.

What say you, is the wasted memory too high a price to pay for the thread safety and implementation simplicity of this approach? I think making drm_get_connector_name and drm_get_encoder_name return allocated buffers makes a lot of code really ugly and error prone.

I thought we could use dev_set_name(connector->kdev, "foo"). -Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#8 lore raw
Quoted 6 lines · Chris Wilson
Quoted 5 lines · Jani Nikula

Hi all -

This series stores connector/encoder names in the relevant structs to make the name getters thread safe.

Quoted 6 lines
Quoted 4 lines

What say you, is the wasted memory too high a price to pay for the thread safety and implementation simplicity of this approach? I think making drm_get_connector_name and drm_get_encoder_name return allocated buffers makes a lot of code really ugly and error prone.

I thought we could use dev_set_name(connector->kdev, "foo").

Hmm, connector->kdev is created at drm_sysfs_connector_add(), and it uses a drm_get_connector_name() prefixed with "cardN-". So there's the naming difference and the lifetime difference, and additionally encoder doesn't have anything similar. Unless I'm missing something, what I'm suggesting is way simpler.

BR,
Jani.

Quoted 4 lines

-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
Reviewed-by on v1 1/2
#2 lore raw

Re: [RFC PATCH 1/2] drm: store connector name in connector struct

In reply to Jani Nikula

Hi

Quoted 4 lines · Jani Nikula

This makes drm_get_connector_name() thread safe.

Reference: http://lkml.kernel.org/r/[email protected]

Signed-off-by Jani Nikula <[email protected]>

I like that approach, but we should also kill drm_get_connector_name() in a follow-up. I don't see why that wrapper is needed.

Reviewed-by David Herrmann <[email protected]>

Thanks
David

Quoted 111 lines
---
 drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++----------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 22 insertions(+), 16 deletions(-)
drivers/gpu/drm/drm_crtc.c +20 -16
index 461d19bd14ee..5781130b4126 100644
@@ -277,21 +277,11 @@ EXPORT_SYMBOL(drm_get_encoder_name);
 
 /**
  * drm_get_connector_name - return a string for connector
- * @connector: connector to compute name of
- *
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * @connector: the connector to get name for
  */
 const char *drm_get_connector_name(const struct drm_connector *connector)
 {
-       static char buf[32];
-
-       snprintf(buf, 32, "%s-%d",
-                drm_connector_enum_list[connector->connector_type].name,
-                connector->connector_type_id);
-       return buf;
+       return connector->name;
 }
 EXPORT_SYMBOL(drm_get_connector_name);
 

@@ -824,7 +814,7 @@ int drm_connector_init(struct drm_device *dev,
 
        ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
        if (ret)
-               goto out;
+               goto out_unlock;
 
        connector->base.properties = &connector->properties;
        connector->dev = dev;

@@ -834,9 +824,17 @@ int drm_connector_init(struct drm_device *dev,
                ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
        if (connector->connector_type_id < 0) {
                ret = connector->connector_type_id;
-               drm_mode_object_put(dev, &connector->base);
-               goto out;
+               goto out_out;
        }
+       connector->name =
+               kasprintf(GFP_KERNEL, "%s-%d",
+                         drm_connector_enum_list[connector_type].name,
+                         connector->connector_type_id);
+       if (!connector->name) {
+               ret = -ENOMEM;
+               goto out_put;
+       }
+
        INIT_LIST_HEAD(&connector->probed_modes);
        INIT_LIST_HEAD(&connector->modes);
        connector->edid_blob_ptr = NULL;

@@ -853,7 +851,11 @@ int drm_connector_init(struct drm_device *dev,
        drm_object_attach_property(&connector->base,
                                      dev->mode_config.dpms_property, 0);
 
- out:
+out_put:
+       if (ret)
+               drm_mode_object_put(dev, &connector->base);
+
+out_unlock:
        drm_modeset_unlock_all(dev);
 
        return ret;

@@ -881,6 +883,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
                   connector->connector_type_id);
 
        drm_mode_object_put(dev, &connector->base);
+       kfree(connector->name);
+       connector->name = NULL;
        list_del(&connector->head);
        dev->mode_config.num_connector--;
 }
include/drm/drm_crtc.h +2 -1
index c061bb372199..d4cd7e513280 100644
@@ -444,6 +444,7 @@ struct drm_encoder {
  * @attr: sysfs attributes
  * @head: list management
  * @base: base KMS object
+ * @name: connector name
  * @connector_type: one of the %DRM_MODE_CONNECTOR_<foo> types from drm_mode.h
  * @connector_type_id: index into connector type enum
  * @interlace_allowed: can this connector handle interlaced modes?

@@ -482,6 +483,7 @@ struct drm_connector {
 
        struct drm_mode_object base;
 
+       char *name;
        int connector_type;
        int connector_type_id;
        bool interlace_allowed;
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Acked-by on the v1 series
#9 lore raw
In reply to Jani Nikula

Hi

CC Dave & Daniel
Quoted 5 lines · Jani Nikula

Hi all -

This series stores connector/encoder names in the relevant structs to make the name getters thread safe.

Quoted 8 lines

What say you, is the wasted memory too high a price to pay for the thread safety and implementation simplicity of this approach? I think making drm_get_connector_name and drm_get_encoder_name return allocated buffers makes a lot of code really ugly and error prone.

I am assuming connector_type, connector_type_id, encoder_type, and encoder.base.id remain unchanged for the lifetime of the connector/encoder - is that a correct assumption?

I like the approach. Using dev-name does not work for encoders, so I'd keep them separate. I mean, we're talking about ~32 bytes per connector here..

Acked-by David Herrmann <[email protected]>

Thanks
David

Reviewed-by on v1 2/2
#6 lore raw

Re: [RFC PATCH 2/2] drm: store encoder name in encoder struct

In reply to Jani Nikula

Hi

Quoted 5 lines · Jani Nikula

This makes drm_get_encoder_name() thread safe.

Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01\
.ptsecurity.ru

Signed-off-by Jani Nikula <[email protected]>

Same as for 1/2, a followup would be nice.

Reviewed-by David Herrmann <[email protected]>

Thanks
David

Quoted 99 lines
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++--------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 19 insertions(+), 14 deletions(-)
drivers/gpu/drm/drm_crtc.c +17 -14
index 5781130b4126..1c4cb74ede77 100644
@@ -257,21 +257,11 @@ void drm_connector_ida_destroy(void)
 
 /**
  * drm_get_encoder_name - return a string for encoder
- * @encoder: encoder to compute name of
- *
- * Note that the buffer used by this function is globally shared and owned by
- * the function itself.
- *
- * FIXME: This isn't really multithreading safe.
+ * @encoder: the encoder to get name for
  */
 const char *drm_get_encoder_name(const struct drm_encoder *encoder)
 {
-       static char buf[32];
-
-       snprintf(buf, 32, "%s-%d",
-                drm_encoder_enum_list[encoder->encoder_type].name,
-                encoder->base.id);
-       return buf;
+       return encoder->name;
 }
 EXPORT_SYMBOL(drm_get_encoder_name);
 

@@ -986,16 +976,27 @@ int drm_encoder_init(struct drm_device *dev,
 
        ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
        if (ret)
-               goto out;
+               goto out_unlock;
 
        encoder->dev = dev;
        encoder->encoder_type = encoder_type;
        encoder->funcs = funcs;
+       encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
+                                 drm_encoder_enum_list[encoder_type].name,
+                                 encoder->base.id);
+       if (!encoder->name) {
+               ret = -ENOMEM;
+               goto out_put;
+       }
 
        list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
        dev->mode_config.num_encoder++;
 
- out:
+out_put:
+       if (ret)
+               drm_mode_object_put(dev, &encoder->base);
+
+out_unlock:
        drm_modeset_unlock_all(dev);
 
        return ret;

@@ -1013,6 +1014,8 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
        struct drm_device *dev = encoder->dev;
        drm_modeset_lock_all(dev);
        drm_mode_object_put(dev, &encoder->base);
+       kfree(encoder->name);
+       encoder->name = NULL;
        list_del(&encoder->head);
        dev->mode_config.num_encoder--;
        drm_modeset_unlock_all(dev);
include/drm/drm_crtc.h +2 -1
index d4cd7e513280..219b533a2c15 100644
@@ -400,6 +400,7 @@ struct drm_encoder_funcs {
  * @dev: parent DRM device
  * @head: list management
  * @base: base KMS object
+ * @name: encoder name
  * @encoder_type: one of the %DRM_MODE_ENCODER_<foo> types in drm_mode.h
  * @possible_crtcs: bitmask of potential CRTC bindings
  * @possible_clones: bitmask of potential sibling encoders for cloning

@@ -416,6 +417,7 @@ struct drm_encoder {
        struct list_head head;
 
        struct drm_mode_object base;
+       char *name;
        int encoder_type;
        uint32_t possible_crtcs;
        uint32_t possible_clones;
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

maintainer
#3 lore raw

Re: [RFC PATCH 1/2] drm: store connector name in connector struct

In reply to Jani Nikula

drivers/gpu/drm/drm_crtc.c int drm_connector_init(struct drm_device *dev, · line 827

Quoted 11 lines

Reference: http://lkml.kernel.org/r/[email protected]

Signed-off-by Jani Nikula <[email protected]>
drivers/gpu/drm/drm_crtc.c +1 -2
@@ -834,9 +824,17 @@ int drm_connector_init(struct drm_device *dev,
                ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
        if (connector->connector_type_id < 0) {
                ret = connector->connector_type_id;
-               drm_mode_object_put(dev, &connector->base);
-               goto out;
+               goto out_out;

^^ one of these things is not like the other,

drivers/gpu/drm/drm_crtc.c int drm_connector_init(struct drm_device *dev, · line 835

Quoted 8 lines
drivers/gpu/drm/drm_crtc.c +7 -0
        }
+       connector->name =
+               kasprintf(GFP_KERNEL, "%s-%d",
+                         drm_connector_enum_list[connector_type].name,
+                         connector->connector_type_id);
+       if (!connector->name) {
+               ret = -ENOMEM;
+               goto out_put;

^^,

I've merged this fixed into my tree, I'm sure Daniel won't mind :)

Dave.

author
#4 lore raw

Re: [RFC PATCH 1/2] drm: store connector name in connector struct

Quoted 24 lines · Dave Airlie

drivers/gpu/drm/drm_crtc.c int drm_connector_init(struct drm_device *dev, · line 827

Quoted 11 lines

Reference: http://lkml.kernel.org/r/[email protected]

Signed-off-by Jani Nikula <[email protected]>
drivers/gpu/drm/drm_crtc.c +1 -2
@@ -834,9 +824,17 @@ int drm_connector_init(struct drm_device *dev,
                ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
        if (connector->connector_type_id < 0) {
                ret = connector->connector_type_id;
-               drm_mode_object_put(dev, &connector->base);
-               goto out;
+               goto out_out;

^^ one of these things is not like the other,

drivers/gpu/drm/drm_crtc.c int drm_connector_init(struct drm_device *dev, · line 835

Quoted 8 lines
drivers/gpu/drm/drm_crtc.c +7 -0
        }
+       connector->name =
+               kasprintf(GFP_KERNEL, "%s-%d",
+                         drm_connector_enum_list[connector_type].name,
+                         connector->connector_type_id);
+       if (!connector->name) {
+               ret = -ENOMEM;
+               goto out_put;
Quoted 3 lines

^^,

I've merged this fixed into my tree, I'm sure Daniel won't mind :)

Thanks. I'll hide behind the "RFC PATCH" subject. ;)

BR,
Jani.

Signature · 1 line
Jani Nikula, Intel Open Source Technology Center