From 6ada1328642b8ffc25917c89569d3e16354b43d2 Mon Sep 17 00:00:00 2001 From: Souptick Joarder Date: Tue, 22 May 2018 23:43:57 +0530 Subject: gpu: drm: omapdrm: Adding new typedef vm_fault_t Use new return type vm_fault_t for fault handler. For now, this is just documenting that the function returns a VM_FAULT value rather than an errno. Once all instances are converted, vm_fault_t will become a distinct type. Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t") Previously vm_insert_mixed() returns err which driver mapped into VM_FAULT_* type. Also return value of vm_insert_mixed() not handled correctly and 0 was returned inside fault_2d() as default. The new function vmf_insert_mixed() will replace this inefficiency by returning correct VM_FAULT_* type. vmf_error() is the newly introduce inline function in 4.17-rc6. Signed-off-by: Souptick Joarder Reviewed-by: Matthew Wilcox Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_gem.c | 51 +++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 28 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 17a53d207978..6030de7ec2ba 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -371,7 +371,7 @@ size_t omap_gem_mmap_size(struct drm_gem_object *obj) */ /* Normal handling for the case of faulting in non-tiled buffers */ -static int fault_1d(struct drm_gem_object *obj, +static vm_fault_t fault_1d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -392,11 +392,12 @@ static int fault_1d(struct drm_gem_object *obj, VERB("Inserting %p pfn %lx, pa %lx", (void *)vmf->address, pfn, pfn << PAGE_SHIFT); - return vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV)); + return vmf_insert_mixed(vma, vmf->address, + __pfn_to_pfn_t(pfn, PFN_DEV)); } /* Special handling for the case of faulting in 2d tiled buffers */ -static int fault_2d(struct drm_gem_object *obj, +static vm_fault_t fault_2d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -407,7 +408,8 @@ static int fault_2d(struct drm_gem_object *obj, unsigned long pfn; pgoff_t pgoff, base_pgoff; unsigned long vaddr; - int i, ret, slots; + int i, err, slots; + vm_fault_t ret = VM_FAULT_NOPAGE; /* * Note the height of the slot is also equal to the number of pages @@ -473,9 +475,10 @@ static int fault_2d(struct drm_gem_object *obj, memset(pages + slots, 0, sizeof(struct page *) * (n - slots)); - ret = tiler_pin(entry->block, pages, ARRAY_SIZE(pages), 0, true); - if (ret) { - dev_err(obj->dev->dev, "failed to pin: %d\n", ret); + err = tiler_pin(entry->block, pages, ARRAY_SIZE(pages), 0, true); + if (err) { + ret = vmf_error(err); + dev_err(obj->dev->dev, "failed to pin: %d\n", err); return ret; } @@ -485,7 +488,10 @@ static int fault_2d(struct drm_gem_object *obj, pfn, pfn << PAGE_SHIFT); for (i = n; i > 0; i--) { - vm_insert_mixed(vma, vaddr, __pfn_to_pfn_t(pfn, PFN_DEV)); + ret = vmf_insert_mixed(vma, + vaddr, __pfn_to_pfn_t(pfn, PFN_DEV)); + if (ret & VM_FAULT_ERROR) + break; pfn += priv->usergart[fmt].stride_pfn; vaddr += PAGE_SIZE * m; } @@ -494,7 +500,7 @@ static int fault_2d(struct drm_gem_object *obj, priv->usergart[fmt].last = (priv->usergart[fmt].last + 1) % NUM_USERGART_ENTRIES; - return 0; + return ret; } /** @@ -509,14 +515,15 @@ static int fault_2d(struct drm_gem_object *obj, * vma->vm_private_data points to the GEM object that is backing this * mapping. */ -int omap_gem_fault(struct vm_fault *vmf) +vm_fault_t omap_gem_fault(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; struct drm_gem_object *obj = vma->vm_private_data; struct omap_gem_object *omap_obj = to_omap_bo(obj); struct drm_device *dev = obj->dev; struct page **pages; - int ret; + int err; + vm_fault_t ret; /* Make sure we don't parallel update on a fault, nor move or remove * something from beneath our feet @@ -524,9 +531,11 @@ int omap_gem_fault(struct vm_fault *vmf) mutex_lock(&dev->struct_mutex); /* if a shmem backed object, make sure we have pages attached now */ - ret = get_pages(obj, &pages); - if (ret) + err = get_pages(obj, &pages); + if (err) { + ret = vmf_error(err); goto fail; + } /* where should we do corresponding put_pages().. we are mapping * the original page, rather than thru a GART, so we can't rely @@ -542,21 +551,7 @@ int omap_gem_fault(struct vm_fault *vmf) fail: mutex_unlock(&dev->struct_mutex); - switch (ret) { - case 0: - case -ERESTARTSYS: - case -EINTR: - case -EBUSY: - /* - * EBUSY is ok: this just means that another thread - * already did the job. - */ - return VM_FAULT_NOPAGE; - case -ENOMEM: - return VM_FAULT_OOM; - default: - return VM_FAULT_SIGBUS; - } + return ret; } /** We override mainly to fix up some of the vm mapping flags.. */ -- cgit v1.2.3 From 620063e10ed48c63027c4f59dab97d2ead67f9f1 Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Fri, 25 May 2018 19:39:20 +0300 Subject: drm/omap: gem: Rename GEM function with omap_gem_* prefix get_pages() as a local function name is too generic and easily confused for a generic MM kernel function. Rename it to __omap_gem_get_pages(). Rename the is_contiguous(), is_cache_coherent(), evict(), evict_entry(), fault_1d() and fault_2d() functions for the same reason. Signed-off-by: Laurent Pinchart Reviewed-by: Tomi Valkeinen Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_gem.c | 48 +++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 22 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 6030de7ec2ba..7a4ee4edab5b 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -156,7 +156,7 @@ static u64 mmap_offset(struct drm_gem_object *obj) return drm_vma_node_offset_addr(&obj->vma_node); } -static bool is_contiguous(struct omap_gem_object *omap_obj) +static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj) { if (omap_obj->flags & OMAP_BO_MEM_DMA_API) return true; @@ -171,7 +171,7 @@ static bool is_contiguous(struct omap_gem_object *omap_obj) * Eviction */ -static void evict_entry(struct drm_gem_object *obj, +static void omap_gem_evict_entry(struct drm_gem_object *obj, enum tiler_fmt fmt, struct omap_drm_usergart_entry *entry) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -199,7 +199,7 @@ static void evict_entry(struct drm_gem_object *obj, } /* Evict a buffer from usergart, if it is mapped there */ -static void evict(struct drm_gem_object *obj) +static void omap_gem_evict(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); struct omap_drm_private *priv = obj->dev->dev_private; @@ -213,7 +213,7 @@ static void evict(struct drm_gem_object *obj) &priv->usergart[fmt].entry[i]; if (entry->obj == obj) - evict_entry(obj, fmt, entry); + omap_gem_evict_entry(obj, fmt, entry); } } } @@ -291,7 +291,8 @@ free_pages: /* acquire pages when needed (for example, for DMA where physically * contiguous buffer is not required */ -static int get_pages(struct drm_gem_object *obj, struct page ***pages) +static int __omap_gem_get_pages(struct drm_gem_object *obj, + struct page ***pages) { struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret = 0; @@ -371,7 +372,7 @@ size_t omap_gem_mmap_size(struct drm_gem_object *obj) */ /* Normal handling for the case of faulting in non-tiled buffers */ -static vm_fault_t fault_1d(struct drm_gem_object *obj, +static vm_fault_t omap_gem_fault_1d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -385,7 +386,7 @@ static vm_fault_t fault_1d(struct drm_gem_object *obj, omap_gem_cpu_sync_page(obj, pgoff); pfn = page_to_pfn(omap_obj->pages[pgoff]); } else { - BUG_ON(!is_contiguous(omap_obj)); + BUG_ON(!omap_gem_is_contiguous(omap_obj)); pfn = (omap_obj->dma_addr >> PAGE_SHIFT) + pgoff; } @@ -397,7 +398,7 @@ static vm_fault_t fault_1d(struct drm_gem_object *obj, } /* Special handling for the case of faulting in 2d tiled buffers */ -static vm_fault_t fault_2d(struct drm_gem_object *obj, +static vm_fault_t omap_gem_fault_2d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -445,7 +446,7 @@ static vm_fault_t fault_2d(struct drm_gem_object *obj, /* evict previous buffer using this usergart entry, if any: */ if (entry->obj) - evict_entry(entry->obj, fmt, entry); + omap_gem_evict_entry(entry->obj, fmt, entry); entry->obj = obj; entry->obj_pgoff = base_pgoff; @@ -531,7 +532,7 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) mutex_lock(&dev->struct_mutex); /* if a shmem backed object, make sure we have pages attached now */ - err = get_pages(obj, &pages); + err = __omap_gem_get_pages(obj, &pages); if (err) { ret = vmf_error(err); goto fail; @@ -544,9 +545,9 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) */ if (omap_obj->flags & OMAP_BO_TILED) - ret = fault_2d(obj, vma, vmf); + ret = omap_gem_fault_2d(obj, vma, vmf); else - ret = fault_1d(obj, vma, vmf); + ret = omap_gem_fault_1d(obj, vma, vmf); fail: @@ -689,7 +690,8 @@ int omap_gem_roll(struct drm_gem_object *obj, u32 roll) /* if we aren't mapped yet, we don't need to do anything */ if (omap_obj->block) { struct page **pages; - ret = get_pages(obj, &pages); + + ret = __omap_gem_get_pages(obj, &pages); if (ret) goto fail; ret = tiler_pin(omap_obj->block, pages, npages, roll, true); @@ -717,7 +719,7 @@ fail: * the omap_obj->dma_addrs[i] is set to the DMA address, and the page is * unmapped from the CPU. */ -static inline bool is_cached_coherent(struct drm_gem_object *obj) +static inline bool omap_gem_is_cached_coherent(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -733,7 +735,7 @@ void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff) struct drm_device *dev = obj->dev; struct omap_gem_object *omap_obj = to_omap_bo(obj); - if (is_cached_coherent(obj)) + if (omap_gem_is_cached_coherent(obj)) return; if (omap_obj->dma_addrs[pgoff]) { @@ -753,7 +755,7 @@ void omap_gem_dma_sync_buffer(struct drm_gem_object *obj, struct page **pages = omap_obj->pages; bool dirty = false; - if (is_cached_coherent(obj)) + if (omap_gem_is_cached_coherent(obj)) return; for (i = 0; i < npages; i++) { @@ -801,7 +803,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) mutex_lock(&obj->dev->struct_mutex); - if (!is_contiguous(omap_obj) && priv->has_dmm) { + if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) { if (omap_obj->dma_addr_cnt == 0) { struct page **pages; u32 npages = obj->size >> PAGE_SHIFT; @@ -810,7 +812,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) BUG_ON(omap_obj->block); - ret = get_pages(obj, &pages); + ret = __omap_gem_get_pages(obj, &pages); if (ret) goto fail; @@ -848,7 +850,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) omap_obj->dma_addr_cnt++; *dma_addr = omap_obj->dma_addr; - } else if (is_contiguous(omap_obj)) { + } else if (omap_gem_is_contiguous(omap_obj)) { *dma_addr = omap_obj->dma_addr; } else { ret = -EINVAL; @@ -948,7 +950,7 @@ int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages, return 0; } mutex_lock(&obj->dev->struct_mutex); - ret = get_pages(obj, pages); + ret = __omap_gem_get_pages(obj, pages); mutex_unlock(&obj->dev->struct_mutex); return ret; } @@ -974,7 +976,9 @@ void *omap_gem_vaddr(struct drm_gem_object *obj) WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); if (!omap_obj->vaddr) { struct page **pages; - int ret = get_pages(obj, &pages); + int ret; + + ret = __omap_gem_get_pages(obj, &pages); if (ret) return ERR_PTR(ret); omap_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT, @@ -1076,7 +1080,7 @@ void omap_gem_free_object(struct drm_gem_object *obj) struct omap_drm_private *priv = dev->dev_private; struct omap_gem_object *omap_obj = to_omap_bo(obj); - evict(obj); + omap_gem_evict(obj); WARN_ON(!mutex_is_locked(&dev->struct_mutex)); -- cgit v1.2.3 From 2491244d7709d4e35f61d75ed3f6b4ea31b0a6f3 Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Fri, 25 May 2018 19:39:21 +0300 Subject: drm/omap: gem: Merge __omap_gem_get_pages() and omap_gem_attach_pages() The __omap_gem_get_pages() function is a wrapper around omap_gem_attach_pages() that returns the omap_obj->pages pointer through a function argument. Some callers don't need the pages pointer, and all of them can access omap_obj->pages directly. To simplify the code merge the __omap_gem_get_pages() wrapper with omap_gem_attach_pages() and update the callers accordingly. Signed-off-by: Laurent Pinchart Reviewed-by: Tomi Valkeinen Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_gem.c | 59 +++++++++++++------------------------- 1 file changed, 20 insertions(+), 39 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 7a4ee4edab5b..a3efac4abd4b 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -222,7 +222,7 @@ static void omap_gem_evict(struct drm_gem_object *obj) * Page Management */ -/** ensure backing pages are allocated */ +/* Ensure backing pages are allocated. */ static int omap_gem_attach_pages(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; @@ -232,7 +232,12 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) int i, ret; dma_addr_t *addrs; - WARN_ON(omap_obj->pages); + /* + * If not using shmem (in which case backing pages don't need to be + * allocated) or if pages are already allocated we're done. + */ + if (!(omap_obj->flags & OMAP_BO_MEM_SHMEM) || omap_obj->pages) + return 0; pages = drm_gem_get_pages(obj); if (IS_ERR(pages)) { @@ -288,29 +293,6 @@ free_pages: return ret; } -/* acquire pages when needed (for example, for DMA where physically - * contiguous buffer is not required - */ -static int __omap_gem_get_pages(struct drm_gem_object *obj, - struct page ***pages) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret = 0; - - if ((omap_obj->flags & OMAP_BO_MEM_SHMEM) && !omap_obj->pages) { - ret = omap_gem_attach_pages(obj); - if (ret) { - dev_err(obj->dev->dev, "could not attach pages\n"); - return ret; - } - } - - /* TODO: even phys-contig.. we should have a list of pages? */ - *pages = omap_obj->pages; - - return 0; -} - /** release backing pages */ static void omap_gem_detach_pages(struct drm_gem_object *obj) { @@ -522,7 +504,6 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) struct drm_gem_object *obj = vma->vm_private_data; struct omap_gem_object *omap_obj = to_omap_bo(obj); struct drm_device *dev = obj->dev; - struct page **pages; int err; vm_fault_t ret; @@ -532,7 +513,7 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) mutex_lock(&dev->struct_mutex); /* if a shmem backed object, make sure we have pages attached now */ - err = __omap_gem_get_pages(obj, &pages); + err = omap_gem_attach_pages(obj); if (err) { ret = vmf_error(err); goto fail; @@ -689,12 +670,12 @@ int omap_gem_roll(struct drm_gem_object *obj, u32 roll) /* if we aren't mapped yet, we don't need to do anything */ if (omap_obj->block) { - struct page **pages; - - ret = __omap_gem_get_pages(obj, &pages); + ret = omap_gem_attach_pages(obj); if (ret) goto fail; - ret = tiler_pin(omap_obj->block, pages, npages, roll, true); + + ret = tiler_pin(omap_obj->block, omap_obj->pages, npages, + roll, true); if (ret) dev_err(obj->dev->dev, "could not repin: %d\n", ret); } @@ -805,14 +786,13 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) { if (omap_obj->dma_addr_cnt == 0) { - struct page **pages; u32 npages = obj->size >> PAGE_SHIFT; enum tiler_fmt fmt = gem2fmt(omap_obj->flags); struct tiler_block *block; BUG_ON(omap_obj->block); - ret = __omap_gem_get_pages(obj, &pages); + ret = omap_gem_attach_pages(obj); if (ret) goto fail; @@ -832,7 +812,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) } /* TODO: enable async refill.. */ - ret = tiler_pin(block, pages, npages, + ret = tiler_pin(block, omap_obj->pages, npages, omap_obj->roll, true); if (ret) { tiler_release(block); @@ -941,16 +921,18 @@ int omap_gem_tiled_stride(struct drm_gem_object *obj, u32 orient) int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages, bool remap) { + struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret; + if (!remap) { - struct omap_gem_object *omap_obj = to_omap_bo(obj); if (!omap_obj->pages) return -ENOMEM; *pages = omap_obj->pages; return 0; } mutex_lock(&obj->dev->struct_mutex); - ret = __omap_gem_get_pages(obj, pages); + ret = omap_gem_attach_pages(obj); + *pages = omap_obj->pages; mutex_unlock(&obj->dev->struct_mutex); return ret; } @@ -975,13 +957,12 @@ void *omap_gem_vaddr(struct drm_gem_object *obj) struct omap_gem_object *omap_obj = to_omap_bo(obj); WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); if (!omap_obj->vaddr) { - struct page **pages; int ret; - ret = __omap_gem_get_pages(obj, &pages); + ret = omap_gem_attach_pages(obj); if (ret) return ERR_PTR(ret); - omap_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT, + omap_obj->vaddr = vmap(omap_obj->pages, obj->size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); } return omap_obj->vaddr; -- cgit v1.2.3 From dc8c9aeee5098688c1085691213fb9a703bf20ad Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Fri, 25 May 2018 19:39:22 +0300 Subject: drm/omap: gem: Don't take struct_mutex to get GEM object mmap offset GEM objects mmap offsets are created by calling drm_gem_create_mmap_offset_size() that doesn't need struct_mutex protection as it includes its own locking, based on a size that is static across the object's life time. Remove the unneeded struct_mutex locking. Signed-off-by: Laurent Pinchart Reviewed-by: Daniel Vetter Reviewed-by: Tomi Valkeinen Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_gem.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index a3efac4abd4b..623856d9b85a 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -137,14 +137,12 @@ struct omap_drm_usergart { */ /** get mmap offset */ -static u64 mmap_offset(struct drm_gem_object *obj) +u64 omap_gem_mmap_offset(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; int ret; size_t size; - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - /* Make it mmapable */ size = omap_gem_mmap_size(obj); ret = drm_gem_create_mmap_offset_size(obj, size); @@ -178,7 +176,7 @@ static void omap_gem_evict_entry(struct drm_gem_object *obj, struct omap_drm_private *priv = obj->dev->dev_private; int n = priv->usergart[fmt].height; size_t size = PAGE_SIZE * n; - loff_t off = mmap_offset(obj) + + loff_t off = omap_gem_mmap_offset(obj) + (entry->obj_pgoff << PAGE_SHIFT); const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE); @@ -319,16 +317,6 @@ u32 omap_gem_flags(struct drm_gem_object *obj) return to_omap_bo(obj)->flags; } -u64 omap_gem_mmap_offset(struct drm_gem_object *obj) -{ - u64 offset; - - mutex_lock(&obj->dev->struct_mutex); - offset = mmap_offset(obj); - mutex_unlock(&obj->dev->struct_mutex); - return offset; -} - /** get mmap size */ size_t omap_gem_mmap_size(struct drm_gem_object *obj) { -- cgit v1.2.3 From 3cbd0c587b129beaefb1405bbe43831e6bc9461e Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Sat, 26 May 2018 19:54:33 +0300 Subject: drm/omap: gem: Replace struct_mutex usage with omap_obj private lock The DRM device struct_mutex is used to protect against concurrent GEM object operations that deal with memory allocation and pinning. All those operations are local to a GEM object and don't need to be serialized across different GEM objects. Replace the struct_mutex with a local omap_obj.lock or drop it altogether where not needed. Signed-off-by: Laurent Pinchart Reviewed-by: Tomi Valkeinen Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_debugfs.c | 7 -- drivers/gpu/drm/omapdrm/omap_fbdev.c | 8 +-- drivers/gpu/drm/omapdrm/omap_gem.c | 127 ++++++++++++++++++++++----------- 3 files changed, 86 insertions(+), 56 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c index b42e286616b0..95ade441caa8 100644 --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c @@ -30,17 +30,10 @@ static int gem_show(struct seq_file *m, void *arg) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct omap_drm_private *priv = dev->dev_private; - int ret; - - ret = mutex_lock_interruptible(&dev->struct_mutex); - if (ret) - return ret; seq_printf(m, "All Objects:\n"); omap_gem_describe_objects(&priv->obj_list, m); - mutex_unlock(&dev->struct_mutex); - return 0; } diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 0f66c74a54b0..d958cc813a94 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -170,13 +170,11 @@ static int omap_fbdev_create(struct drm_fb_helper *helper, goto fail; } - mutex_lock(&dev->struct_mutex); - fbi = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(fbi)) { dev_err(dev->dev, "failed to allocate fb info\n"); ret = PTR_ERR(fbi); - goto fail_unlock; + goto fail; } DBG("fbi=%p, dev=%p", fbi, dev); @@ -212,12 +210,8 @@ static int omap_fbdev_create(struct drm_fb_helper *helper, DBG("par=%p, %dx%d", fbi->par, fbi->var.xres, fbi->var.yres); DBG("allocated %dx%d fb", fbdev->fb->width, fbdev->fb->height); - mutex_unlock(&dev->struct_mutex); - return 0; -fail_unlock: - mutex_unlock(&dev->struct_mutex); fail: if (ret) { diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 623856d9b85a..cebbdf081e5d 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -47,6 +47,9 @@ struct omap_gem_object { /** roll applied when mapping to DMM */ u32 roll; + /** protects dma_addr_cnt, block, pages, dma_addrs and vaddr */ + struct mutex lock; + /** * dma_addr contains the buffer DMA address. It is valid for * @@ -220,7 +223,10 @@ static void omap_gem_evict(struct drm_gem_object *obj) * Page Management */ -/* Ensure backing pages are allocated. */ +/* + * Ensure backing pages are allocated. Must be called with the omap_obj.lock + * held. + */ static int omap_gem_attach_pages(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; @@ -230,6 +236,8 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj) int i, ret; dma_addr_t *addrs; + lockdep_assert_held(&omap_obj->lock); + /* * If not using shmem (in which case backing pages don't need to be * allocated) or if pages are already allocated we're done. @@ -291,13 +299,15 @@ free_pages: return ret; } -/** release backing pages */ +/* Release backing pages. Must be called with the omap_obj.lock held. */ static void omap_gem_detach_pages(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); unsigned int npages = obj->size >> PAGE_SHIFT; unsigned int i; + lockdep_assert_held(&omap_obj->lock); + for (i = 0; i < npages; i++) { if (omap_obj->dma_addrs[i]) dma_unmap_page(obj->dev->dev, omap_obj->dma_addrs[i], @@ -491,14 +501,13 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; struct drm_gem_object *obj = vma->vm_private_data; struct omap_gem_object *omap_obj = to_omap_bo(obj); - struct drm_device *dev = obj->dev; int err; vm_fault_t ret; /* Make sure we don't parallel update on a fault, nor move or remove * something from beneath our feet */ - mutex_lock(&dev->struct_mutex); + mutex_lock(&omap_obj->lock); /* if a shmem backed object, make sure we have pages attached now */ err = omap_gem_attach_pages(obj); @@ -520,7 +529,7 @@ vm_fault_t omap_gem_fault(struct vm_fault *vmf) fail: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&omap_obj->lock); return ret; } @@ -654,7 +663,7 @@ int omap_gem_roll(struct drm_gem_object *obj, u32 roll) omap_obj->roll = roll; - mutex_lock(&obj->dev->struct_mutex); + mutex_lock(&omap_obj->lock); /* if we aren't mapped yet, we don't need to do anything */ if (omap_obj->block) { @@ -669,7 +678,7 @@ int omap_gem_roll(struct drm_gem_object *obj, u32 roll) } fail: - mutex_unlock(&obj->dev->struct_mutex); + mutex_unlock(&omap_obj->lock); return ret; } @@ -770,7 +779,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret = 0; - mutex_lock(&obj->dev->struct_mutex); + mutex_lock(&omap_obj->lock); if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) { if (omap_obj->dma_addr_cnt == 0) { @@ -826,7 +835,7 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) } fail: - mutex_unlock(&obj->dev->struct_mutex); + mutex_unlock(&omap_obj->lock); return ret; } @@ -844,7 +853,8 @@ void omap_gem_unpin(struct drm_gem_object *obj) struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret; - mutex_lock(&obj->dev->struct_mutex); + mutex_lock(&omap_obj->lock); + if (omap_obj->dma_addr_cnt > 0) { omap_obj->dma_addr_cnt--; if (omap_obj->dma_addr_cnt == 0) { @@ -863,7 +873,7 @@ void omap_gem_unpin(struct drm_gem_object *obj) } } - mutex_unlock(&obj->dev->struct_mutex); + mutex_unlock(&omap_obj->lock); } /* Get rotated scanout address (only valid if already pinned), at the @@ -876,13 +886,16 @@ int omap_gem_rotated_dma_addr(struct drm_gem_object *obj, u32 orient, struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret = -EINVAL; - mutex_lock(&obj->dev->struct_mutex); + mutex_lock(&omap_obj->lock); + if ((omap_obj->dma_addr_cnt > 0) && omap_obj->block && (omap_obj->flags & OMAP_BO_TILED)) { *dma_addr = tiler_tsptr(omap_obj->block, orient, x, y); ret = 0; } - mutex_unlock(&obj->dev->struct_mutex); + + mutex_unlock(&omap_obj->lock); + return ret; } @@ -910,18 +923,26 @@ int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages, bool remap) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret; + int ret = 0; - if (!remap) { - if (!omap_obj->pages) - return -ENOMEM; - *pages = omap_obj->pages; - return 0; + mutex_lock(&omap_obj->lock); + + if (remap) { + ret = omap_gem_attach_pages(obj); + if (ret) + goto unlock; } - mutex_lock(&obj->dev->struct_mutex); - ret = omap_gem_attach_pages(obj); + + if (!omap_obj->pages) { + ret = -ENOMEM; + goto unlock; + } + *pages = omap_obj->pages; - mutex_unlock(&obj->dev->struct_mutex); + +unlock: + mutex_unlock(&omap_obj->lock); + return ret; } @@ -936,24 +957,34 @@ int omap_gem_put_pages(struct drm_gem_object *obj) } #ifdef CONFIG_DRM_FBDEV_EMULATION -/* Get kernel virtual address for CPU access.. this more or less only - * exists for omap_fbdev. This should be called with struct_mutex - * held. +/* + * Get kernel virtual address for CPU access.. this more or less only + * exists for omap_fbdev. */ void *omap_gem_vaddr(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); - if (!omap_obj->vaddr) { - int ret; + void *vaddr; + int ret; + + mutex_lock(&omap_obj->lock); + if (!omap_obj->vaddr) { ret = omap_gem_attach_pages(obj); - if (ret) - return ERR_PTR(ret); + if (ret) { + vaddr = ERR_PTR(ret); + goto unlock; + } + omap_obj->vaddr = vmap(omap_obj->pages, obj->size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); } - return omap_obj->vaddr; + + vaddr = omap_obj->vaddr; + +unlock: + mutex_unlock(&omap_obj->lock); + return vaddr; } #endif @@ -1001,6 +1032,8 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m) off = drm_vma_node_start(&obj->vma_node); + mutex_lock(&omap_obj->lock); + seq_printf(m, "%08x: %2d (%2d) %08llx %pad (%2d) %p %4d", omap_obj->flags, obj->name, kref_read(&obj->refcount), off, &omap_obj->dma_addr, omap_obj->dma_addr_cnt, @@ -1018,6 +1051,8 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m) seq_printf(m, " %zu", obj->size); } + mutex_unlock(&omap_obj->lock); + seq_printf(m, "\n"); } @@ -1051,15 +1086,19 @@ void omap_gem_free_object(struct drm_gem_object *obj) omap_gem_evict(obj); - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - spin_lock(&priv->list_lock); list_del(&omap_obj->mm_list); spin_unlock(&priv->list_lock); - /* this means the object is still pinned.. which really should - * not happen. I think.. + /* + * We own the sole reference to the object at this point, but to keep + * lockdep happy, we must still take the omap_obj_lock to call + * omap_gem_detach_pages(). This should hardly make any difference as + * there can't be any lock contention. */ + mutex_lock(&omap_obj->lock); + + /* The object should not be pinned. */ WARN_ON(omap_obj->dma_addr_cnt > 0); if (omap_obj->pages) { @@ -1078,8 +1117,12 @@ void omap_gem_free_object(struct drm_gem_object *obj) drm_prime_gem_destroy(obj, omap_obj->sgt); } + mutex_unlock(&omap_obj->lock); + drm_gem_object_release(obj); + mutex_destroy(&omap_obj->lock); + kfree(omap_obj); } @@ -1135,6 +1178,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, obj = &omap_obj->base; omap_obj->flags = flags; + mutex_init(&omap_obj->lock); if (flags & OMAP_BO_TILED) { /* @@ -1199,16 +1243,15 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, if (sgt->orig_nents != 1 && !priv->has_dmm) return ERR_PTR(-EINVAL); - mutex_lock(&dev->struct_mutex); - gsize.bytes = PAGE_ALIGN(size); obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC); - if (!obj) { - obj = ERR_PTR(-ENOMEM); - goto done; - } + if (!obj) + return ERR_PTR(-ENOMEM); omap_obj = to_omap_bo(obj); + + mutex_lock(&omap_obj->lock); + omap_obj->sgt = sgt; if (sgt->orig_nents == 1) { @@ -1244,7 +1287,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, } done: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&omap_obj->lock); return obj; } -- cgit v1.2.3 From 5117bd898e8c0a31e8ab3a9b8523aecf0706e997 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Fri, 25 May 2018 19:39:24 +0300 Subject: drm/omap: gem: Fix mm_list locking - None of the list walkings where protected. - Switch to a mutex since the list walking at device resume time can sleep when pinning buffers through the tiler. Only thing we need to be careful with here is that while we walk the list we can't unreference any gem objects, since the final unref would result in a recursive deadlock. But the only functions that walk the list is the device resume and debugfs dumping, so all safe. Signed-off-by: Daniel Vetter Reviewed-by: Tomi Valkeinen Reviewed-by: Laurent Pinchart Signed-off-by: Laurent Pinchart Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_debugfs.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 +++++++++------ 4 files changed, 13 insertions(+), 8 deletions(-) (limited to 'drivers/gpu/drm/omapdrm/omap_gem.c') diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c index 95ade441caa8..91cf043f2b6b 100644 --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c @@ -32,7 +32,9 @@ static int gem_show(struct seq_file *m, void *arg) struct omap_drm_private *priv = dev->dev_private; seq_printf(m, "All Objects:\n"); + mutex_lock(&priv->list_lock); omap_gem_describe_objects(&priv->obj_list, m); + mutex_unlock(&priv->list_lock); return 0; } diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index ef3b0e3571ec..5fcf9eaf3eaf 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -540,7 +540,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) priv->omaprev = soc ? (unsigned int)soc->data : 0; priv->wq = alloc_ordered_workqueue("omapdrm", 0); - spin_lock_init(&priv->list_lock); + mutex_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list); /* Allocate and initialize the DRM device. */ diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 6eaee4df4559..f27c8e216adf 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -71,7 +71,7 @@ struct omap_drm_private { struct workqueue_struct *wq; /* lock for obj_list below */ - spinlock_t list_lock; + struct mutex list_lock; /* list of GEM objects: */ struct list_head obj_list; diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index cebbdf081e5d..4ba5d035c590 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1000,6 +1000,7 @@ int omap_gem_resume(struct drm_device *dev) struct omap_gem_object *omap_obj; int ret = 0; + mutex_lock(&priv->list_lock); list_for_each_entry(omap_obj, &priv->obj_list, mm_list) { if (omap_obj->block) { struct drm_gem_object *obj = &omap_obj->base; @@ -1011,12 +1012,14 @@ int omap_gem_resume(struct drm_device *dev) omap_obj->roll, true); if (ret) { dev_err(dev->dev, "could not repin: %d\n", ret); - return ret; + goto done; } } } - return 0; +done: + mutex_unlock(&priv->list_lock); + return ret; } #endif @@ -1086,9 +1089,9 @@ void omap_gem_free_object(struct drm_gem_object *obj) omap_gem_evict(obj); - spin_lock(&priv->list_lock); + mutex_lock(&priv->list_lock); list_del(&omap_obj->mm_list); - spin_unlock(&priv->list_lock); + mutex_unlock(&priv->list_lock); /* * We own the sole reference to the object at this point, but to keep @@ -1218,9 +1221,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, goto err_release; } - spin_lock(&priv->list_lock); + mutex_lock(&priv->list_lock); list_add(&omap_obj->mm_list, &priv->obj_list); - spin_unlock(&priv->list_lock); + mutex_unlock(&priv->list_lock); return obj; -- cgit v1.2.3