This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu ---
Only mild testing... reloaded nouveau a few times, all the connectors kept their original ids.
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..fff238f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name; - int count; + struct ida count; };
/* * Connector and encoder types. */ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 }, - { DRM_MODE_CONNECTOR_VGA, "VGA", 0 }, - { DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 }, - { DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 }, - { DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 }, - { DRM_MODE_CONNECTOR_Composite, "Composite", 0 }, - { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 }, - { DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 }, - { DRM_MODE_CONNECTOR_Component, "Component", 0 }, - { DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 }, - { DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 }, - { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 }, - { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 }, - { DRM_MODE_CONNECTOR_TV, "TV", 0 }, - { DRM_MODE_CONNECTOR_eDP, "eDP", 0 }, - { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0}, +{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" }, + { DRM_MODE_CONNECTOR_VGA, "VGA" }, + { DRM_MODE_CONNECTOR_DVII, "DVI-I" }, + { DRM_MODE_CONNECTOR_DVID, "DVI-D" }, + { DRM_MODE_CONNECTOR_DVIA, "DVI-A" }, + { DRM_MODE_CONNECTOR_Composite, "Composite" }, + { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" }, + { DRM_MODE_CONNECTOR_LVDS, "LVDS" }, + { DRM_MODE_CONNECTOR_Component, "Component" }, + { DRM_MODE_CONNECTOR_9PinDIN, "DIN" }, + { DRM_MODE_CONNECTOR_DisplayPort, "DP" }, + { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" }, + { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" }, + { DRM_MODE_CONNECTOR_TV, "TV" }, + { DRM_MODE_CONNECTOR_eDP, "eDP" }, + { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, };
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{ + int i; + + for (i = 0; i <= ARRAY_SIZE(drm_connector_enum_list); i++) + ida_init(&drm_connector_enum_list[i].count); +} + +void drm_connector_ida_destroy(void) +{ + int i; + + for (i = 0; i <= ARRAY_SIZE(drm_connector_enum_list); i++) + ida_destroy(&drm_connector_enum_list[i].count); +} + const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret; + struct ida *count = &drm_connector_enum_list[connector_type].count; + + ida_pre_get(count, GFP_KERNEL);
drm_modeset_lock_all(dev);
@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type; - connector->connector_type_id = - ++drm_connector_enum_list[connector_type].count; /* TODO */ + ret = ida_get_new_above(count, 1, &connector->connector_type_id); + if (ret) + goto out; INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL; @@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
+ ida_remove(&drm_connector_enum_list[connector->connector_type].count, + connector->connector_type_id); + drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init(); + drm_connector_ida_init(); idr_init(&drm_minors_idr);
if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) @@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
+ drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr); }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs,
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu ---
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name; - int count; + struct ida count; };
/* * Connector and encoder types. */ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 }, - { DRM_MODE_CONNECTOR_VGA, "VGA", 0 }, - { DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 }, - { DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 }, - { DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 }, - { DRM_MODE_CONNECTOR_Composite, "Composite", 0 }, - { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 }, - { DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 }, - { DRM_MODE_CONNECTOR_Component, "Component", 0 }, - { DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 }, - { DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 }, - { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 }, - { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 }, - { DRM_MODE_CONNECTOR_TV, "TV", 0 }, - { DRM_MODE_CONNECTOR_eDP, "eDP", 0 }, - { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0}, +{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" }, + { DRM_MODE_CONNECTOR_VGA, "VGA" }, + { DRM_MODE_CONNECTOR_DVII, "DVI-I" }, + { DRM_MODE_CONNECTOR_DVID, "DVI-D" }, + { DRM_MODE_CONNECTOR_DVIA, "DVI-A" }, + { DRM_MODE_CONNECTOR_Composite, "Composite" }, + { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" }, + { DRM_MODE_CONNECTOR_LVDS, "LVDS" }, + { DRM_MODE_CONNECTOR_Component, "Component" }, + { DRM_MODE_CONNECTOR_9PinDIN, "DIN" }, + { DRM_MODE_CONNECTOR_DisplayPort, "DP" }, + { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" }, + { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" }, + { DRM_MODE_CONNECTOR_TV, "TV" }, + { DRM_MODE_CONNECTOR_eDP, "eDP" }, + { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, };
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++) + ida_init(&drm_connector_enum_list[i].count); +} + +void drm_connector_ida_destroy(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++) + ida_destroy(&drm_connector_enum_list[i].count); +} + const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret; + struct ida *count = &drm_connector_enum_list[connector_type].count; + + ida_pre_get(count, GFP_KERNEL);
drm_modeset_lock_all(dev);
@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type; - connector->connector_type_id = - ++drm_connector_enum_list[connector_type].count; /* TODO */ + ret = ida_get_new_above(count, 1, &connector->connector_type_id); + if (ret) + goto out; INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL; @@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
+ ida_remove(&drm_connector_enum_list[connector->connector_type].count, + connector->connector_type_id); + drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init(); + drm_connector_ida_init(); idr_init(&drm_minors_idr);
if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) @@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
+ drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr); }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs,
On Tue, Jul 30, 2013 at 5:51 PM, Ilia Mirkin imirkin@alum.mit.edu wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
I like this, anyone else want to comment/review?
Dave.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
int count;
struct ida count;};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
{ DRM_MODE_CONNECTOR_VGA, "VGA", 0 },{ DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },{ DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },{ DRM_MODE_CONNECTOR_Composite, "Composite", 0 },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },{ DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },{ DRM_MODE_CONNECTOR_Component, "Component", 0 },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },{ DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },{ DRM_MODE_CONNECTOR_TV, "TV", 0 },{ DRM_MODE_CONNECTOR_eDP, "eDP", 0 },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
{ DRM_MODE_CONNECTOR_VGA, "VGA" },{ DRM_MODE_CONNECTOR_DVII, "DVI-I" },{ DRM_MODE_CONNECTOR_DVID, "DVI-D" },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A" },{ DRM_MODE_CONNECTOR_Composite, "Composite" },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },{ DRM_MODE_CONNECTOR_LVDS, "LVDS" },{ DRM_MODE_CONNECTOR_Component, "Component" },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN" },{ DRM_MODE_CONNECTOR_DisplayPort, "DP" },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },{ DRM_MODE_CONNECTOR_TV, "TV" },{ DRM_MODE_CONNECTOR_eDP, "eDP" },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_init(&drm_connector_enum_list[i].count);+}
+void drm_connector_ida_destroy(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_destroy(&drm_connector_enum_list[i].count);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *count = &drm_connector_enum_list[connector_type].count;ida_pre_get(count, GFP_KERNEL); drm_modeset_lock_all(dev);@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
connector->connector_type_id =++drm_connector_enum_list[connector_type].count; /* TODO */
ret = ida_get_new_above(count, 1, &connector->connector_type_id);if (ret)goto out; INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL;@@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
ida_remove(&drm_connector_enum_list[connector->connector_type].count,connector->connector_type_id);drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr); if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 6, 2013 at 8:12 PM, Dave Airlie airlied@gmail.com wrote:
On Tue, Jul 30, 2013 at 5:51 PM, Ilia Mirkin imirkin@alum.mit.edu wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
I like this, anyone else want to comment/review?
looks nice.. not sure I actually want to *encourage* someone to rely on stable connector id's (since it can change from kernel version to kernel version, etc), but otoh it would make life nicer for modetest users (since it isn't clever enough to use connector-names)
BR, -R
Dave.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
int count;
struct ida count;};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
{ DRM_MODE_CONNECTOR_VGA, "VGA", 0 },{ DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },{ DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },{ DRM_MODE_CONNECTOR_Composite, "Composite", 0 },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },{ DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },{ DRM_MODE_CONNECTOR_Component, "Component", 0 },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },{ DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },{ DRM_MODE_CONNECTOR_TV, "TV", 0 },{ DRM_MODE_CONNECTOR_eDP, "eDP", 0 },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
{ DRM_MODE_CONNECTOR_VGA, "VGA" },{ DRM_MODE_CONNECTOR_DVII, "DVI-I" },{ DRM_MODE_CONNECTOR_DVID, "DVI-D" },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A" },{ DRM_MODE_CONNECTOR_Composite, "Composite" },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },{ DRM_MODE_CONNECTOR_LVDS, "LVDS" },{ DRM_MODE_CONNECTOR_Component, "Component" },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN" },{ DRM_MODE_CONNECTOR_DisplayPort, "DP" },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },{ DRM_MODE_CONNECTOR_TV, "TV" },{ DRM_MODE_CONNECTOR_eDP, "eDP" },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_init(&drm_connector_enum_list[i].count);+}
+void drm_connector_ida_destroy(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_destroy(&drm_connector_enum_list[i].count);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *count = &drm_connector_enum_list[connector_type].count;ida_pre_get(count, GFP_KERNEL); drm_modeset_lock_all(dev);@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
connector->connector_type_id =++drm_connector_enum_list[connector_type].count; /* TODO */
ret = ida_get_new_above(count, 1, &connector->connector_type_id);if (ret)goto out; INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL;@@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
ida_remove(&drm_connector_enum_list[connector->connector_type].count,connector->connector_type_id);drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr); if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 6, 2013 at 8:40 PM, Rob Clark robdclark@gmail.com wrote:
On Tue, Aug 6, 2013 at 8:12 PM, Dave Airlie airlied@gmail.com wrote:
On Tue, Jul 30, 2013 at 5:51 PM, Ilia Mirkin imirkin@alum.mit.edu wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
I like this, anyone else want to comment/review?
looks nice.. not sure I actually want to *encourage* someone to rely on stable connector id's (since it can change from kernel version to kernel version, etc), but otoh it would make life nicer for modetest users (since it isn't clever enough to use connector-names)
My personal use-case is running xrandr --output DVI-I-1 --brightness .8 every time. Really nice if I can just ^Rxr and be done with it. [Would be even nicer if I could stick it in the X config, but I digress.] Also e.g. for Zaphod heads, you specify the connector names in the X config (although I don't personally use that).
-ilia
BR, -R
Dave.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
int count;
struct ida count;};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
{ DRM_MODE_CONNECTOR_VGA, "VGA", 0 },{ DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },{ DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },{ DRM_MODE_CONNECTOR_Composite, "Composite", 0 },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },{ DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },{ DRM_MODE_CONNECTOR_Component, "Component", 0 },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },{ DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },{ DRM_MODE_CONNECTOR_TV, "TV", 0 },{ DRM_MODE_CONNECTOR_eDP, "eDP", 0 },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
{ DRM_MODE_CONNECTOR_VGA, "VGA" },{ DRM_MODE_CONNECTOR_DVII, "DVI-I" },{ DRM_MODE_CONNECTOR_DVID, "DVI-D" },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A" },{ DRM_MODE_CONNECTOR_Composite, "Composite" },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },{ DRM_MODE_CONNECTOR_LVDS, "LVDS" },{ DRM_MODE_CONNECTOR_Component, "Component" },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN" },{ DRM_MODE_CONNECTOR_DisplayPort, "DP" },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },{ DRM_MODE_CONNECTOR_TV, "TV" },{ DRM_MODE_CONNECTOR_eDP, "eDP" },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_init(&drm_connector_enum_list[i].count);+}
+void drm_connector_ida_destroy(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_destroy(&drm_connector_enum_list[i].count);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *count = &drm_connector_enum_list[connector_type].count;ida_pre_get(count, GFP_KERNEL); drm_modeset_lock_all(dev);@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
connector->connector_type_id =++drm_connector_enum_list[connector_type].count; /* TODO */
ret = ida_get_new_above(count, 1, &connector->connector_type_id);if (ret)goto out; INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL;@@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
ida_remove(&drm_connector_enum_list[connector->connector_type].count,connector->connector_type_id);drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr); if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jul 30, 2013 at 03:51:49AM -0400, Ilia Mirkin wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
- int count;
- struct ida count;
I'd rename this to something else. 'count' is just confusing.
};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
- { DRM_MODE_CONNECTOR_VGA, "VGA", 0 },
- { DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },
- { DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },
- { DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },
- { DRM_MODE_CONNECTOR_Composite, "Composite", 0 },
- { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },
- { DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },
- { DRM_MODE_CONNECTOR_Component, "Component", 0 },
- { DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },
- { DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },
- { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },
- { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },
- { DRM_MODE_CONNECTOR_TV, "TV", 0 },
- { DRM_MODE_CONNECTOR_eDP, "eDP", 0 },
- { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},
+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
- { DRM_MODE_CONNECTOR_VGA, "VGA" },
- { DRM_MODE_CONNECTOR_DVII, "DVI-I" },
- { DRM_MODE_CONNECTOR_DVID, "DVI-D" },
- { DRM_MODE_CONNECTOR_DVIA, "DVI-A" },
- { DRM_MODE_CONNECTOR_Composite, "Composite" },
- { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },
- { DRM_MODE_CONNECTOR_LVDS, "LVDS" },
- { DRM_MODE_CONNECTOR_Component, "Component" },
- { DRM_MODE_CONNECTOR_9PinDIN, "DIN" },
- { DRM_MODE_CONNECTOR_DisplayPort, "DP" },
- { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },
- { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },
- { DRM_MODE_CONNECTOR_TV, "TV" },
- { DRM_MODE_CONNECTOR_eDP, "eDP" },
- { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)
ida_init(&drm_connector_enum_list[i].count);+}
+void drm_connector_ida_destroy(void) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)
ida_destroy(&drm_connector_enum_list[i].count);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *count = &drm_connector_enum_list[connector_type].count;
ida_pre_get(count, GFP_KERNEL);
drm_modeset_lock_all(dev);
@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
- connector->connector_type_id =
++drm_connector_enum_list[connector_type].count; /* TODO */
- ret = ida_get_new_above(count, 1, &connector->connector_type_id);
- if (ret)
goto out;
Leak.
Also there's no retry loop w/ ida_pre_get() and since that's outside the big kms lock, there could be a (small) chance that someone else already consumed the allocation done in ida_pre_get(). And then we'll return -EAGAIN which is a rather confusing error from an init function. I guess you could just move the ida_pre_get() inside the kms lock and avoid that race.
INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL; @@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
- ida_remove(&drm_connector_enum_list[connector->connector_type].count,
connector->connector_type_id);- drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr);
if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))
@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
- drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 7, 2013 at 4:00 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jul 30, 2013 at 03:51:49AM -0400, Ilia Mirkin wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v1 -> v2: correct loop condition... not sure how that slipped past me... the code started out by being <= DRM...VIRTUAL, I guess
drivers/gpu/drm/drm_crtc.c | 61 +++++++++++++++++++++++++++++++--------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..ed7599a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
int count;
struct ida count;I'd rename this to something else. 'count' is just confusing.
};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
{ DRM_MODE_CONNECTOR_VGA, "VGA", 0 },{ DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },{ DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },{ DRM_MODE_CONNECTOR_Composite, "Composite", 0 },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },{ DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },{ DRM_MODE_CONNECTOR_Component, "Component", 0 },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },{ DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },{ DRM_MODE_CONNECTOR_TV, "TV", 0 },{ DRM_MODE_CONNECTOR_eDP, "eDP", 0 },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
{ DRM_MODE_CONNECTOR_VGA, "VGA" },{ DRM_MODE_CONNECTOR_DVII, "DVI-I" },{ DRM_MODE_CONNECTOR_DVID, "DVI-D" },{ DRM_MODE_CONNECTOR_DVIA, "DVI-A" },{ DRM_MODE_CONNECTOR_Composite, "Composite" },{ DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },{ DRM_MODE_CONNECTOR_LVDS, "LVDS" },{ DRM_MODE_CONNECTOR_Component, "Component" },{ DRM_MODE_CONNECTOR_9PinDIN, "DIN" },{ DRM_MODE_CONNECTOR_DisplayPort, "DP" },{ DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },{ DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },{ DRM_MODE_CONNECTOR_TV, "TV" },{ DRM_MODE_CONNECTOR_eDP, "eDP" },{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_init(&drm_connector_enum_list[i].count);+}
+void drm_connector_ida_destroy(void) +{
int i;for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)ida_destroy(&drm_connector_enum_list[i].count);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,9 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *count = &drm_connector_enum_list[connector_type].count;ida_pre_get(count, GFP_KERNEL); drm_modeset_lock_all(dev);@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
connector->connector_type_id =++drm_connector_enum_list[connector_type].count; /* TODO */
ret = ida_get_new_above(count, 1, &connector->connector_type_id);if (ret)goto out;Leak.
I assume this is in reference to the drm_mode_object_get above? I just noticed that... or was there something else too?
Also there's no retry loop w/ ida_pre_get() and since that's outside the big kms lock, there could be a (small) chance that someone else already consumed the allocation done in ida_pre_get(). And then we'll return -EAGAIN which is a rather confusing error from an init function. I guess you could just move the ida_pre_get() inside the kms lock and avoid that race.
OK, I'm not really sure what the whole drm locking situation is. If it's OK to do a GFP_KERNEL alloc inside the "big kms lock" (BKL v2?), I might as well use the ida_simple_get interface.
INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL;@@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
ida_remove(&drm_connector_enum_list[connector->connector_type].count,connector->connector_type_id);drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr); if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
On Wed, Aug 7, 2013 at 10:28 AM, Ilia Mirkin imirkin@alum.mit.edu wrote:
Also there's no retry loop w/ ida_pre_get() and since that's outside the big kms lock, there could be a (small) chance that someone else already consumed the allocation done in ida_pre_get(). And then we'll return -EAGAIN which is a rather confusing error from an init function. I guess you could just move the ida_pre_get() inside the kms lock and avoid that race.
OK, I'm not really sure what the whole drm locking situation is. If it's OK to do a GFP_KERNEL alloc inside the "big kms lock" (BKL v2?), I might as well use the ida_simple_get interface.
The big kms lock is just a pile of nested mutexes which all may not be held from shrinker callbacks and similar places. So you can allocate GFP_KERNEL memory with impunity.
Cheers, Daniel
On Wed, Aug 07, 2013 at 04:28:35AM -0400, Ilia Mirkin wrote:
On Wed, Aug 7, 2013 at 4:00 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jul 30, 2013 at 03:51:49AM -0400, Ilia Mirkin wrote:
@@ -722,8 +741,9 @@ int drm_connector_init(struct drm_device *dev, connector->dev = dev; connector->funcs = funcs; connector->connector_type = connector_type;
connector->connector_type_id =++drm_connector_enum_list[connector_type].count; /* TODO */
ret = ida_get_new_above(count, 1, &connector->connector_type_id);if (ret)goto out;Leak.
I assume this is in reference to the drm_mode_object_get above? I just noticed that... or was there something else too?
Just the obj id.
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu ---
v2 -> v3: - rename ida struct name to... ida. (i'm ready to accept my "creative name of the year" award.) - fix object id leak when allocation fails. - use ida_simple_get, since it's OK to allocate memory inside kms lock
drivers/gpu/drm/drm_crtc.c | 62 ++++++++++++++++++++++++++++++++-------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..a691764 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name; - int count; + struct ida ida; };
/* * Connector and encoder types. */ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 }, - { DRM_MODE_CONNECTOR_VGA, "VGA", 0 }, - { DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 }, - { DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 }, - { DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 }, - { DRM_MODE_CONNECTOR_Composite, "Composite", 0 }, - { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 }, - { DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 }, - { DRM_MODE_CONNECTOR_Component, "Component", 0 }, - { DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 }, - { DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 }, - { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 }, - { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 }, - { DRM_MODE_CONNECTOR_TV, "TV", 0 }, - { DRM_MODE_CONNECTOR_eDP, "eDP", 0 }, - { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0}, +{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" }, + { DRM_MODE_CONNECTOR_VGA, "VGA" }, + { DRM_MODE_CONNECTOR_DVII, "DVI-I" }, + { DRM_MODE_CONNECTOR_DVID, "DVI-D" }, + { DRM_MODE_CONNECTOR_DVIA, "DVI-A" }, + { DRM_MODE_CONNECTOR_Composite, "Composite" }, + { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" }, + { DRM_MODE_CONNECTOR_LVDS, "LVDS" }, + { DRM_MODE_CONNECTOR_Component, "Component" }, + { DRM_MODE_CONNECTOR_9PinDIN, "DIN" }, + { DRM_MODE_CONNECTOR_DisplayPort, "DP" }, + { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" }, + { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" }, + { DRM_MODE_CONNECTOR_TV, "TV" }, + { DRM_MODE_CONNECTOR_eDP, "eDP" }, + { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, };
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++) + ida_init(&drm_connector_enum_list[i].ida); +} + +void drm_connector_ida_destroy(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++) + ida_destroy(&drm_connector_enum_list[i].ida); +} + const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,8 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret; + struct ida *connector_ida = + &drm_connector_enum_list[connector_type].ida;
drm_modeset_lock_all(dev);
@@ -723,7 +741,12 @@ int drm_connector_init(struct drm_device *dev, connector->funcs = funcs; connector->connector_type = connector_type; connector->connector_type_id = - ++drm_connector_enum_list[connector_type].count; /* TODO */ + 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; + } INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL; @@ -764,6 +787,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
+ ida_remove(&drm_connector_enum_list[connector->connector_type].ida, + connector->connector_type_id); + drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init(); + drm_connector_ida_init(); idr_init(&drm_minors_idr);
if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) @@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
+ drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr); }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs,
On Wed, Aug 07, 2013 at 10:34:48PM -0400, Ilia Mirkin wrote:
This makes it so that reloading a module does not cause all the connector ids to change, which are user-visible and sometimes used for configuration.
Signed-off-by: Ilia Mirkin imirkin@alum.mit.edu
v2 -> v3:
- rename ida struct name to... ida. (i'm ready to accept my "creative name of the year" award.)
Seems fine to me. We already have the foo_idr and whatever.
- fix object id leak when allocation fails.
- use ida_simple_get, since it's OK to allocate memory inside kms lock
Looks fine to me.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_crtc.c | 62 ++++++++++++++++++++++++++++++++-------------- drivers/gpu/drm/drm_drv.c | 2 ++ include/drm/drm_crtc.h | 2 ++ 3 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..a691764 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -186,29 +186,29 @@ static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = { struct drm_conn_prop_enum_list { int type; const char *name;
- int count;
- struct ida ida;
};
/*
- Connector and encoder types.
*/ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = -{ { DRM_MODE_CONNECTOR_Unknown, "Unknown", 0 },
- { DRM_MODE_CONNECTOR_VGA, "VGA", 0 },
- { DRM_MODE_CONNECTOR_DVII, "DVI-I", 0 },
- { DRM_MODE_CONNECTOR_DVID, "DVI-D", 0 },
- { DRM_MODE_CONNECTOR_DVIA, "DVI-A", 0 },
- { DRM_MODE_CONNECTOR_Composite, "Composite", 0 },
- { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO", 0 },
- { DRM_MODE_CONNECTOR_LVDS, "LVDS", 0 },
- { DRM_MODE_CONNECTOR_Component, "Component", 0 },
- { DRM_MODE_CONNECTOR_9PinDIN, "DIN", 0 },
- { DRM_MODE_CONNECTOR_DisplayPort, "DP", 0 },
- { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A", 0 },
- { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B", 0 },
- { DRM_MODE_CONNECTOR_TV, "TV", 0 },
- { DRM_MODE_CONNECTOR_eDP, "eDP", 0 },
- { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual", 0},
+{ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
- { DRM_MODE_CONNECTOR_VGA, "VGA" },
- { DRM_MODE_CONNECTOR_DVII, "DVI-I" },
- { DRM_MODE_CONNECTOR_DVID, "DVI-D" },
- { DRM_MODE_CONNECTOR_DVIA, "DVI-A" },
- { DRM_MODE_CONNECTOR_Composite, "Composite" },
- { DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO" },
- { DRM_MODE_CONNECTOR_LVDS, "LVDS" },
- { DRM_MODE_CONNECTOR_Component, "Component" },
- { DRM_MODE_CONNECTOR_9PinDIN, "DIN" },
- { DRM_MODE_CONNECTOR_DisplayPort, "DP" },
- { DRM_MODE_CONNECTOR_HDMIA, "HDMI-A" },
- { DRM_MODE_CONNECTOR_HDMIB, "HDMI-B" },
- { DRM_MODE_CONNECTOR_TV, "TV" },
- { DRM_MODE_CONNECTOR_eDP, "eDP" },
- { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
};
static const struct drm_prop_enum_list drm_encoder_enum_list[] = @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, };
+void drm_connector_ida_init(void) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)
ida_init(&drm_connector_enum_list[i].ida);+}
+void drm_connector_ida_destroy(void) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(drm_connector_enum_list); i++)
ida_destroy(&drm_connector_enum_list[i].ida);+}
const char *drm_get_encoder_name(const struct drm_encoder *encoder) { static char buf[32]; @@ -711,6 +727,8 @@ int drm_connector_init(struct drm_device *dev, int connector_type) { int ret;
struct ida *connector_ida =
&drm_connector_enum_list[connector_type].ida;drm_modeset_lock_all(dev);
@@ -723,7 +741,12 @@ int drm_connector_init(struct drm_device *dev, connector->funcs = funcs; connector->connector_type = connector_type; connector->connector_type_id =
++drm_connector_enum_list[connector_type].count; /* TODO */
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;- } INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); connector->edid_blob_ptr = NULL;
@@ -764,6 +787,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->modes, head) drm_mode_remove(connector, mode);
- ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
connector->connector_type_id);- drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 99fcd7c..00597a1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -251,6 +251,7 @@ static int __init drm_core_init(void) int ret = -ENOMEM;
drm_global_init();
drm_connector_ida_init(); idr_init(&drm_minors_idr);
if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))
@@ -298,6 +299,7 @@ static void __exit drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
- drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr);
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..effee9d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,6 +869,8 @@ extern int drm_crtc_init(struct drm_device *dev, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern void drm_connector_ida_init(void); +extern void drm_connector_ida_destroy(void); extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, -- 1.8.1.5
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
Ilia Mirkin -
Rob Clark -
Ville Syrjälä