From patchwork Sun Jun 17 14:45:51 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rob Clark X-Patchwork-Id: 9371 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id BA75D23EE1 for ; Sun, 17 Jun 2012 14:46:04 +0000 (UTC) Received: from mail-yw0-f52.google.com (mail-yw0-f52.google.com [209.85.213.52]) by fiordland.canonical.com (Postfix) with ESMTP id 5AE2CA18973 for ; Sun, 17 Jun 2012 14:46:04 +0000 (UTC) Received: by yhpp61 with SMTP id p61so3606131yhp.11 for ; Sun, 17 Jun 2012 07:46:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf :dkim-signature:sender:from:to:cc:subject:date:message-id:x-mailer :x-gm-message-state; bh=jCbn4ZLCqPk4sk4Q2CyqJ2JXZ+VGSZDKhZPayCPLHHw=; b=L1fu8DugMHS4j9Vqk2Jn8SuWjRpYU+0WmRHJIvoBL1BRoXSyePxIN/11iLZzR28mdd eadzb4aCJ5aQFD0QqDgO0D93du/Bk/gTK3tPLMX5QT9CQh7On+MR98aF3CTo3weG1jKw 3HGkhbLJv0nJTvBA/pLZDveMysQNbqqxTW+H04Qp2ze/blOWCVRemZJgX5+oxi+/ZK+i qv129gKWZ8ExooDGl5aYEJRPGyhbASwMZVjPvx6hbOpN7bM0Im4ObjIssd5oVSGUuBEk 1rY92KuREw/UgMe6vzTNpbwaXs9m9sT0CXOiW+cTQrh7kRExouyuZUZFfRlq4tXmsvzw +ASw== Received: by 10.50.46.232 with SMTP id y8mr6191502igm.57.1339944363541; Sun, 17 Jun 2012 07:46:03 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.231.24.148 with SMTP id v20csp24604ibb; Sun, 17 Jun 2012 07:46:02 -0700 (PDT) Received: by 10.182.167.39 with SMTP id zl7mr12589461obb.10.1339944361717; Sun, 17 Jun 2012 07:46:01 -0700 (PDT) Received: from mail-ob0-f178.google.com (mail-ob0-f178.google.com [209.85.214.178]) by mx.google.com with ESMTPS id hx8si10051376obb.15.2012.06.17.07.46.01 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 17 Jun 2012 07:46:01 -0700 (PDT) Received-SPF: pass (google.com: domain of robdclark@gmail.com designates 209.85.214.178 as permitted sender) client-ip=209.85.214.178; Authentication-Results: mx.google.com; spf=pass (google.com: domain of robdclark@gmail.com designates 209.85.214.178 as permitted sender) smtp.mail=robdclark@gmail.com; dkim=pass header.i=@gmail.com Received: by obceq6 with SMTP id eq6so9715676obc.37 for ; Sun, 17 Jun 2012 07:46:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer; bh=jCbn4ZLCqPk4sk4Q2CyqJ2JXZ+VGSZDKhZPayCPLHHw=; b=XfqmxbN4eMPtPPlLyqHbAXHhFRYQLYGr1kP0qBTmVg5CKcZ22HfpEY2Jyt+l+HSs6b OnBHGPBFLH5eUurElwX3P/qbsVso8Jp7oLp8J3F4iKBq7KugYilNJuz0KzdYD7kMTDM3 OUKZvDvCNR5uH6v7pIK7r1cGxZW6ymQOUgfZFW/BlQpDAY9ARRtxP1b/kGwEaYDCFlFq SVPOuU5F5rKvs4yzRIv3xeclOj1RDbHtjLOhJfwgYCMNEVUIj3JMf4ahy8X8vs1gXuIg jz1DvPLLuo5V07/meR52r38+WHy8XoSVInl9M8U0PoZoIWN9w1iznBtx7uOjANynPgc1 nLzQ== Received: by 10.60.4.165 with SMTP id l5mr12468662oel.41.1339944361457; Sun, 17 Jun 2012 07:46:01 -0700 (PDT) Received: from localhost (ppp-70-253-38-6.dsl.rcsntx.swbell.net. [70.253.38.6]) by mx.google.com with ESMTPS id n9sm9254569oen.2.2012.06.17.07.46.00 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 17 Jun 2012 07:46:00 -0700 (PDT) Sender: Rob Clark From: Rob Clark To: dri-devel@lists.freedesktop.org Cc: patches@linaro.org, Rob Clark Subject: [RFC libdrm] omap: add refcnting and handle tracking Date: Sun, 17 Jun 2012 09:45:51 -0500 Message-Id: <1339944351-24709-1-git-send-email-rob.clark@linaro.org> X-Mailer: git-send-email 1.7.9.5 X-Gm-Message-State: ALoCoQldiJvnYj2ub/bFW/CcswZW760jBvF2e7Jje9rhFIbnQk0SjHvnWjkr9/fiIOW6YsKGeqEJ From: Rob Clark There can be scenarios, especially when re-importing an existing buffer, where you end up with multiple 'struct omap_bo's wrapping a single GEM object handle. Which causes badness when the first of the evil-clones is omap_bo_del()'d. To do this, introduce reference counting and a hashtable to track the handles per fd. First, to avoid bo's slipping through the crack if multiple 'struct omap_device's are created for one drm fd, a hashtable mapping drm fd to omap_device, and the omap_device itself is reference counted. Per omap_device, we keep a handle_table mapping GEM handle to omap_bo. When buffers are imported from flink name or dmabuf fd, the handle table is consulted, and if an omap_bo already exists, it's refcnt is incremented and it is returned. For good measure, to avoid the handle_table being deleted before the omap_bo is freed, the omap_bo holds a reference to the omap_device. TODO: check the overhead of the hashtable. If too much we could maybe get away with only tracking exported and imported bo's in the table. TODO: all the import/export flink/dmabuf operations are generic DRM ioctls. Really all this functionality could be handled by a generic drm_bo and drm_device "base class" that could be extended by omap, exynos, etc. That would also allow more common userspace code by avoiding artificial libdrm_omap dependencies. --- omap/omap_drm.c | 150 +++++++++++++++++++++++++++++++++++++++++++---------- omap/omap_drmif.h | 2 + 2 files changed, 126 insertions(+), 26 deletions(-) diff --git a/omap/omap_drm.c b/omap/omap_drm.c index 1d37e45..cd8e8bc 100644 --- a/omap/omap_drm.c +++ b/omap/omap_drm.c @@ -32,12 +32,15 @@ #include #include +#include #include #include #include #include +#include #include +#include #include "omap_drm.h" #include "omap_drmif.h" @@ -46,8 +49,23 @@ #define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1) #define PAGE_SIZE 4096 +static pthread_mutex_t table_lock = PTHREAD_MUTEX_INITIALIZER; +static void * dev_table; + struct omap_device { int fd; + atomic_t refcnt; + + /* The handle_table is used to track GEM bo handles associated w/ + * this fd. This is needed, in particular, when importing + * dmabuf's because we don't want multiple 'struct omap_bo's + * floating around with the same handle. Otherwise, when the + * first one is omap_bo_del()'d the handle becomes no longer + * valid, and the remaining 'struct omap_bo's are left pointing + * to an invalid handle (and possible a GEM bo that is already + * free'd). + */ + void *handle_table; }; /* a GEM buffer object allocated from the DRM device */ @@ -59,19 +77,57 @@ struct omap_bo { uint32_t name; /* flink global handle (DRI2 name) */ uint64_t offset; /* offset to mmap() */ int fd; /* dmabuf handle */ + atomic_t refcnt; }; -struct omap_device * omap_device_new(int fd) +static struct omap_device * omap_device_new_impl(int fd) { struct omap_device *dev = calloc(sizeof(*dev), 1); if (!dev) return NULL; dev->fd = fd; + atomic_set(&dev->refcnt, 1); + dev->handle_table = drmHashCreate(); + return dev; +} + +struct omap_device * omap_device_new(int fd) +{ + struct omap_device *dev = NULL; + + pthread_mutex_lock(&table_lock); + + if (!dev_table) + dev_table = drmHashCreate(); + + if (drmHashLookup(dev_table, fd, (void **)&dev)) { + /* not found, create new device */ + dev = omap_device_new_impl(fd); + drmHashInsert(dev_table, fd, dev); + } else { + /* found, just incr refcnt */ + dev = omap_device_ref(dev); + } + + pthread_mutex_unlock(&table_lock); + + return dev; +} + +struct omap_device * omap_device_ref(struct omap_device *dev) +{ + atomic_inc(&dev->refcnt); return dev; } void omap_device_del(struct omap_device *dev) { + if (!atomic_dec_and_test(&dev->refcnt)) + return; + pthread_mutex_lock(&table_lock); + drmHashDestroy(dev->handle_table); + drmHashDelete(dev_table, dev->fd); + pthread_mutex_unlock(&table_lock); free(dev); } @@ -101,6 +157,38 @@ int omap_set_param(struct omap_device *dev, uint64_t param, uint64_t value) return drmCommandWrite(dev->fd, DRM_OMAP_SET_PARAM, &req, sizeof(req)); } +/* lookup a buffer from it's handle, call w/ table_lock held: */ +static struct omap_bo * lookup_bo(struct omap_device *dev, + uint32_t handle) +{ + struct omap_bo *bo = NULL; + if (!drmHashLookup(dev->handle_table, handle, (void **)&bo)) { + /* found, incr refcnt and return: */ + bo = omap_bo_ref(bo); + } + return bo; +} + +/* allocate a new buffer object, call w/ table_lock held */ +static struct omap_bo * bo_from_handle(struct omap_device *dev, + uint32_t handle) +{ + struct omap_bo *bo = calloc(sizeof(*bo), 1); + if (!bo) { + struct drm_gem_close req = { + .handle = handle, + }; + drmIoctl(dev->fd, DRM_IOCTL_GEM_CLOSE, &req); + return NULL; + } + bo->dev = omap_device_ref(dev); + bo->handle = handle; + atomic_set(&bo->refcnt, 1); + /* add ourselves to the handle table: */ + drmHashInsert(dev->handle_table, handle, bo); + return bo; +} + /* allocate a new buffer object */ static struct omap_bo * omap_bo_new_impl(struct omap_device *dev, union omap_gem_size size, uint32_t flags) @@ -115,12 +203,13 @@ static struct omap_bo * omap_bo_new_impl(struct omap_device *dev, goto fail; } - bo = calloc(sizeof(*bo), 1); - if (!bo) { + if (drmCommandWriteRead(dev->fd, DRM_OMAP_GEM_NEW, &req, sizeof(req))) { goto fail; } - bo->dev = dev; + pthread_mutex_lock(&table_lock); + bo = bo_from_handle(dev, req.handle); + pthread_mutex_unlock(&table_lock); if (flags & OMAP_BO_TILED) { bo->size = round_up(size.tiled.width, PAGE_SIZE) * size.tiled.height; @@ -128,12 +217,6 @@ static struct omap_bo * omap_bo_new_impl(struct omap_device *dev, bo->size = size.bytes; } - if (drmCommandWriteRead(dev->fd, DRM_OMAP_GEM_NEW, &req, sizeof(req))) { - goto fail; - } - - bo->handle = req.handle; - return bo; fail: @@ -171,6 +254,12 @@ struct omap_bo * omap_bo_new_tiled(struct omap_device *dev, return omap_bo_new_impl(dev, gsize, flags); } +struct omap_bo * omap_bo_ref(struct omap_bo *bo) +{ + atomic_inc(&bo->refcnt); + return bo; +} + /* get buffer info */ static int get_buffer_info(struct omap_bo *bo) { @@ -193,23 +282,24 @@ static int get_buffer_info(struct omap_bo *bo) /* import a buffer object from DRI2 name */ struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name) { - struct omap_bo *bo; + struct omap_bo *bo = NULL; struct drm_gem_open req = { .name = name, }; - bo = calloc(sizeof(*bo), 1); - if (!bo) { - goto fail; - } + pthread_mutex_lock(&table_lock); if (drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req)) { goto fail; } - bo->dev = dev; - bo->name = name; - bo->handle = req.handle; + bo = lookup_bo(dev, req.handle); + if (!bo) { + bo = bo_from_handle(dev, req.handle); + bo->name = name; + } + + pthread_mutex_unlock(&table_lock); return bo; @@ -224,24 +314,25 @@ fail: */ struct omap_bo * omap_bo_from_dmabuf(struct omap_device *dev, int fd) { - struct omap_bo *bo; + struct omap_bo *bo = NULL; struct drm_prime_handle req = { .fd = fd, }; int ret; - bo = calloc(sizeof(*bo), 1); - if (!bo) { - goto fail; - } + pthread_mutex_lock(&table_lock); ret = drmIoctl(dev->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &req); if (ret) { goto fail; } - bo->dev = dev; - bo->handle = req.handle; + bo = lookup_bo(dev, req.handle); + if (!bo) { + bo = bo_from_handle(dev, req.handle); + } + + pthread_mutex_unlock(&table_lock); return bo; @@ -257,6 +348,9 @@ void omap_bo_del(struct omap_bo *bo) return; } + if (!atomic_dec_and_test(&bo->refcnt)) + return; + if (bo->map) { munmap(bo->map, bo->size); } @@ -269,10 +363,14 @@ void omap_bo_del(struct omap_bo *bo) struct drm_gem_close req = { .handle = bo->handle, }; - + pthread_mutex_lock(&table_lock); + drmHashDelete(bo->dev->handle_table, bo->handle); drmIoctl(bo->dev->fd, DRM_IOCTL_GEM_CLOSE, &req); + pthread_mutex_unlock(&table_lock); } + omap_device_del(bo->dev); + free(bo); } diff --git a/omap/omap_drmif.h b/omap/omap_drmif.h index 284b9cc..e62d127 100644 --- a/omap/omap_drmif.h +++ b/omap/omap_drmif.h @@ -38,6 +38,7 @@ struct omap_device; */ struct omap_device * omap_device_new(int fd); +struct omap_device * omap_device_ref(struct omap_device *dev); void omap_device_del(struct omap_device *dev); int omap_get_param(struct omap_device *dev, uint64_t param, uint64_t *value); int omap_set_param(struct omap_device *dev, uint64_t param, uint64_t value); @@ -49,6 +50,7 @@ struct omap_bo * omap_bo_new(struct omap_device *dev, uint32_t size, uint32_t flags); struct omap_bo * omap_bo_new_tiled(struct omap_device *dev, uint32_t width, uint32_t height, uint32_t flags); +struct omap_bo * omap_bo_ref(struct omap_bo *bo); struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name); struct omap_bo * omap_bo_from_dmabuf(struct omap_device *dev, int fd); void omap_bo_del(struct omap_bo *bo);