| Message ID | 20170622134617.17912-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 22, 2017 at 02:46:17PM +0100, Chris Wilson wrote: > When the caller maps their dmabuf and we return an sg_table, the caller > doesn't expect the pages beneath that sg_table to vanish on a whim (i.e. > under mempressure). The contract is that the pages are pinned for the > duration of the mapping (from dma_buf_map_attachment() to > dma_buf_unmap_attachment). To comply, we need to introduce our own > vgem_object.pages_pin_count and elevate it across the mapping. However, > the drm_prime interface we use calls drv->prime_pin on dma_buf_attach > and drv->prime_unpin on dma_buf_detach, which while that does cover the > mapping is much broader than is desired -- but it will do for now. We could/should probably fix that ... Most drivers hold onto the mapping forever anyway. > v2: also hold the pin across prime_vmap/vunmap > > Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com> > Testcase: igt/gem_concurrent_blit/*swap*vgem* > Fixes: 5ba6c9ff961a ("drm/vgem: Fix mmaping") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com> > Cc: Laura Abbott <labbott@redhat.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: <stable@vger.kernel.org> > --- > drivers/gpu/drm/vgem/vgem_drv.c | 81 ++++++++++++++++++++++++++++++----------- > drivers/gpu/drm/vgem/vgem_drv.h | 4 ++ > 2 files changed, 64 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c > index 18f401b442c2..c938af8c40cf 100644 > --- a/drivers/gpu/drm/vgem/vgem_drv.c > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > @@ -52,6 +52,7 @@ static void vgem_gem_free_object(struct drm_gem_object *obj) > struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj); > > kvfree(vgem_obj->pages); > + mutex_destroy(&vgem_obj->pages_lock); > > if (obj->import_attach) > drm_prime_gem_destroy(obj, vgem_obj->table); > @@ -76,11 +77,15 @@ static int vgem_gem_fault(struct vm_fault *vmf) > if (page_offset > num_pages) > return VM_FAULT_SIGBUS; > > + ret = -ENOENT; > + mutex_lock(&obj->pages_lock); > if (obj->pages) { > get_page(obj->pages[page_offset]); > vmf->page = obj->pages[page_offset]; > ret = 0; > - } else { > + } > + mutex_unlock(&obj->pages_lock); > + if (ret) { > struct page *page; > > page = shmem_read_mapping_page( > @@ -161,6 +166,8 @@ static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev, > return ERR_PTR(ret); > } > > + mutex_init(&obj->pages_lock); > + > return obj; > } > > @@ -274,37 +281,66 @@ static const struct file_operations vgem_driver_fops = { > .release = drm_release, > }; > > +static struct page **vgem_pin_pages(struct drm_vgem_gem_object *bo) > +{ > + mutex_lock(&bo->pages_lock); > + if (bo->pages_pin_count++ == 0) { > + struct page **pages; > + > + pages = drm_gem_get_pages(&bo->base); > + if (IS_ERR(pages)) { > + bo->pages_pin_count--; > + mutex_unlock(&bo->pages_lock); > + return pages; > + } > + > + bo->pages = pages; > + } > + mutex_unlock(&bo->pages_lock); > + > + return bo->pages; > +} > + > +static void vgem_unpin_pages(struct drm_vgem_gem_object *bo) > +{ > + mutex_lock(&bo->pages_lock); > + if (--bo->pages_pin_count == 0) { > + drm_gem_put_pages(&bo->base, bo->pages, true, true); > + bo->pages = NULL; > + } > + mutex_unlock(&bo->pages_lock); > +} > + > static int vgem_prime_pin(struct drm_gem_object *obj) > { > + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); > long n_pages = obj->size >> PAGE_SHIFT; > struct page **pages; > > - /* Flush the object from the CPU cache so that importers can rely > - * on coherent indirect access via the exported dma-address. > - */ > - pages = drm_gem_get_pages(obj); > + pages = vgem_pin_pages(bo); > if (IS_ERR(pages)) > return PTR_ERR(pages); > > + /* Flush the object from the CPU cache so that importers can rely > + * on coherent indirect access via the exported dma-address. > + */ > drm_clflush_pages(pages, n_pages); Just spotted this, but at least on x86 dma is supposed to be coherent. We're the exception. But then this is all ill-defined with dma_buf, so meh. > - drm_gem_put_pages(obj, pages, true, false); > > return 0; > } > > -static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj) > +static void vgem_prime_unpin(struct drm_gem_object *obj) > { > - struct sg_table *st; > - struct page **pages; > + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); > > - pages = drm_gem_get_pages(obj); > - if (IS_ERR(pages)) > - return ERR_CAST(pages); > + vgem_unpin_pages(bo); > +} > > - st = drm_prime_pages_to_sg(pages, obj->size >> PAGE_SHIFT); > - drm_gem_put_pages(obj, pages, false, false); > +static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj) > +{ > + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); > > - return st; > + return drm_prime_pages_to_sg(bo->pages, bo->base.size >> PAGE_SHIFT); > } > > static struct drm_gem_object* vgem_prime_import(struct drm_device *dev, > @@ -333,6 +369,8 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev, > __vgem_gem_destroy(obj); > return ERR_PTR(-ENOMEM); > } > + > + obj->pages_pin_count++; /* perma-pinned */ > drm_prime_sg_to_page_addr_arrays(obj->table, obj->pages, NULL, > npages); > return &obj->base; > @@ -340,23 +378,23 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev, > > static void *vgem_prime_vmap(struct drm_gem_object *obj) > { > + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); > long n_pages = obj->size >> PAGE_SHIFT; > struct page **pages; > - void *addr; > > - pages = drm_gem_get_pages(obj); > + pages = vgem_pin_pages(bo); > if (IS_ERR(pages)) > return NULL; > > - addr = vmap(pages, n_pages, 0, pgprot_writecombine(PAGE_KERNEL)); > - drm_gem_put_pages(obj, pages, false, false); > - > - return addr; > + return vmap(pages, n_pages, 0, pgprot_writecombine(PAGE_KERNEL)); > } > > static void vgem_prime_vunmap(struct drm_gem_object *obj, void *vaddr) > { > + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); > + > vunmap(vaddr); > + vgem_unpin_pages(bo); > } > > static int vgem_prime_mmap(struct drm_gem_object *obj, > @@ -409,6 +447,7 @@ static struct drm_driver vgem_driver = { > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > .gem_prime_pin = vgem_prime_pin, > + .gem_prime_unpin = vgem_prime_unpin, > .gem_prime_import = vgem_prime_import, > .gem_prime_export = drm_gem_prime_export, > .gem_prime_import_sg_table = vgem_prime_import_sg_table, > diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h > index 1aae01419112..5c8f6d619ff3 100644 > --- a/drivers/gpu/drm/vgem/vgem_drv.h > +++ b/drivers/gpu/drm/vgem/vgem_drv.h > @@ -43,7 +43,11 @@ struct vgem_file { > #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base) > struct drm_vgem_gem_object { > struct drm_gem_object base; > + > struct page **pages; > + unsigned int pages_pin_count; > + struct mutex pages_lock; > + > struct sg_table *table; > }; > > -- > 2.11.0 Anyway looks all good, will push to drm-misc-fixes. Thanks, Daniel >
On Fri, Jun 23, 2017 at 01:02:53PM +0200, Daniel Vetter wrote:
> Anyway looks all good, will push to drm-misc-fixes.
Correction, pushed to -misc-next because it conflicts with the dma-buf
import stuff from Laura and other bits. If you want it in -fixes I need a
backport. I left the cc: stable in just to annoy Greg a bit :-)
-Daniel
Quoting Daniel Vetter (2017-06-23 12:02:53) > On Thu, Jun 22, 2017 at 02:46:17PM +0100, Chris Wilson wrote: > > + /* Flush the object from the CPU cache so that importers can rely > > + * on coherent indirect access via the exported dma-address. > > + */ > > drm_clflush_pages(pages, n_pages); > > Just spotted this, but at least on x86 dma is supposed to be coherent. > We're the exception. But then this is all ill-defined with dma_buf, so > meh. It's been a running debate on whether these are meant to be WC mapped or not. dmabuf does have its sync ioctls that do allow us to mix WB for clients and UC for device, which for us is very important. But there are a few edge cases like device importing pages from a dmabuf as above. -Chris
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 18f401b442c2..c938af8c40cf 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -52,6 +52,7 @@ static void vgem_gem_free_object(struct drm_gem_object *obj) struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj); kvfree(vgem_obj->pages); + mutex_destroy(&vgem_obj->pages_lock); if (obj->import_attach) drm_prime_gem_destroy(obj, vgem_obj->table); @@ -76,11 +77,15 @@ static int vgem_gem_fault(struct vm_fault *vmf) if (page_offset > num_pages) return VM_FAULT_SIGBUS; + ret = -ENOENT; + mutex_lock(&obj->pages_lock); if (obj->pages) { get_page(obj->pages[page_offset]); vmf->page = obj->pages[page_offset]; ret = 0; - } else { + } + mutex_unlock(&obj->pages_lock); + if (ret) { struct page *page; page = shmem_read_mapping_page( @@ -161,6 +166,8 @@ static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev, return ERR_PTR(ret); } + mutex_init(&obj->pages_lock); + return obj; } @@ -274,37 +281,66 @@ static const struct file_operations vgem_driver_fops = { .release = drm_release, }; +static struct page **vgem_pin_pages(struct drm_vgem_gem_object *bo) +{ + mutex_lock(&bo->pages_lock); + if (bo->pages_pin_count++ == 0) { + struct page **pages; + + pages = drm_gem_get_pages(&bo->base); + if (IS_ERR(pages)) { + bo->pages_pin_count--; + mutex_unlock(&bo->pages_lock); + return pages; + } + + bo->pages = pages; + } + mutex_unlock(&bo->pages_lock); + + return bo->pages; +} + +static void vgem_unpin_pages(struct drm_vgem_gem_object *bo) +{ + mutex_lock(&bo->pages_lock); + if (--bo->pages_pin_count == 0) { + drm_gem_put_pages(&bo->base, bo->pages, true, true); + bo->pages = NULL; + } + mutex_unlock(&bo->pages_lock); +} + static int vgem_prime_pin(struct drm_gem_object *obj) { + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); long n_pages = obj->size >> PAGE_SHIFT; struct page **pages; - /* Flush the object from the CPU cache so that importers can rely - * on coherent indirect access via the exported dma-address. - */ - pages = drm_gem_get_pages(obj); + pages = vgem_pin_pages(bo); if (IS_ERR(pages)) return PTR_ERR(pages); + /* Flush the object from the CPU cache so that importers can rely + * on coherent indirect access via the exported dma-address. + */ drm_clflush_pages(pages, n_pages); - drm_gem_put_pages(obj, pages, true, false); return 0; } -static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj) +static void vgem_prime_unpin(struct drm_gem_object *obj) { - struct sg_table *st; - struct page **pages; + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); - pages = drm_gem_get_pages(obj); - if (IS_ERR(pages)) - return ERR_CAST(pages); + vgem_unpin_pages(bo); +} - st = drm_prime_pages_to_sg(pages, obj->size >> PAGE_SHIFT); - drm_gem_put_pages(obj, pages, false, false); +static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj) +{ + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); - return st; + return drm_prime_pages_to_sg(bo->pages, bo->base.size >> PAGE_SHIFT); } static struct drm_gem_object* vgem_prime_import(struct drm_device *dev, @@ -333,6 +369,8 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev, __vgem_gem_destroy(obj); return ERR_PTR(-ENOMEM); } + + obj->pages_pin_count++; /* perma-pinned */ drm_prime_sg_to_page_addr_arrays(obj->table, obj->pages, NULL, npages); return &obj->base; @@ -340,23 +378,23 @@ static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev, static void *vgem_prime_vmap(struct drm_gem_object *obj) { + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); long n_pages = obj->size >> PAGE_SHIFT; struct page **pages; - void *addr; - pages = drm_gem_get_pages(obj); + pages = vgem_pin_pages(bo); if (IS_ERR(pages)) return NULL; - addr = vmap(pages, n_pages, 0, pgprot_writecombine(PAGE_KERNEL)); - drm_gem_put_pages(obj, pages, false, false); - - return addr; + return vmap(pages, n_pages, 0, pgprot_writecombine(PAGE_KERNEL)); } static void vgem_prime_vunmap(struct drm_gem_object *obj, void *vaddr) { + struct drm_vgem_gem_object *bo = to_vgem_bo(obj); + vunmap(vaddr); + vgem_unpin_pages(bo); } static int vgem_prime_mmap(struct drm_gem_object *obj, @@ -409,6 +447,7 @@ static struct drm_driver vgem_driver = { .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_pin = vgem_prime_pin, + .gem_prime_unpin = vgem_prime_unpin, .gem_prime_import = vgem_prime_import, .gem_prime_export = drm_gem_prime_export, .gem_prime_import_sg_table = vgem_prime_import_sg_table, diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h index 1aae01419112..5c8f6d619ff3 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.h +++ b/drivers/gpu/drm/vgem/vgem_drv.h @@ -43,7 +43,11 @@ struct vgem_file { #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base) struct drm_vgem_gem_object { struct drm_gem_object base; + struct page **pages; + unsigned int pages_pin_count; + struct mutex pages_lock; + struct sg_table *table; };
When the caller maps their dmabuf and we return an sg_table, the caller doesn't expect the pages beneath that sg_table to vanish on a whim (i.e. under mempressure). The contract is that the pages are pinned for the duration of the mapping (from dma_buf_map_attachment() to dma_buf_unmap_attachment). To comply, we need to introduce our own vgem_object.pages_pin_count and elevate it across the mapping. However, the drm_prime interface we use calls drv->prime_pin on dma_buf_attach and drv->prime_unpin on dma_buf_detach, which while that does cover the mapping is much broader than is desired -- but it will do for now. v2: also hold the pin across prime_vmap/vunmap Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com> Testcase: igt/gem_concurrent_blit/*swap*vgem* Fixes: 5ba6c9ff961a ("drm/vgem: Fix mmaping") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com> Cc: Laura Abbott <labbott@redhat.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: <stable@vger.kernel.org> --- drivers/gpu/drm/vgem/vgem_drv.c | 81 ++++++++++++++++++++++++++++++----------- drivers/gpu/drm/vgem/vgem_drv.h | 4 ++ 2 files changed, 64 insertions(+), 21 deletions(-)