From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/2] drm: don't recycle used modeset IDs Date: Fri, 29 Aug 2014 14:51:25 +0200 Message-ID: <20140829125125.GG15520@phenom.ffwll.local> References: <1409313661-20696-1-git-send-email-dh.herrmann@gmail.com> <1409313661-20696-2-git-send-email-dh.herrmann@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f181.google.com (mail-we0-f181.google.com [74.125.82.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 507C56E06F for ; Fri, 29 Aug 2014 05:51:05 -0700 (PDT) Received: by mail-we0-f181.google.com with SMTP id x48so2174836wes.12 for ; Fri, 29 Aug 2014 05:51:04 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1409313661-20696-2-git-send-email-dh.herrmann@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: David Herrmann Cc: Daniel Vetter , stable@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: > With MST, we now have connector hotplugging, yey! Pretty easy to use in > user-space, but introduces some nasty races: > * If a connector is removed and added again while a compositor is in > background, it will get the same ID again. If the compositor wakes up, > it cannot know whether its the same connector or a new one, thus they > must re-read EDID information, etc. > * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if > an object is removed and a new one is added, those bitmasks are invalid > and must be refreshed. This currently does not affect connectors, but > only crtcs and encoders, but it's only a matter of time when this will > change. > > The easiest way to protect against this, is to not recylce modeset IDs. > Instead, always allocate a new, higher ID. All ioctls that affect modeset > objects, take IDs. Thus, during hotplug races, those ioctls will simply > fail if invalid IDs are passed. They will no longer silently run on a > newly hotplugged object. > > Furthermore, hotplug-races during state sync can now be easily detected. A > call to GET_RESOURCES returns a list of available IDs atomically. > User-space can now start fetching all those objects via GET_* ioctls. If > any of those fails, they know that the given object was unplugged. Thus, > the "possible_*" bit-fields are invalidated. User-space can now decide > whether to restart the sync all over or wait for the 'change' uevent that > is sent on modeset object modifications (or, well, is supposed to be sent > and probably will be at some point). > > With this in place, modeset object hotplugging should work fine for all > modeset objects in the KMS API. > > CC'ing stable so we can rely on all kernels with MST to not recycle IDs. > > Cc: # 3.16+ > Signed-off-by: David Herrmann So userspace just needs to cycle through piles of framebuffer objects to make bad things happen? Doesn't sound like a terribly solid plan. I guess we could save this by doing normal id allocations for fbs and monotonically increasing allocations for all other objects. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 97eba56..ab0fe6d 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -286,7 +286,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev, > idr_preload(GFP_KERNEL); > spin_lock_irq(&dev->mode_config.idr_lock); > > - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); > + ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); > if (ret >= 0) { > /* > * Set up the object linking under the protection of the idr > -- > 2.1.0 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch