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(-)
This makes drm_get_connector_name() thread safe.
Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecur... 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;
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.ptsecur... 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
Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecur... 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.
On Tue, 27 May 2014, Dave Airlie airlied@gmail.com wrote:
Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecur... 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.
This makes drm_get_encoder_name() thread safe.
Reference: http://lkml.kernel.org/r/645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01%5C .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;
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%5C .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
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
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
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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Dave Airlie -
David Herrmann -
Jani Nikula