From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: Re: [PATCH 00/20] prime/flink fixes and related stuff Date: Sat, 27 Jul 2013 18:22:08 +0900 Message-ID: References: <1373958731-4132-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1017119165==" Return-path: Received: from mail-la0-f41.google.com (mail-la0-f41.google.com [209.85.215.41]) by gabe.freedesktop.org (Postfix) with ESMTP id 3FCA4E644D for ; Sat, 27 Jul 2013 02:22:09 -0700 (PDT) Received: by mail-la0-f41.google.com with SMTP id fn20so2957594lab.0 for ; Sat, 27 Jul 2013 02:22:08 -0700 (PDT) In-Reply-To: <1373958731-4132-1-git-send-email-daniel.vetter@ffwll.ch> 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: Daniel Vetter Cc: DRI Development List-Id: dri-devel@lists.freedesktop.org --===============1017119165== Content-Type: multipart/alternative; boundary=14dae94ed96f18109004e27acb5c --14dae94ed96f18109004e27acb5c Content-Type: text/plain; charset=ISO-8859-1 Hi Daniel, sorry for being late. 2013/7/16 Daniel Vetter > Hi all, > > This patch series is my 2nd real stab at fixing up the locking issues > around our > two buffer sharing mechanisms in gem: flink and prime. > > I think the approach taken here is much better than my first stab, and it > also > seems to no longer leak buffers ;-) There some assorted cleanup and prep > work > (and one i915 fix) thrown into the mix, it's all stuff I've stumbled over > while > digging through the code. > > Open issues left in prime-land after these patches: > - exynos probably wants a similar patch to "drm/i915: explicit store base > gem > object in dma_buf->priv". The current code should be correct, but it's a > bit > How about using stuffs of drm_prime instead of specific ones? Seem like that we could replace specific dmabuf stuffs with common ones of drm_prime, at least in case of Exynos: i.e. each driver can export a gem to a dmabuf through drm_gem_prime_export function of drm_prime instead of specific one. By doing so, I think we could remove duplicated codes of drivers, specific dmabuf stuffs. I'm not sure but it seems like that there is any reason you try to use existing stuffs with a little change: maybe the stuffs of drm_prime couldn't be used for all drm drivers commonly. Thanks, Inki Dae > tricky. I've opted not to do that since last time around I've touched > exynos a > bit it broke horribly ;-) > - The prime core should now no longer depend upon obj->import_attach being > set > by drivers in their prime_import callback. This should allos us to fix > udl > which really doesn't need (nor want, it confuses swiotlb among other > things) > a device attachment. Didn't write that patch since my displaylink seems > to > have died. > - There's still the issue Inki's team pointed out where if you import a > foreign > object on different fds you'll get different gem objects. So we need > some form > of a per-device import cache (on top of the per-file-priv dma-buf cache > we > already have). Didn't do this yet since I want to have good test coverage > (already started a bit), it looks like a bit more work and I'm not sure > about > the exact design of the code yet. > > Review and testing highly welcome. > > Cheers, Daniel > > Daniel Vetter (20): > drm: use common drm_gem_dmabuf_release in i915/exynos drivers > drm/i915: unpin backing storage in dmabuf_unmap > drm/i915: explicit store base gem object in dma_buf->priv > drm/prime: add a bit of documentation about gem_obj->import_attach > drm/gem: remove drm_gem_object_handle_unreference > drm/gem: inline drm_gem_object_handle_reference > drm/gem: move drm_gem_object_handle_unreference_unlocked into > drm_gem.c > drm/gem: remove bogus NULL check from > drm_gem_object_handle_unreference_unlocked > drm/gem: WARN about unbalanced handle refcounts > drm/gem: fix up flink name create race > drm/prime: fix error path in drm_gem_prime_fd_to_handle > drm/gem: make drm_gem_object_handle_unreference_unlocked static > drm/gem: create drm_gem_dumb_destroy > drm/prime: use proper pointer in drm_gem_prime_handle_to_fd > drm/prime: shrink critical section protected by prime lock > drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle > drm/gem: switch dev->object_name_lock to a mutex > drm/gem: completely close gem_open vs. gem_close races > drm/prime: proper locking+refcounting for obj->dma_buf link > drm/prime: Simplify drm_gem_remove_prime_handles > > drivers/gpu/drm/ast/ast_drv.c | 2 +- > drivers/gpu/drm/ast/ast_drv.h | 3 - > drivers/gpu/drm/ast/ast_main.c | 7 -- > drivers/gpu/drm/cirrus/cirrus_drv.c | 2 +- > drivers/gpu/drm/cirrus/cirrus_drv.h | 3 - > drivers/gpu/drm/cirrus/cirrus_main.c | 7 -- > drivers/gpu/drm/drm_fops.c | 1 + > drivers/gpu/drm/drm_gem.c | 192 > ++++++++++++++++++++--------- > drivers/gpu/drm/drm_gem_cma_helper.c | 10 -- > drivers/gpu/drm/drm_info.c | 2 +- > drivers/gpu/drm/drm_prime.c | 96 ++++++++++----- > drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +--- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 22 +--- > drivers/gpu/drm/exynos/exynos_drm_gem.h | 9 -- > drivers/gpu/drm/gma500/gem.c | 17 --- > drivers/gpu/drm/gma500/psb_drv.c | 2 +- > drivers/gpu/drm/gma500/psb_drv.h | 2 - > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 2 - > drivers/gpu/drm/i915/i915_gem.c | 7 -- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 34 +++-- > drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_drv.h | 3 - > drivers/gpu/drm/mgag200/mgag200_main.c | 7 -- > drivers/gpu/drm/nouveau/nouveau_display.c | 7 -- > drivers/gpu/drm/nouveau/nouveau_display.h | 2 - > drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +- > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- > drivers/gpu/drm/omapdrm/omap_drv.h | 2 - > drivers/gpu/drm/omapdrm/omap_gem.c | 15 --- > drivers/gpu/drm/qxl/qxl_drv.c | 2 +- > drivers/gpu/drm/qxl/qxl_drv.h | 3 - > drivers/gpu/drm/qxl/qxl_dumb.c | 7 -- > drivers/gpu/drm/radeon/radeon.h | 3 - > drivers/gpu/drm/radeon/radeon_drv.c | 5 +- > drivers/gpu/drm/radeon/radeon_gem.c | 7 -- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- > drivers/gpu/drm/shmobile/shmob_drm_drv.c | 2 +- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- > drivers/gpu/drm/udl/udl_drv.c | 2 +- > drivers/gpu/drm/udl/udl_drv.h | 2 - > drivers/gpu/drm/udl/udl_gem.c | 6 - > drivers/gpu/host1x/drm/drm.c | 2 +- > drivers/gpu/host1x/drm/gem.c | 6 - > drivers/gpu/host1x/drm/gem.h | 2 - > drivers/staging/imx-drm/imx-drm-core.c | 2 +- > include/drm/drmP.h | 94 +++++++------- > include/drm/drm_gem_cma_helper.h | 8 -- > 49 files changed, 279 insertions(+), 367 deletions(-) > > -- > 1.8.3.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > --14dae94ed96f18109004e27acb5c Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
Hi Daniel, sorry for being late.


