| Message ID | 1376652277-12193-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Fri, Aug 16, 2013 at 1:24 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > The primary purpose of this list is for pretty printing the maps through > debugfs/vma - per process information can be found in /proc/*/map et al. > However, the list manipulation eats processor cycles resulting in a near > indefinite (I got bored) stall for a leaky i-g-t/gem_concurrent_blit. > > There is one true user of the vmalist though. drm_vm_shm_close() uses > the list to count the number of drm_local_map references. For this, we > switch to using a counter inside drm_local_map. > > This patch kills drm_vm_open_locked and drm_vm_close_locked, and > redirects the one user outside of the core (exoynos) to use the now > semantically equivalent drm_gem_vm_open. I don't like the drm_vma_open/close() function names. We currently use the "drm_vma_*" prefix in drm_vma_manager.c. On the other hand, this file exclusively uses "drm_vma_node_*" and "drm_vma_offset_*" prefixes, so maybe we're fine here. Don't know.. I tried removing drm_vma_open/close entirely, but it seems not all drivers set vma->vma_file->private_data to the drm_device, hmm. So the patch looks fine: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Regards David > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Inki Dae <inki.dae@samsung.com> > --- > drivers/gpu/drm/drm_bufs.c | 1 + > drivers/gpu/drm/drm_drv.c | 8 ----- > drivers/gpu/drm/drm_gem.c | 16 +++------ > drivers/gpu/drm/drm_info.c | 38 --------------------- > drivers/gpu/drm/drm_stub.c | 1 - > drivers/gpu/drm/drm_vm.c | 58 +++++++-------------------------- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 +---- > include/drm/drmP.h | 7 ++-- > 8 files changed, 20 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > index bef4abf..b0aa2eb 100644 > --- a/drivers/gpu/drm/drm_bufs.c > +++ b/drivers/gpu/drm/drm_bufs.c > @@ -152,6 +152,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, > map->size = size; > map->flags = flags; > map->type = type; > + map->count = 0; > > /* Only allow shared memory to be removable since we only keep enough > * book keeping information about shared memory to allow for removal > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index dddd799..7ec9959 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -182,8 +182,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > */ > int drm_lastclose(struct drm_device * dev) > { > - struct drm_vma_entry *vma, *vma_temp; > - > DRM_DEBUG("\n"); > > if (dev->driver->lastclose) > @@ -203,12 +201,6 @@ int drm_lastclose(struct drm_device * dev) > dev->sg = NULL; > } > > - /* Clear vma list (only built for debugging) */ > - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { > - list_del(&vma->head); > - kfree(vma); > - } > - > if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) && > !drm_core_check_feature(dev, DRIVER_MODESET)) > drm_dma_takedown(dev); > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 9ab038c..84a5834 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -573,22 +573,16 @@ void drm_gem_vm_open(struct vm_area_struct *vma) > struct drm_gem_object *obj = vma->vm_private_data; > > drm_gem_object_reference(obj); > - > - mutex_lock(&obj->dev->struct_mutex); > - drm_vm_open_locked(obj->dev, vma); > - mutex_unlock(&obj->dev->struct_mutex); > + drm_vma_open(obj->dev, vma); > } > EXPORT_SYMBOL(drm_gem_vm_open); > > void drm_gem_vm_close(struct vm_area_struct *vma) > { > struct drm_gem_object *obj = vma->vm_private_data; > - struct drm_device *dev = obj->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_close_locked(obj->dev, vma); > - drm_gem_object_unreference(obj); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_close(obj->dev, vma); > + drm_gem_object_unreference_unlocked(obj); > } > EXPORT_SYMBOL(drm_gem_vm_close); > > @@ -639,9 +633,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > * (which should happen whether the vma was created by this call, or > * by a vm_open due to mremap or partial unmap or whatever). > */ > - drm_gem_object_reference(obj); > - > - drm_vm_open_locked(dev, vma); > + drm_gem_vm_open(vma); > return 0; > } > EXPORT_SYMBOL(drm_gem_mmap_obj); > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > index d4b20ce..6ea817c 100644 > --- a/drivers/gpu/drm/drm_info.c > +++ b/drivers/gpu/drm/drm_info.c > @@ -228,49 +228,11 @@ int drm_vma_info(struct seq_file *m, void *data) > { > struct drm_info_node *node = (struct drm_info_node *) m->private; > struct drm_device *dev = node->minor->dev; > - struct drm_vma_entry *pt; > - struct vm_area_struct *vma; > -#if defined(__i386__) > - unsigned int pgprot; > -#endif > > - mutex_lock(&dev->struct_mutex); > seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n", > atomic_read(&dev->vma_count), > high_memory, (void *)(unsigned long)virt_to_phys(high_memory)); > > - list_for_each_entry(pt, &dev->vmalist, head) { > - vma = pt->vma; > - if (!vma) > - continue; > - seq_printf(m, > - "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000", > - pt->pid, > - (void *)vma->vm_start, (void *)vma->vm_end, > - vma->vm_flags & VM_READ ? 'r' : '-', > - vma->vm_flags & VM_WRITE ? 'w' : '-', > - vma->vm_flags & VM_EXEC ? 'x' : '-', > - vma->vm_flags & VM_MAYSHARE ? 's' : 'p', > - vma->vm_flags & VM_LOCKED ? 'l' : '-', > - vma->vm_flags & VM_IO ? 'i' : '-', > - vma->vm_pgoff); > - > -#if defined(__i386__) > - pgprot = pgprot_val(vma->vm_page_prot); > - seq_printf(m, " %c%c%c%c%c%c%c%c%c", > - pgprot & _PAGE_PRESENT ? 'p' : '-', > - pgprot & _PAGE_RW ? 'w' : 'r', > - pgprot & _PAGE_USER ? 'u' : 's', > - pgprot & _PAGE_PWT ? 't' : 'b', > - pgprot & _PAGE_PCD ? 'u' : 'c', > - pgprot & _PAGE_ACCESSED ? 'a' : '-', > - pgprot & _PAGE_DIRTY ? 'd' : '-', > - pgprot & _PAGE_PSE ? 'm' : 'k', > - pgprot & _PAGE_GLOBAL ? 'g' : 'l'); > -#endif > - seq_printf(m, "\n"); > - } > - mutex_unlock(&dev->struct_mutex); > return 0; > } > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index d663f7d..0a1db60 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -258,7 +258,6 @@ int drm_fill_in_dev(struct drm_device *dev, > > INIT_LIST_HEAD(&dev->filelist); > INIT_LIST_HEAD(&dev->ctxlist); > - INIT_LIST_HEAD(&dev->vmalist); > INIT_LIST_HEAD(&dev->maplist); > INIT_LIST_HEAD(&dev->vblank_event_list); > > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c > index feb2003..bc2b26f 100644 > --- a/drivers/gpu/drm/drm_vm.c > +++ b/drivers/gpu/drm/drm_vm.c > @@ -213,7 +213,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > - struct drm_vma_entry *pt, *temp; > struct drm_local_map *map; > struct drm_map_list *r_list; > int found_maps = 0; > @@ -225,17 +224,8 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) > map = vma->vm_private_data; > > mutex_lock(&dev->struct_mutex); > - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { > - if (pt->vma->vm_private_data == map) > - found_maps++; > - if (pt->vma == vma) { > - list_del(&pt->head); > - kfree(pt); > - } > - } > - > /* We were the only map that was found */ > - if (found_maps == 1 && map->flags & _DRM_REMOVABLE) { > + if (--map->count == 0) { > /* Check to see if we are in the maplist, if we are not, then > * we delete this mappings information. > */ > @@ -402,68 +392,41 @@ static const struct vm_operations_struct drm_vm_sg_ops = { > * Create a new drm_vma_entry structure as the \p vma private data entry and > * add it to drm_device::vmalist. > */ > -void drm_vm_open_locked(struct drm_device *dev, > - struct vm_area_struct *vma) > +void drm_vma_open(struct drm_device *dev, > + struct vm_area_struct *vma) > { > - struct drm_vma_entry *vma_entry; > - > DRM_DEBUG("0x%08lx,0x%08lx\n", > vma->vm_start, vma->vm_end - vma->vm_start); > atomic_inc(&dev->vma_count); > - > - vma_entry = kmalloc(sizeof(*vma_entry), GFP_KERNEL); > - if (vma_entry) { > - vma_entry->vma = vma; > - vma_entry->pid = current->pid; > - list_add(&vma_entry->head, &dev->vmalist); > - } > } > -EXPORT_SYMBOL_GPL(drm_vm_open_locked); > > static void drm_vm_open(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_open_locked(dev, vma); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_open(dev, vma); > } > > -void drm_vm_close_locked(struct drm_device *dev, > - struct vm_area_struct *vma) > +void drm_vma_close(struct drm_device *dev, > + struct vm_area_struct *vma) > { > - struct drm_vma_entry *pt, *temp; > - > DRM_DEBUG("0x%08lx,0x%08lx\n", > vma->vm_start, vma->vm_end - vma->vm_start); > atomic_dec(&dev->vma_count); > - > - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { > - if (pt->vma == vma) { > - list_del(&pt->head); > - kfree(pt); > - break; > - } > - } > } > > /** > * \c close method for all virtual memory types. > * > * \param vma virtual memory area. > - * > - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and > - * free it. > */ > static void drm_vm_close(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_close_locked(dev, vma); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_close(dev, vma); > } > > /** > @@ -513,7 +476,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) > > vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; > > - drm_vm_open_locked(dev, vma); > + drm_vma_open(dev, vma); > return 0; > } > > @@ -543,7 +506,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) > { > struct drm_file *priv = filp->private_data; > struct drm_device *dev = priv->minor->dev; > - struct drm_local_map *map = NULL; > + struct drm_local_map *map; > resource_size_t offset = 0; > struct drm_hash_item *hash; > > @@ -648,8 +611,9 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) > return -EINVAL; /* This should never happen. */ > } > vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; > + map->count++; > > - drm_vm_open_locked(dev, vma); > + drm_vma_open(dev, vma); > return 0; > } > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index b904633..04cb3ea 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -401,14 +401,7 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, > return ret; > } > > - /* > - * take a reference to this mapping of the object. And this reference > - * is unreferenced by the corresponding vm_close call. > - */ > - drm_gem_object_reference(obj); > - > - drm_vm_open_locked(drm_dev, vma); > - > + drm_gem_vm_open(vma); > return 0; > } > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 3ecdde6..e4b8658 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -573,6 +573,7 @@ struct drm_local_map { > void *handle; /**< User-space: "Handle" to pass to mmap() */ > /**< Kernel-space: kernel-virtual address */ > int mtrr; /**< MTRR slot used */ > + int count; > }; > > typedef struct drm_local_map drm_local_map_t; > @@ -1116,8 +1117,6 @@ struct drm_device { > > struct idr ctx_idr; > > - struct list_head vmalist; /**< List of vmas (for debugging) */ > - > /*@} */ > > /** \name DMA support */ > @@ -1274,8 +1273,8 @@ extern int drm_release(struct inode *inode, struct file *filp); > /* Mapping support (drm_vm.h) */ > extern int drm_mmap(struct file *filp, struct vm_area_struct *vma); > extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma); > -extern void drm_vm_open_locked(struct drm_device *dev, struct vm_area_struct *vma); > -extern void drm_vm_close_locked(struct drm_device *dev, struct vm_area_struct *vma); > +extern void drm_vma_open(struct drm_device *dev, struct vm_area_struct *vma); > +extern void drm_vma_close(struct drm_device *dev, struct vm_area_struct *vma); > extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait); > > /* Memory management support (drm_memory.h) */ > -- > 1.8.4.rc2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Aug 16, 2013 at 12:24:37PM +0100, Chris Wilson wrote: > The primary purpose of this list is for pretty printing the maps through > debugfs/vma - per process information can be found in /proc/*/map et al. > However, the list manipulation eats processor cycles resulting in a near > indefinite (I got bored) stall for a leaky i-g-t/gem_concurrent_blit. > > There is one true user of the vmalist though. drm_vm_shm_close() uses > the list to count the number of drm_local_map references. For this, we > switch to using a counter inside drm_local_map. > > This patch kills drm_vm_open_locked and drm_vm_close_locked, and > redirects the one user outside of the core (exoynos) to use the now > semantically equivalent drm_gem_vm_open. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Inki Dae <inki.dae@samsung.com> Ok, I've accidentally read through the exynos mmap code and now I'm desparately trying to get a bunch of forks out of my eyes again. So just one question: Why is this stuff not using the gem mmap offset stuff, but still tries to reinvent the useless parts of that wheel (like drm_vm_open/close)? /rant Cheers, Daniel > --- > drivers/gpu/drm/drm_bufs.c | 1 + > drivers/gpu/drm/drm_drv.c | 8 ----- > drivers/gpu/drm/drm_gem.c | 16 +++------ > drivers/gpu/drm/drm_info.c | 38 --------------------- > drivers/gpu/drm/drm_stub.c | 1 - > drivers/gpu/drm/drm_vm.c | 58 +++++++-------------------------- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 +---- > include/drm/drmP.h | 7 ++-- > 8 files changed, 20 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > index bef4abf..b0aa2eb 100644 > --- a/drivers/gpu/drm/drm_bufs.c > +++ b/drivers/gpu/drm/drm_bufs.c > @@ -152,6 +152,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, > map->size = size; > map->flags = flags; > map->type = type; > + map->count = 0; > > /* Only allow shared memory to be removable since we only keep enough > * book keeping information about shared memory to allow for removal > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index dddd799..7ec9959 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -182,8 +182,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > */ > int drm_lastclose(struct drm_device * dev) > { > - struct drm_vma_entry *vma, *vma_temp; > - > DRM_DEBUG("\n"); > > if (dev->driver->lastclose) > @@ -203,12 +201,6 @@ int drm_lastclose(struct drm_device * dev) > dev->sg = NULL; > } > > - /* Clear vma list (only built for debugging) */ > - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { > - list_del(&vma->head); > - kfree(vma); > - } > - > if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) && > !drm_core_check_feature(dev, DRIVER_MODESET)) > drm_dma_takedown(dev); > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 9ab038c..84a5834 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -573,22 +573,16 @@ void drm_gem_vm_open(struct vm_area_struct *vma) > struct drm_gem_object *obj = vma->vm_private_data; > > drm_gem_object_reference(obj); > - > - mutex_lock(&obj->dev->struct_mutex); > - drm_vm_open_locked(obj->dev, vma); > - mutex_unlock(&obj->dev->struct_mutex); > + drm_vma_open(obj->dev, vma); > } > EXPORT_SYMBOL(drm_gem_vm_open); > > void drm_gem_vm_close(struct vm_area_struct *vma) > { > struct drm_gem_object *obj = vma->vm_private_data; > - struct drm_device *dev = obj->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_close_locked(obj->dev, vma); > - drm_gem_object_unreference(obj); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_close(obj->dev, vma); > + drm_gem_object_unreference_unlocked(obj); > } > EXPORT_SYMBOL(drm_gem_vm_close); > > @@ -639,9 +633,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > * (which should happen whether the vma was created by this call, or > * by a vm_open due to mremap or partial unmap or whatever). > */ > - drm_gem_object_reference(obj); > - > - drm_vm_open_locked(dev, vma); > + drm_gem_vm_open(vma); > return 0; > } > EXPORT_SYMBOL(drm_gem_mmap_obj); > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > index d4b20ce..6ea817c 100644 > --- a/drivers/gpu/drm/drm_info.c > +++ b/drivers/gpu/drm/drm_info.c > @@ -228,49 +228,11 @@ int drm_vma_info(struct seq_file *m, void *data) > { > struct drm_info_node *node = (struct drm_info_node *) m->private; > struct drm_device *dev = node->minor->dev; > - struct drm_vma_entry *pt; > - struct vm_area_struct *vma; > -#if defined(__i386__) > - unsigned int pgprot; > -#endif > > - mutex_lock(&dev->struct_mutex); > seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n", > atomic_read(&dev->vma_count), > high_memory, (void *)(unsigned long)virt_to_phys(high_memory)); > > - list_for_each_entry(pt, &dev->vmalist, head) { > - vma = pt->vma; > - if (!vma) > - continue; > - seq_printf(m, > - "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000", > - pt->pid, > - (void *)vma->vm_start, (void *)vma->vm_end, > - vma->vm_flags & VM_READ ? 'r' : '-', > - vma->vm_flags & VM_WRITE ? 'w' : '-', > - vma->vm_flags & VM_EXEC ? 'x' : '-', > - vma->vm_flags & VM_MAYSHARE ? 's' : 'p', > - vma->vm_flags & VM_LOCKED ? 'l' : '-', > - vma->vm_flags & VM_IO ? 'i' : '-', > - vma->vm_pgoff); > - > -#if defined(__i386__) > - pgprot = pgprot_val(vma->vm_page_prot); > - seq_printf(m, " %c%c%c%c%c%c%c%c%c", > - pgprot & _PAGE_PRESENT ? 'p' : '-', > - pgprot & _PAGE_RW ? 'w' : 'r', > - pgprot & _PAGE_USER ? 'u' : 's', > - pgprot & _PAGE_PWT ? 't' : 'b', > - pgprot & _PAGE_PCD ? 'u' : 'c', > - pgprot & _PAGE_ACCESSED ? 'a' : '-', > - pgprot & _PAGE_DIRTY ? 'd' : '-', > - pgprot & _PAGE_PSE ? 'm' : 'k', > - pgprot & _PAGE_GLOBAL ? 'g' : 'l'); > -#endif > - seq_printf(m, "\n"); > - } > - mutex_unlock(&dev->struct_mutex); > return 0; > } > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index d663f7d..0a1db60 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -258,7 +258,6 @@ int drm_fill_in_dev(struct drm_device *dev, > > INIT_LIST_HEAD(&dev->filelist); > INIT_LIST_HEAD(&dev->ctxlist); > - INIT_LIST_HEAD(&dev->vmalist); > INIT_LIST_HEAD(&dev->maplist); > INIT_LIST_HEAD(&dev->vblank_event_list); > > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c > index feb2003..bc2b26f 100644 > --- a/drivers/gpu/drm/drm_vm.c > +++ b/drivers/gpu/drm/drm_vm.c > @@ -213,7 +213,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > - struct drm_vma_entry *pt, *temp; > struct drm_local_map *map; > struct drm_map_list *r_list; > int found_maps = 0; > @@ -225,17 +224,8 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) > map = vma->vm_private_data; > > mutex_lock(&dev->struct_mutex); > - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { > - if (pt->vma->vm_private_data == map) > - found_maps++; > - if (pt->vma == vma) { > - list_del(&pt->head); > - kfree(pt); > - } > - } > - > /* We were the only map that was found */ > - if (found_maps == 1 && map->flags & _DRM_REMOVABLE) { > + if (--map->count == 0) { > /* Check to see if we are in the maplist, if we are not, then > * we delete this mappings information. > */ > @@ -402,68 +392,41 @@ static const struct vm_operations_struct drm_vm_sg_ops = { > * Create a new drm_vma_entry structure as the \p vma private data entry and > * add it to drm_device::vmalist. > */ > -void drm_vm_open_locked(struct drm_device *dev, > - struct vm_area_struct *vma) > +void drm_vma_open(struct drm_device *dev, > + struct vm_area_struct *vma) > { > - struct drm_vma_entry *vma_entry; > - > DRM_DEBUG("0x%08lx,0x%08lx\n", > vma->vm_start, vma->vm_end - vma->vm_start); > atomic_inc(&dev->vma_count); > - > - vma_entry = kmalloc(sizeof(*vma_entry), GFP_KERNEL); > - if (vma_entry) { > - vma_entry->vma = vma; > - vma_entry->pid = current->pid; > - list_add(&vma_entry->head, &dev->vmalist); > - } > } > -EXPORT_SYMBOL_GPL(drm_vm_open_locked); > > static void drm_vm_open(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_open_locked(dev, vma); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_open(dev, vma); > } > > -void drm_vm_close_locked(struct drm_device *dev, > - struct vm_area_struct *vma) > +void drm_vma_close(struct drm_device *dev, > + struct vm_area_struct *vma) > { > - struct drm_vma_entry *pt, *temp; > - > DRM_DEBUG("0x%08lx,0x%08lx\n", > vma->vm_start, vma->vm_end - vma->vm_start); > atomic_dec(&dev->vma_count); > - > - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { > - if (pt->vma == vma) { > - list_del(&pt->head); > - kfree(pt); > - break; > - } > - } > } > > /** > * \c close method for all virtual memory types. > * > * \param vma virtual memory area. > - * > - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and > - * free it. > */ > static void drm_vm_close(struct vm_area_struct *vma) > { > struct drm_file *priv = vma->vm_file->private_data; > struct drm_device *dev = priv->minor->dev; > > - mutex_lock(&dev->struct_mutex); > - drm_vm_close_locked(dev, vma); > - mutex_unlock(&dev->struct_mutex); > + drm_vma_close(dev, vma); > } > > /** > @@ -513,7 +476,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) > > vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; > > - drm_vm_open_locked(dev, vma); > + drm_vma_open(dev, vma); > return 0; > } > > @@ -543,7 +506,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) > { > struct drm_file *priv = filp->private_data; > struct drm_device *dev = priv->minor->dev; > - struct drm_local_map *map = NULL; > + struct drm_local_map *map; > resource_size_t offset = 0; > struct drm_hash_item *hash; > > @@ -648,8 +611,9 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) > return -EINVAL; /* This should never happen. */ > } > vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; > + map->count++; > > - drm_vm_open_locked(dev, vma); > + drm_vma_open(dev, vma); > return 0; > } > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index b904633..04cb3ea 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -401,14 +401,7 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, > return ret; > } > > - /* > - * take a reference to this mapping of the object. And this reference > - * is unreferenced by the corresponding vm_close call. > - */ > - drm_gem_object_reference(obj); > - > - drm_vm_open_locked(drm_dev, vma); > - > + drm_gem_vm_open(vma); > return 0; > } > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 3ecdde6..e4b8658 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -573,6 +573,7 @@ struct drm_local_map { > void *handle; /**< User-space: "Handle" to pass to mmap() */ > /**< Kernel-space: kernel-virtual address */ > int mtrr; /**< MTRR slot used */ > + int count; > }; > > typedef struct drm_local_map drm_local_map_t; > @@ -1116,8 +1117,6 @@ struct drm_device { > > struct idr ctx_idr; > > - struct list_head vmalist; /**< List of vmas (for debugging) */ > - > /*@} */ > > /** \name DMA support */ > @@ -1274,8 +1273,8 @@ extern int drm_release(struct inode *inode, struct file *filp); > /* Mapping support (drm_vm.h) */ > extern int drm_mmap(struct file *filp, struct vm_area_struct *vma); > extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma); > -extern void drm_vm_open_locked(struct drm_device *dev, struct vm_area_struct *vma); > -extern void drm_vm_close_locked(struct drm_device *dev, struct vm_area_struct *vma); > +extern void drm_vma_open(struct drm_device *dev, struct vm_area_struct *vma); > +extern void drm_vma_close(struct drm_device *dev, struct vm_area_struct *vma); > extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait); > > /* Memory management support (drm_memory.h) */ > -- > 1.8.4.rc2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index bef4abf..b0aa2eb 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -152,6 +152,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset, map->size = size; map->flags = flags; map->type = type; + map->count = 0; /* Only allow shared memory to be removable since we only keep enough * book keeping information about shared memory to allow for removal diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index dddd799..7ec9959 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -182,8 +182,6 @@ static const struct drm_ioctl_desc drm_ioctls[] = { */ int drm_lastclose(struct drm_device * dev) { - struct drm_vma_entry *vma, *vma_temp; - DRM_DEBUG("\n"); if (dev->driver->lastclose) @@ -203,12 +201,6 @@ int drm_lastclose(struct drm_device * dev) dev->sg = NULL; } - /* Clear vma list (only built for debugging) */ - list_for_each_entry_safe(vma, vma_temp, &dev->vmalist, head) { - list_del(&vma->head); - kfree(vma); - } - if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) && !drm_core_check_feature(dev, DRIVER_MODESET)) drm_dma_takedown(dev); diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9ab038c..84a5834 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -573,22 +573,16 @@ void drm_gem_vm_open(struct vm_area_struct *vma) struct drm_gem_object *obj = vma->vm_private_data; drm_gem_object_reference(obj); - - mutex_lock(&obj->dev->struct_mutex); - drm_vm_open_locked(obj->dev, vma); - mutex_unlock(&obj->dev->struct_mutex); + drm_vma_open(obj->dev, vma); } EXPORT_SYMBOL(drm_gem_vm_open); void drm_gem_vm_close(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; - struct drm_device *dev = obj->dev; - mutex_lock(&dev->struct_mutex); - drm_vm_close_locked(obj->dev, vma); - drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex); + drm_vma_close(obj->dev, vma); + drm_gem_object_unreference_unlocked(obj); } EXPORT_SYMBOL(drm_gem_vm_close); @@ -639,9 +633,7 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, * (which should happen whether the vma was created by this call, or * by a vm_open due to mremap or partial unmap or whatever). */ - drm_gem_object_reference(obj); - - drm_vm_open_locked(dev, vma); + drm_gem_vm_open(vma); return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index d4b20ce..6ea817c 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -228,49 +228,11 @@ int drm_vma_info(struct seq_file *m, void *data) { struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; - struct drm_vma_entry *pt; - struct vm_area_struct *vma; -#if defined(__i386__) - unsigned int pgprot; -#endif - mutex_lock(&dev->struct_mutex); seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n", atomic_read(&dev->vma_count), high_memory, (void *)(unsigned long)virt_to_phys(high_memory)); - list_for_each_entry(pt, &dev->vmalist, head) { - vma = pt->vma; - if (!vma) - continue; - seq_printf(m, - "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000", - pt->pid, - (void *)vma->vm_start, (void *)vma->vm_end, - vma->vm_flags & VM_READ ? 'r' : '-', - vma->vm_flags & VM_WRITE ? 'w' : '-', - vma->vm_flags & VM_EXEC ? 'x' : '-', - vma->vm_flags & VM_MAYSHARE ? 's' : 'p', - vma->vm_flags & VM_LOCKED ? 'l' : '-', - vma->vm_flags & VM_IO ? 'i' : '-', - vma->vm_pgoff); - -#if defined(__i386__) - pgprot = pgprot_val(vma->vm_page_prot); - seq_printf(m, " %c%c%c%c%c%c%c%c%c", - pgprot & _PAGE_PRESENT ? 'p' : '-', - pgprot & _PAGE_RW ? 'w' : 'r', - pgprot & _PAGE_USER ? 'u' : 's', - pgprot & _PAGE_PWT ? 't' : 'b', - pgprot & _PAGE_PCD ? 'u' : 'c', - pgprot & _PAGE_ACCESSED ? 'a' : '-', - pgprot & _PAGE_DIRTY ? 'd' : '-', - pgprot & _PAGE_PSE ? 'm' : 'k', - pgprot & _PAGE_GLOBAL ? 'g' : 'l'); -#endif - seq_printf(m, "\n"); - } - mutex_unlock(&dev->struct_mutex); return 0; } diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d663f7d..0a1db60 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -258,7 +258,6 @@ int drm_fill_in_dev(struct drm_device *dev, INIT_LIST_HEAD(&dev->filelist); INIT_LIST_HEAD(&dev->ctxlist); - INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist); INIT_LIST_HEAD(&dev->vblank_event_list); diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index feb2003..bc2b26f 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -213,7 +213,6 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev; - struct drm_vma_entry *pt, *temp; struct drm_local_map *map; struct drm_map_list *r_list; int found_maps = 0; @@ -225,17 +224,8 @@ static void drm_vm_shm_close(struct vm_area_struct *vma) map = vma->vm_private_data; mutex_lock(&dev->struct_mutex); - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { - if (pt->vma->vm_private_data == map) - found_maps++; - if (pt->vma == vma) { - list_del(&pt->head); - kfree(pt); - } - } - /* We were the only map that was found */ - if (found_maps == 1 && map->flags & _DRM_REMOVABLE) { + if (--map->count == 0) { /* Check to see if we are in the maplist, if we are not, then * we delete this mappings information. */ @@ -402,68 +392,41 @@ static const struct vm_operations_struct drm_vm_sg_ops = { * Create a new drm_vma_entry structure as the \p vma private data entry and * add it to drm_device::vmalist. */ -void drm_vm_open_locked(struct drm_device *dev, - struct vm_area_struct *vma) +void drm_vma_open(struct drm_device *dev, + struct vm_area_struct *vma) { - struct drm_vma_entry *vma_entry; - DRM_DEBUG("0x%08lx,0x%08lx\n", vma->vm_start, vma->vm_end - vma->vm_start); atomic_inc(&dev->vma_count); - - vma_entry = kmalloc(sizeof(*vma_entry), GFP_KERNEL); - if (vma_entry) { - vma_entry->vma = vma; - vma_entry->pid = current->pid; - list_add(&vma_entry->head, &dev->vmalist); - } } -EXPORT_SYMBOL_GPL(drm_vm_open_locked); static void drm_vm_open(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev; - mutex_lock(&dev->struct_mutex); - drm_vm_open_locked(dev, vma); - mutex_unlock(&dev->struct_mutex); + drm_vma_open(dev, vma); } -void drm_vm_close_locked(struct drm_device *dev, - struct vm_area_struct *vma) +void drm_vma_close(struct drm_device *dev, + struct vm_area_struct *vma) { - struct drm_vma_entry *pt, *temp; - DRM_DEBUG("0x%08lx,0x%08lx\n", vma->vm_start, vma->vm_end - vma->vm_start); atomic_dec(&dev->vma_count); - - list_for_each_entry_safe(pt, temp, &dev->vmalist, head) { - if (pt->vma == vma) { - list_del(&pt->head); - kfree(pt); - break; - } - } } /** * \c close method for all virtual memory types. * * \param vma virtual memory area. - * - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and - * free it. */ static void drm_vm_close(struct vm_area_struct *vma) { struct drm_file *priv = vma->vm_file->private_data; struct drm_device *dev = priv->minor->dev; - mutex_lock(&dev->struct_mutex); - drm_vm_close_locked(dev, vma); - mutex_unlock(&dev->struct_mutex); + drm_vma_close(dev, vma); } /** @@ -513,7 +476,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; - drm_vm_open_locked(dev, vma); + drm_vma_open(dev, vma); return 0; } @@ -543,7 +506,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) { struct drm_file *priv = filp->private_data; struct drm_device *dev = priv->minor->dev; - struct drm_local_map *map = NULL; + struct drm_local_map *map; resource_size_t offset = 0; struct drm_hash_item *hash; @@ -648,8 +611,9 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) return -EINVAL; /* This should never happen. */ } vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; + map->count++; - drm_vm_open_locked(dev, vma); + drm_vma_open(dev, vma); return 0; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index b904633..04cb3ea 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -401,14 +401,7 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, return ret; } - /* - * take a reference to this mapping of the object. And this reference - * is unreferenced by the corresponding vm_close call. - */ - drm_gem_object_reference(obj); - - drm_vm_open_locked(drm_dev, vma); - + drm_gem_vm_open(vma); return 0; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3ecdde6..e4b8658 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -573,6 +573,7 @@ struct drm_local_map { void *handle; /**< User-space: "Handle" to pass to mmap() */ /**< Kernel-space: kernel-virtual address */ int mtrr; /**< MTRR slot used */ + int count; }; typedef struct drm_local_map drm_local_map_t; @@ -1116,8 +1117,6 @@ struct drm_device { struct idr ctx_idr; - struct list_head vmalist; /**< List of vmas (for debugging) */ - /*@} */ /** \name DMA support */ @@ -1274,8 +1273,8 @@ extern int drm_release(struct inode *inode, struct file *filp); /* Mapping support (drm_vm.h) */ extern int drm_mmap(struct file *filp, struct vm_area_struct *vma); extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma); -extern void drm_vm_open_locked(struct drm_device *dev, struct vm_area_struct *vma); -extern void drm_vm_close_locked(struct drm_device *dev, struct vm_area_struct *vma); +extern void drm_vma_open(struct drm_device *dev, struct vm_area_struct *vma); +extern void drm_vma_close(struct drm_device *dev, struct vm_area_struct *vma); extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait); /* Memory management support (drm_memory.h) */
The primary purpose of this list is for pretty printing the maps through debugfs/vma - per process information can be found in /proc/*/map et al. However, the list manipulation eats processor cycles resulting in a near indefinite (I got bored) stall for a leaky i-g-t/gem_concurrent_blit. There is one true user of the vmalist though. drm_vm_shm_close() uses the list to count the number of drm_local_map references. For this, we switch to using a counter inside drm_local_map. This patch kills drm_vm_open_locked and drm_vm_close_locked, and redirects the one user outside of the core (exoynos) to use the now semantically equivalent drm_gem_vm_open. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Dave Airlie <airlied@redhat.com> Cc: Rob Clark <robdclark@gmail.com> Cc: Inki Dae <inki.dae@samsung.com> --- drivers/gpu/drm/drm_bufs.c | 1 + drivers/gpu/drm/drm_drv.c | 8 ----- drivers/gpu/drm/drm_gem.c | 16 +++------ drivers/gpu/drm/drm_info.c | 38 --------------------- drivers/gpu/drm/drm_stub.c | 1 - drivers/gpu/drm/drm_vm.c | 58 +++++++-------------------------- drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 +---- include/drm/drmP.h | 7 ++-- 8 files changed, 20 insertions(+), 118 deletions(-)