From patchwork Wed Jan 16 23:36:27 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aaron Plattner X-Patchwork-Id: 1993761 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork1.kernel.org (Postfix) with ESMTP id 474A03FC85 for ; Wed, 16 Jan 2013 23:37:30 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 12917E6B74 for ; Wed, 16 Jan 2013 15:37:30 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from hqemgate04.nvidia.com (hqemgate04.nvidia.com [216.228.121.35]) by gabe.freedesktop.org (Postfix) with ESMTP id 5B739E5D0B for ; Wed, 16 Jan 2013 15:36:33 -0800 (PST) Received: from hqnvupgp06.nvidia.com (Not Verified[216.228.121.13]) by hqemgate04.nvidia.com id ; Wed, 16 Jan 2013 15:36:13 -0800 Received: from hqemhub03.nvidia.com ([172.17.108.22]) by hqnvupgp06.nvidia.com (PGP Universal service); Wed, 16 Jan 2013 15:35:17 -0800 X-PGP-Universal: processed; by hqnvupgp06.nvidia.com on Wed, 16 Jan 2013 15:35:17 -0800 Received: from tenor.nvidia.com (172.20.144.16) by hqemhub03.nvidia.com (172.20.150.15) with Microsoft SMTP Server id 8.3.279.1; Wed, 16 Jan 2013 15:36:27 -0800 Message-ID: <50F7397B.8090003@nvidia.com> Date: Wed, 16 Jan 2013 15:36:27 -0800 From: Aaron Plattner User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130109 Thunderbird/17.0.2 MIME-Version: 1.0 To: Daniel Vetter Subject: Re: [PATCH v3 1/3] drm: add prime helpers References: <1358282864-2888-1-git-send-email-aplattner@nvidia.com> <1358282864-2888-2-git-send-email-aplattner@nvidia.com> <20130116095014.GB8347@phenom.ffwll.local> In-Reply-To: <20130116095014.GB8347@phenom.ffwll.local> X-NVConfidentiality: public Cc: Daniel Vetter , "dri-devel@lists.freedesktop.org" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org On 01/16/2013 01:50 AM, Daniel Vetter wrote: > On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote: >> Instead of reimplementing all of the dma_buf functionality in every driver, >> create helpers drm_prime_import and drm_prime_export that implement them in >> terms of new, lower-level hook functions: >> >> gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT >> gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export >> gem_prime_import_sg_table: convert an sg_table into a drm_gem_object >> gem_prime_vmap, gem_prime_vunmap: map and unmap an object >> >> These hooks are optional; drivers can opt in by using drm_gem_prime_import and >> drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of >> struct drm_driver. >> >> v2: >> - Drop .begin_cpu_access. None of the drivers this code replaces implemented >> it. Having it here was a leftover from when I was trying to include i915 in >> this rework. >> - Use mutex_lock instead of mutex_lock_interruptible, as these three drivers >> did. This patch series shouldn't change that behavior. >> - Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. >> Rename struct sg_table* variables to 'sgt' for clarity. >> - Update drm.tmpl for these new hooks. >> >> v3: >> - Pass the vaddr down to the driver. This lets drivers that just call vunmap on >> the pointer avoid having to store the pointer in their GEM private structures. >> - Move documentation into a /** DOC */ comment in drm_prime.c and include it in >> drm.tmpl with a !P line. I tried to use !F lines to include documentation of >> the individual functions from drmP.h, but the docproc / kernel-doc scripts >> barf on that file, so hopefully this is good enough for now. >> - apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec >> ("drm/prime: drop reference on imported dma-buf come from gem") >> >> Signed-off-by: Aaron Plattner >> Cc: Daniel Vetter >> Cc: David Airlie > > Two minor things, but since you're not touching drm/i915 I don't care that > much ... > >> +static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, >> + enum dma_data_direction dir) >> +{ >> + struct drm_gem_object *obj = attach->dmabuf->priv; >> + struct sg_table *sgt; >> + >> + mutex_lock(&obj->dev->struct_mutex); >> + >> + sgt = obj->dev->driver->gem_prime_get_sg_table(obj); >> + >> + if (!IS_ERR_OR_NULL(sgt)) >> + dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); >> + >> + mutex_unlock(&obj->dev->struct_mutex); > > The locking here looks superflous and not required to protect anything > drm_gem_map_dma_buf does. If drivers need this (and ttm based ones > shouldn't really), they can grab it in their ->gem_prime_get_sg_table > callback. So I think a follow-up patch to ditch this would be good. If you look at the later patches, this was just moved from the drivers. I agree that evaluating whether or not it's actually necessary should be a separate follow-up. >> + return sgt; >> +} >> + >> +static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, >> + struct sg_table *sgt, enum dma_data_direction dir) >> +{ >> + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir); >> + sg_free_table(sgt); >> + kfree(sgt); >> +} >> + >> +static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) >> +{ >> + struct drm_gem_object *obj = dma_buf->priv; >> + >> + if (obj->export_dma_buf == dma_buf) { >> + /* drop the reference on the export fd holds */ >> + obj->export_dma_buf = NULL; >> + drm_gem_object_unreference_unlocked(obj); >> + } >> +} >> + >> +static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) >> +{ >> + struct drm_gem_object *obj = dma_buf->priv; >> + struct drm_device *dev = obj->dev; >> + >> + return dev->driver->gem_prime_vmap(obj); >> +} >> + >> +static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) >> +{ >> + struct drm_gem_object *obj = dma_buf->priv; >> + struct drm_device *dev = obj->dev; >> + >> + dev->driver->gem_prime_vunmap(obj, vaddr); >> +} > > Imo these two don't add value, simpler to add a typechecking > drm_dmabuf_to_gem_obj static inline somewhere. But then you'd need to pass > in the dmabuf fops struct from drivers and export the helpers. More > flexible, but also a bit work to redo. Like I've said, I don't care too > much ... I considered that, but it seems like the wrong abstraction. The advantage here is that drivers don't have to care even a little bit about the buffer sharing mechanism, which simplifies the drivers. I.e., from patch #2, Aaron diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index 8bf695c..24e0aab 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -24,8 +24,6 @@ * */ -#include Can I consider this a Reviewed-by? --