2013/7/16 Daniel Vetter <daniel.vetter@ffwll.ch>
Hi all,

This patch series is my 2nd real stab at fixing up the locking issues aroun= d our
two buffer sharing mechanisms in gem: flink and prime.

I think the approach taken here is much better than my first stab, and it a= lso
seems to no longer leak buffers ;-) There some assorted cleanup and prep wo= rk
(and one i915 fix) thrown into the mix, it's all stuff I've stumble= d over while
digging through the code.

Open issues left in prime-land after these patches:
- exynos probably wants a similar patch to "drm/i915: explicit store b= ase gem
=A0 object in dma_buf->priv". The current code should be correct, b= ut it's a bit

How about using stuff= s of drm_prime instead of specific ones? Seem like that we could replace sp= ecific dmabuf stuffs with common ones of drm_prime, at least in case of Exy= nos: i.e. each driver can export a gem to a dmabuf through drm_gem_prime_ex= port function of drm_prime instead of specific one. By doing so, I think we= could remove duplicated codes of drivers, specific dmabuf stuffs. I'm = not sure but it seems like that there is any reason you try to use existing= stuffs with a little change: maybe the stuffs of drm_prime couldn't be= used for all drm drivers commonly.

Thanks,
Inki Dae
=A0
=A0 tricky. I've opted not to do that since last time around I've t= ouched exynos a
=A0 bit it broke horribly ;-)
- The prime core should now no longer depend upon obj->import_attach bei= ng set
=A0 by drivers in their prime_import callback. This should allos us to fix = udl
=A0 which really doesn't need (nor want, it confuses swiotlb among othe= r things)
=A0 a device attachment. Didn't write that patch since my displaylink s= eems to
=A0 have died.
- There's still the issue Inki's team pointed out where if you impo= rt a foreign
=A0 object on different fds you'll get different gem objects. So we nee= d some form
=A0 of a per-device import cache (on top of the per-file-priv dma-buf cache= we
=A0 already have). Didn't do this yet since I want to have good test co= verage
=A0 (already started a bit), it looks like a bit more work and I'm not = sure about
=A0 the exact design of the code yet.

Review and testing highly welcome.

Cheers, Daniel

Daniel Vetter (20):
=A0 drm: use common drm_gem_dmabuf_release in i915/exynos drivers
=A0 drm/i915: unpin backing storage in dmabuf_unmap
=A0 drm/i915: explicit store base gem object in dma_buf->priv
=A0 drm/prime: add a bit of documentation about gem_obj->import_attach =A0 drm/gem: remove drm_gem_object_handle_unreference
=A0 drm/gem: inline drm_gem_object_handle_reference
=A0 drm/gem: move drm_gem_object_handle_unreference_unlocked into
=A0 =A0 drm_gem.c
=A0 drm/gem: remove bogus NULL check from
=A0 =A0 drm_gem_object_handle_unreference_unlocked
=A0 drm/gem: WARN about unbalanced handle refcounts
=A0 drm/gem: fix up flink name create race
=A0 drm/prime: fix error path in drm_gem_prime_fd_to_handle
=A0 drm/gem: make drm_gem_object_handle_unreference_unlocked static
=A0 drm/gem: create drm_gem_dumb_destroy
=A0 drm/prime: use proper pointer in drm_gem_prime_handle_to_fd
=A0 drm/prime: shrink critical section protected by prime lock
=A0 drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle
=A0 drm/gem: switch dev->object_name_lock to a mutex
=A0 drm/gem: completely close gem_open vs. gem_close races
=A0 drm/prime: proper locking+refcounting for obj->dma_buf link
=A0 drm/prime: Simplify drm_gem_remove_prime_handles

