Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934429AbdDGQ63 (ORCPT ); Fri, 7 Apr 2017 12:58:29 -0400 Received: from mail.fireflyinternet.com ([109.228.58.192]:59098 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933435AbdDGQ6V (ORCPT ); Fri, 7 Apr 2017 12:58:21 -0400 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Date: Fri, 7 Apr 2017 17:58:11 +0100 From: Chris Wilson To: Laura Abbott Cc: Daniel Vetter , Sean Paul , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Sumit Semwal Subject: Re: [RFC PATCH 2/2] drm/vgem: Enable dmabuf import interfaces Message-ID: <20170407165811.GT10496@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Laura Abbott , Daniel Vetter , Sean Paul , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Sumit Semwal References: <1491520713-14277-1-git-send-email-labbott@redhat.com> <1491520713-14277-3-git-send-email-labbott@redhat.com> <20170407073957.GT10496@nuc-i3427.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3168 Lines: 80 On Fri, Apr 07, 2017 at 09:18:30AM -0700, Laura Abbott wrote: > On 04/07/2017 12:39 AM, Chris Wilson wrote: > > On Thu, Apr 06, 2017 at 04:18:33PM -0700, Laura Abbott wrote: > >> > >> Enable the GEM dma-buf import interfaces in addition to the export > >> interfaces. This lets vgem be used as a test source for other allocators > >> (e.g. Ion). > >> > >> +int vgem_gem_get_pages(struct drm_vgem_gem_object *obj) > >> +{ > >> + struct page **pages; > >> + > >> + if (obj->pages) > >> + return 0; > >> + > >> + pages = drm_gem_get_pages(&obj->base); > >> + if (IS_ERR(pages)) { > >> + return PTR_ERR(pages); > >> + } > >> + > >> + obj->pages = pages; > > > > That's a significant loss in functionality (it requires the entire > > object to fit into physical memory at the same time and requires a large > > vmalloc for 32b systems), for what? vgem only has the ability to mmap > > and export a fd -- both of which you already have if you have the dmabuf > > fd. The only extra interface is the signaling, which does not yet have a > > direct correspondence on dmabuf. > > > > (An obvious way to keep both would be to move the get_pages to importing > > and then differentiate in the faulthandler where to get the page from.) > > > > Thanks for pointing this out. I'll look to keep the existing behavior. > > > Can you provide more details on how you are using vgem to justify the > > changes? I'm also not particularly happy in losing testing of a virtual > > platform device from our CI. > > -Chris > > > > There exists a test module in the Ion directory > (drivers/staging/android/ion/ion_test.c) today but it's not actually > Ion specific. It registers a platform device and then provides an > ioctl interface to perform a dma_buf attach and map. I proposed > moving this to a generic dma-buf test module > (https://marc.info/?l=dri-devel&m=148952187004611&w=2) and Daniel > suggested that this was roughly what vgem was supposed to do. mmap(dma_buf_fd) gives you DMA_BUF_IOC_TEST_DMA_MAPPING, and that's the only facility already available via vgem. DMA_BUF_IOC_TEST_KERNEL_MAPPING would be equivalent to read/write(dma_buf_fd). > Adding the import methods for vgem covers all of what the > Ion test was doing and we don't have to invent yet another ioctl > interface and framework for attaching and mapping. I don't think it does :) To muddy the waters further, I've also done something similar: https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/mock_dmabuf.c https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/selftests/i915_gem_dmabuf.c But this feels like a good enough reason for implementing read/write ops for the dma-buf fd, and then we can test the dma_buf->ops->kmap on i915 as well, directly from i915.ko. Sounds fun, I'll see if I can cook something up - and we can then see if that suits your testing as well. > Can you clarify about what you mean about 'virtual platform device'? vgem_device = drm_dev_alloc(&vgem_driver, NULL); helps exercise some more corner cases of the drm core, that we have unwittingly broken in the past. -Chris -- Chris Wilson, Intel Open Source Technology Centre