From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2] drm: use ida to allocate connector ids Date: Wed, 7 Aug 2013 11:00:14 +0300 Message-ID: <20130807080014.GO5004@intel.com> References: <1375168611-3384-1-git-send-email-imirkin@alum.mit.edu> <1375170709-4355-1-git-send-email-imirkin@alum.mit.edu> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 3A5A7E6113 for ; Wed, 7 Aug 2013 01:00:18 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1375170709-4355-1-git-send-email-imirkin@alum.mit.edu> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ilia Mirkin Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 > --- > = > v1 -> v2: correct loop condition... not sure how that slipped past > me... the code started out by being <=3D 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_in= fo_enum_list[] =3D { > 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[] =3D > -{ { 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[] =3D > @@ -220,6 +220,22 @@ static const struct drm_prop_enum_list drm_encoder_e= num_list[] =3D > { DRM_MODE_ENCODER_VIRTUAL, "Virtual" }, > }; > = > +void drm_connector_ida_init(void) > +{ > + int i; > + > + for (i =3D 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 =3D 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 =3D &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 =3D dev; > connector->funcs =3D funcs; > connector->connector_type =3D connector_type; > - connector->connector_type_id =3D > - ++drm_connector_enum_list[connector_type].count; /* TODO */ > + ret =3D 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 =3D NULL; > @@ -764,6 +784,9 @@ void drm_connector_cleanup(struct drm_connector *conn= ector) > 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 =3D -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=E4l=E4 Intel OTC