=A0drivers/gpu/drm/ast/ast_drv.c =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/ast/ast_drv.h =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 3 -
=A0drivers/gpu/drm/ast/ast_main.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 7 --
=A0drivers/gpu/drm/cirrus/cirrus_drv.c =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/cirrus/cirrus_drv.h =A0 =A0 =A0 =A0| =A0 3 -
=A0drivers/gpu/drm/cirrus/cirrus_main.c =A0 =A0 =A0 | =A0 7 --
=A0drivers/gpu/drm/drm_fops.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 1 +
=A0drivers/gpu/drm/drm_gem.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| 192 +++++= +++++++++++++++---------
=A0drivers/gpu/drm/drm_gem_cma_helper.c =A0 =A0 =A0 | =A010 --
=A0drivers/gpu/drm/drm_info.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 2 +- =A0drivers/gpu/drm/drm_prime.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A096 +++++= +++++-----
=A0drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | =A023 +---
=A0drivers/gpu/drm/exynos/exynos_drm_drv.c =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/exynos/exynos_drm_gem.c =A0 =A0| =A022 +---
=A0drivers/gpu/drm/exynos/exynos_drm_gem.h =A0 =A0| =A0 9 --
=A0drivers/gpu/drm/gma500/gem.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A017 ---
=A0drivers/gpu/drm/gma500/psb_drv.c =A0 =A0 =A0 =A0 =A0 | =A0 2 +-
=A0drivers/gpu/drm/gma500/psb_drv.h =A0 =A0 =A0 =A0 =A0 | =A0 2 -
=A0drivers/gpu/drm/i915/i915_drv.c =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/i915/i915_drv.h =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 -
=A0drivers/gpu/drm/i915/i915_gem.c =A0 =A0 =A0 =A0 =A0 =A0| =A0 7 --
=A0drivers/gpu/drm/i915/i915_gem_dmabuf.c =A0 =A0 | =A034 +++--
=A0drivers/gpu/drm/mgag200/mgag200_drv.c =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/mgag200/mgag200_drv.h =A0 =A0 =A0| =A0 3 -
=A0drivers/gpu/drm/mgag200/mgag200_main.c =A0 =A0 | =A0 7 --
=A0drivers/gpu/drm/nouveau/nouveau_display.c =A0| =A0 7 --
=A0drivers/gpu/drm/nouveau/nouveau_display.h =A0| =A0 2 -
=A0drivers/gpu/drm/nouveau/nouveau_drm.c =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/omapdrm/omap_drv.c =A0 =A0 =A0 =A0 | =A0 2 +-
=A0drivers/gpu/drm/omapdrm/omap_drv.h =A0 =A0 =A0 =A0 | =A0 2 -
=A0drivers/gpu/drm/omapdrm/omap_gem.c =A0 =A0 =A0 =A0 | =A015 ---
=A0drivers/gpu/drm/qxl/qxl_drv.c =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/qxl/qxl_drv.h =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 3 -
=A0drivers/gpu/drm/qxl/qxl_dumb.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 7 --
=A0drivers/gpu/drm/radeon/radeon.h =A0 =A0 =A0 =A0 =A0 =A0| =A0 3 -
=A0drivers/gpu/drm/radeon/radeon_drv.c =A0 =A0 =A0 =A0| =A0 5 +-
=A0drivers/gpu/drm/radeon/radeon_gem.c =A0 =A0 =A0 =A0| =A0 7 --
=A0drivers/gpu/drm/rcar-du/rcar_du_drv.c =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/shmobile/shmob_drm_drv.c =A0 | =A0 2 +-
=A0drivers/gpu/drm/tilcdc/tilcdc_drv.c =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/udl/udl_drv.c =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 +-
=A0drivers/gpu/drm/udl/udl_drv.h =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 2 -
=A0drivers/gpu/drm/udl/udl_gem.c =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 6 -
=A0drivers/gpu/host1x/drm/drm.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 2 +-
=A0drivers/gpu/host1x/drm/gem.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 6 -
=A0drivers/gpu/host1x/drm/gem.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 2 -
=A0drivers/staging/imx-drm/imx-drm-core.c =A0 =A0 | =A0 2 +-
=A0include/drm/drmP.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0= 94 +++++++-------
=A0include/drm/drm_gem_cma_helper.h =A0 =A0 =A0 =A0 =A0 | =A0 8 --
=A049 files changed, 279 insertions(+), 367 deletions(-)

--
1.8.3.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesk= top.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

--14dae94ed96f18109004e27acb5c-- --===============1017119165== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1017119165==--