From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/prime: keep a reference from the handle to exported dma-buf (v5) Date: Wed, 24 Apr 2013 17:24:40 +0200 Message-ID: <20130424152440.GV6169@phenom.ffwll.local> References: <1366333916-31695-1-git-send-email-airlied@gmail.com> <1366350898.9802.9.camel@ideak-mobl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f175.google.com (mail-ea0-f175.google.com [209.85.215.175]) by gabe.freedesktop.org (Postfix) with ESMTP id B3C8CE6081 for ; Wed, 24 Apr 2013 08:21:38 -0700 (PDT) Received: by mail-ea0-f175.google.com with SMTP id f15so794231eak.20 for ; Wed, 24 Apr 2013 08:21:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1366350898.9802.9.camel@ideak-mobl> 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: Imre Deak Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Apr 19, 2013 at 08:54:58AM +0300, Imre Deak wrote: > On Fri, 2013-04-19 at 11:11 +1000, Dave Airlie wrote: > > Currently we have a problem with this: > > 1. i915: create gem object > > 2. i915: export gem object to prime > > 3. radeon: import gem object > > 4. close prime fd > > 5. radeon: unref object > > 6. i915: unref object > > > > i915 has an imported object reference in its file priv, that isn't > > cleaned up properly until fd close. The reference gets added at step 2, > > but at step 6 we don't have enough info to clean it up. > > > > The solution is to take a reference on the dma-buf when we export it, > > and drop the reference when the gem handle goes away. > > > > So when we export a dma_buf from a gem object, we keep track of it > > with the handle, we take a reference to the dma_buf. When we close > > the handle (i.e. userspace is finished with the buffer), we drop > > the reference to the dma_buf, and it gets collected. > > > > This patch isn't meant to fix any other problem or bikesheds, and it doesn't > > fix any races with other scenarios. > > > > v1.1: move export symbol line back up. > > > > v2: okay I had to do a bit more, as the first patch showed a leak > > on one of my tests, that I found using the dma-buf debugfs support, > > the problem case is exporting a buffer twice with the same handle, > > we'd add another export handle for it unnecessarily, however > > we now fail if we try to export the same object with a different gem handle, > > however I'm not sure if that is a case I want to support, and I've > > gotten the code to WARN_ON if we hit something like that. > > > > v2.1: rebase this patch, write better commit msg. > > v3: cleanup error handling, track import vs export in linked list, > > these two patches were separate previously, but seem to work better > > like this. > > v4: danvet is correct, this code is no longer useful, since the buffer > > better exist, so remove it. > > v5: always take a reference to the dma buf object, import or export. > > (Imre Deak contributed this originally) > > > > Signed-off-by: Dave Airlie > > drm_prime_destroy_file_private() lacks a drm_buf_put(). A separate > issue, but the list should actually be empty at that point so perhaps we > should warn if it's not. > > bikeshed: we don't really need the PRIME_{EXPORT,IMPORT} tracking. > > Otherwise looks ok: > Reviewed-by: Imre Deak Yeah, I couldn't find anything else to bitch about than what Imre spotted, and we can easily fix that up in follow-up patches for 3.11. So Reviewed-by: Daniel Vetter -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch