On Wed, Jun 05, 2013 at 11:52:59AM +0900, 김승우 wrote: > > > On 2013년 06월 04일 21:55, Daniel Vetter wrote: > > On Tue, Jun 04, 2013 at 07:42:22PM +0900, 김승우 wrote: > >> > >> > >> On 2013년 06월 01일 00:29, Daniel Vetter wrote: > >>> On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote: > >>>> Hello Daniel, > >>>> > >>>> Thanks for your comment. > >>>> > >>>> On 2013년 05월 31일 18:14, Daniel Vetter wrote: > >>>>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim > >>>>> <sw0312....@samsung.com> wrote: > >>>>>> importer private data in dma-buf attachment can be used by importer 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 refcount > >>>>> fixes merged). At least the tests to check both re-import on the same > >>>>> drm fd and on a different all work as expected now. > >>>> > >>>> Currently, prime works well for all case including self-importing, > >>>> 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_priv is in > >>>> struct drm_file. This means mapping to the device is done also twice. > >>>> IMHO, these duplicated creations and maps are not necessary if drm can > >>>> find previous import in different prime_priv. > >>> > >>> Well, that's imo a bug with the other driver. If it doesn't export > >>> something really simple (e.g. contiguous memory which doesn't require 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 drm > >> importer because re-importing case can only checked with prime_priv in > >> drm_file as I described. > > > > Well, but thanks to all the self-import and re-import checks, it's > > _impossible_ to import the same dma_buf twice without noticing (presuming > > both importer and exporter are drm devices). > > No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), checks > re-import with following code. > > ret = drm_prime_lookup_buf_handle(&file_priv->prime, > dma_buf, handle); > > Unfortunately, file_priv is allocated per each open of drm node so this > code can only find re-import within same drm open context. > > And driver specific import functions, like drm_gem_prime_import(), only > check self-import like following code. > > if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) { > obj = dma_buf->priv; > if (obj->dev == dev) { > /* ... */ > } > } > > This means some application like following can make re-import to > different gem objects. > > int drm_fd1, drm_fd2, ret; > int dma_buf_fd; > struct drm_prime_handle prime1, prime2; > > drm_fd1 = open(DRM_NODE, O_RDWR, 0); > drm_fd2 = open(DRM_NODE, O_RDWR, 0); > > /* get some dma-buf_fd from other dma-buf exporter */ > prime1.fd = dma_buf_fd; > prime2.fd = dma_buf_fd; > > ret = ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime1); > ret = ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime2); > > This will import same dma-buf twice as different GEM object because > above checking codes can not check already imported gem object from the > 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 already 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 past few prime changes have fairly good explanations of the tricky stuff going on there. Sorry for being dense for so long, I should have checked my idea of what the drm prime code does with reality sooner ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/