diff mbox

[v2] drm: use ida to allocate connector ids

Message ID 1375170709-4355-1-git-send-email-imirkin@alum.mit.edu (mailing list archive)
State New, archived
Headers show

Commit Message

Ilia Mirkin July 30, 2013, 7:51 a.m. UTC
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(-)

Comments

Dave Airlie Aug. 7, 2013, 12:12 a.m. UTC | #1
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
Rob Clark Aug. 7, 2013, 12:40 a.m. UTC | #2
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
Ilia Mirkin Aug. 7, 2013, 1:04 a.m. UTC | #3
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
Ville Syrjala Aug. 7, 2013, 8 a.m. UTC | #4
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
Ilia Mirkin Aug. 7, 2013, 8:28 a.m. UTC | #5
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
Daniel Vetter Aug. 7, 2013, 8:40 a.m. UTC | #6
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
Ville Syrjala Aug. 7, 2013, 10:47 a.m. UTC | #7
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.
diff mbox

Patch

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,