From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting Date: Wed, 5 Jun 2013 10:38:33 +0200 Message-ID: <20130605083833.GG15743@phenom.ffwll.local> References: <1369990487-23510-1-git-send-email-sw0312.kim@samsung.com> <51A879E0.3080106@samsung.com> <20130531152956.GX15743@phenom.ffwll.local> <51ADC48E.80907@samsung.com> <20130604125558.GB15743@phenom.ffwll.local> <51AEA80B.8030008@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <51AEA80B.8030008@samsung.com> Sender: linux-kernel-owner@vger.kernel.org To: =?utf-8?B?6rmA7Iq57Jqw?= Cc: dri-devel , "linux-media@vger.kernel.org" , "linaro-mm-sig@lists.linaro.org" , Sumit Semwal , Dave Airlie , Linux Kernel Mailing List , Inki Dae , Kyungmin Park List-Id: dri-devel@lists.freedesktop.org On Wed, Jun 05, 2013 at 11:52:59AM +0900, =EA=B9=80=EC=8A=B9=EC=9A=B0 w= rote: >=20 >=20 > On 2013=EB=85=84 06=EC=9B=94 04=EC=9D=BC 21:55, Daniel Vetter wrote: > > On Tue, Jun 04, 2013 at 07:42:22PM +0900, =EA=B9=80=EC=8A=B9=EC=9A=B0= wrote: > >> > >> > >> On 2013=EB=85=84 06=EC=9B=94 01=EC=9D=BC 00:29, Daniel Vetter wrot= e: > >>> On Fri, May 31, 2013 at 07:22:24PM +0900, =EA=B9=80=EC=8A=B9=EC=9A= =B0 wrote: > >>>> Hello Daniel, > >>>> > >>>> Thanks for your comment. > >>>> > >>>> On 2013=EB=85=84 05=EC=9B=94 31=EC=9D=BC 18:14, Daniel Vetter wr= ote: > >>>>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim wrote: > >>>>>> importer private data in dma-buf attachment can be used by imp= orter to > >>>>>> reimport same dma-buf. > >>>>>> > >>>>>> Seung-Woo Kim (2): > >>>>>> dma-buf: add importer private data to attachment > >>>>>> drm/prime: find gem object from the reimported dma-buf > >>>>> > >>>>> Self-import should already work (at least with the latest refco= unt > >>>>> fixes merged). At least the tests to check both re-import on th= e same > >>>>> drm fd and on a different all work as expected now. > >>>> > >>>> Currently, prime works well for all case including self-importin= g, > >>>> importing, and reimporting as you describe. Just, importing dma-= buf from > >>>> other driver twice with different drm_fd, each import create its= own gem > >>>> object even two import is done for same buffer because prime_pri= v is in > >>>> struct drm_file. This means mapping to the device is done also t= wice. > >>>> IMHO, these duplicated creations and maps are not necessary if d= rm can > >>>> find previous import in different prime_priv. > >>> > >>> Well, that's imo a bug with the other driver. If it doesn't expor= t > >>> something really simple (e.g. contiguous memory which doesn't req= uire any > >>> mmio resources at all) it should have a cache of exported dma_buf= fds so > >>> that it hands out the same dma_buf every time. > >> > >> Hm, all existing dma-buf exporter including i915 driver implements= its > >> map_dma_buf callback as allocating scatter-gather table with pages= in > >> its buffer and calling dma_map_sg() with the sgt. With different > >> drm_fds, importing one dma-buf *twice*, then importer calls > >> dma_buf_attach() and dma_buf_map_attachment() twice at least in dr= m > >> importer because re-importing case can only checked with prime_pri= v in > >> drm_file as I described. > >=20 > > Well, but thanks to all the self-import and re-import checks, it's > > _impossible_ to import the same dma_buf twice without noticing (pre= suming > > both importer and exporter are drm devices). >=20 > No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), che= cks > re-import with following code. >=20 > ret =3D drm_prime_lookup_buf_handle(&file_priv->prime, > dma_buf, handle); >=20 > Unfortunately, file_priv is allocated per each open of drm node so th= is > code can only find re-import within same drm open context. >=20 > And driver specific import functions, like drm_gem_prime_import(), on= ly > check self-import like following code. >=20 > if (dma_buf->ops =3D=3D &drm_gem_prime_dmabuf_ops) { > obj =3D dma_buf->priv; > if (obj->dev =3D=3D dev) { > /* ... */ > } > } >=20 > This means some application like following can make re-import to > different gem objects. >=20 > int drm_fd1, drm_fd2, ret; > int dma_buf_fd; > struct drm_prime_handle prime1, prime2; >=20 > drm_fd1 =3D open(DRM_NODE, O_RDWR, 0); > drm_fd2 =3D open(DRM_NODE, O_RDWR, 0); >=20 > /* get some dma-buf_fd from other dma-buf exporter */ > prime1.fd =3D dma_buf_fd; > prime2.fd =3D dma_buf_fd; >=20 > ret =3D ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime1); > ret =3D ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime2); >=20 > This will import same dma-buf twice as different GEM object because > above checking codes can not check already imported gem object from t= he > dma-buf. Oh right, now I understand. Somehow I've always thought we already take care of this case, since I remember discussing it. To fix that we need a device-global import cache similar to how we alre= ady have one for each file_priv. I think we can reuse the same locking and refcounting scheme, but I haven't checked. The commit messages of the p= ast few prime changes have fairly good explanations of the tricky stuff goi= ng on there. Sorry for being dense for so long, I should have checked my idea of wha= t the drm prime code does with reality sooner ;-) Cheers, Daniel --=20 Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch