[RFC PATCH 0/2] drm: make drm_get_{connector, encoder}_name thread safe

[RFC PATCH 0/2] drm: make drm_get_{connector, encoder}_name thread safe

[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 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(-)

-- 
1.9.1

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

[Jani Nikula]

This makes drm_get_connector_name() thread safe.

Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecurity.ru
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++----------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 461d19bd14ee..5781130b4126 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -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--;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c061bb372199..d4cd7e513280 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -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

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++--------------
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5781130b4126..1c4cb74ede77 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -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);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d4cd7e513280..219b533a2c15 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -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

Re: [RFC PATCH 0/2] drm: make drm_get_{connector, encoder}_name thread safe

[Chris Wilson]

On Wed, May 14, 2014 at 04:58:18PM +0300, Jani Nikula wrote:
> 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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC PATCH 0/2] drm: make drm_get_{connector, encoder}_name thread safe

[Jani Nikula]

On Wed, 14 May 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, May 14, 2014 at 04:58:18PM +0300, Jani Nikula wrote:
>> 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").

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.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center

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

[David Herrmann]

Hi

On Wed, May 14, 2014 at 3:58 PM, Jani Nikula <jani.nikula@intel.com> wrote:
> This makes drm_get_connector_name() thread safe.
>
> Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecurity.ru
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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 <dh.herrmann@gmail.com>

Thanks
David

> ---
>  drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++----------------
>  include/drm/drm_crtc.h     |  2 ++
>  2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 461d19bd14ee..5781130b4126 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -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--;
>  }
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c061bb372199..d4cd7e513280 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/2] drm: make drm_get_{connector, encoder}_name thread safe

[David Herrmann]

Hi

CC: Dave & Daniel

On Wed, May 14, 2014 at 3:58 PM, Jani Nikula <jani.nikula@intel.com> wrote:
> 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?

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 <dh.herrmann@gmail.com>

Thanks
David

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

[David Herrmann]

Hi

On Wed, May 14, 2014 at 3:58 PM, Jani Nikula <jani.nikula@intel.com> wrote:
> 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 <jani.nikula@intel.com>

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

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> ---
>  drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++--------------
>  include/drm/drm_crtc.h     |  2 ++
>  2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5781130b4126..1c4cb74ede77 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -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);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index d4cd7e513280..219b533a2c15 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dave Airlie]

> Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecurity.ru
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>


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


>         }
> +       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.

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

[Jani Nikula]

On Tue, 27 May 2014, Dave Airlie <airlied@gmail.com> wrote:
>> Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecurity.ru
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
>
>> @@ -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,
>
>
>>         }
>> +       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 :)

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

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center