From a1d9e5abafe60ca2b7f96cadd1013695ada4ac41 Mon Sep 17 00:00:00 2001 From: Eric Anholt Date: Sun, 7 Nov 2004 04:11:15 +0000 Subject: Refine the locking of the DRM. Most significant is covering the driver ioctls with dev_lock, which is a major step toward being able to remove Giant. Covers some new pieces (dev->unique*) in the core, and avoids one call down into system internals with the drm lock held, which is usually bad (FreeBSD LOR #23, #27). --- bsd-core/drmP.h | 14 ++++------ bsd-core/drm_auth.c | 10 +++++-- bsd-core/drm_context.c | 13 +++++---- bsd-core/drm_drv.c | 17 ++++++++++-- bsd-core/drm_ioctl.c | 54 ++++++++++++++++++++++++------------ bsd-core/drm_irq.c | 18 ++++++------ bsd-core/drm_scatter.c | 22 +++++++++------ bsd-core/drm_vm.c | 75 +++++++++++++++++++++++--------------------------- 8 files changed, 130 insertions(+), 93 deletions(-) (limited to 'bsd-core') diff --git a/bsd-core/drmP.h b/bsd-core/drmP.h index 17af0d18..43e22039 100644 --- a/bsd-core/drmP.h +++ b/bsd-core/drmP.h @@ -197,6 +197,7 @@ MALLOC_DECLARE(M_DRM); #define DRM_SPINUNINIT(l) #define DRM_SPINLOCK(l) #define DRM_SPINUNLOCK(u) +#define DRM_SPINLOCK_ASSERT(l) #define DRM_CURRENTPID curproc->p_pid #define DRM_LOCK() #define DRM_UNLOCK() @@ -353,9 +354,7 @@ do { \ DRM_ERROR("filp doesn't match curproc\n"); \ return EINVAL; \ } \ - DRM_LOCK(); \ _priv = drm_find_file_by_proc(dev, DRM_CURPROC); \ - DRM_UNLOCK(); \ if (_priv == NULL) { \ DRM_ERROR("can't find authenticator\n"); \ return EINVAL; \ @@ -375,6 +374,7 @@ do { \ #define DRM_GETSAREA() \ do { \ drm_map_list_entry_t *listentry; \ + DRM_SPINLOCK_ASSERT(&dev->dev_lock); \ TAILQ_FOREACH(listentry, dev->maplist, link) { \ drm_local_map_t *map = listentry->map; \ if (map->type == _DRM_SHM && \ @@ -388,11 +388,13 @@ do { \ #if defined(__FreeBSD__) && __FreeBSD_version > 500000 #define DRM_WAIT_ON( ret, queue, timeout, condition ) \ for ( ret = 0 ; !ret && !(condition) ; ) { \ + DRM_UNLOCK(); \ mtx_lock(&dev->irq_lock); \ if (!(condition)) \ - ret = msleep(&(queue), &dev->irq_lock, \ + ret = msleep(&(queue), &dev->irq_lock, \ PZERO | PCATCH, "drmwtq", (timeout)); \ - mtx_unlock(&dev->irq_lock); \ + mtx_unlock(&dev->irq_lock); \ + DRM_LOCK(); \ } #else #define DRM_WAIT_ON( ret, queue, timeout, condition ) \ @@ -527,10 +529,6 @@ typedef struct drm_device_dma { _DRM_DMA_USE_AGP = 0x01, _DRM_DMA_USE_SG = 0x02 } flags; - - /* DMA support */ - drm_buf_t *this_buffer; /* Buffer being sent */ - drm_buf_t *next_buffer; /* Selected buffer to send */ } drm_device_dma_t; typedef struct drm_agp_mem { diff --git a/bsd-core/drm_auth.c b/bsd-core/drm_auth.c index fb556f33..dfd820da 100644 --- a/bsd-core/drm_auth.c +++ b/bsd-core/drm_auth.c @@ -116,12 +116,18 @@ static int drm_remove_magic(drm_device_t *dev, drm_magic_t magic) int drm_getmagic(DRM_IOCTL_ARGS) { + DRM_DEVICE; static drm_magic_t sequence = 0; drm_auth_t auth; drm_file_t *priv; - DRM_DEVICE; - DRM_GET_PRIV_WITH_RETURN(priv, filp); + DRM_LOCK(); + priv = drm_find_file_by_proc(dev, p); + DRM_UNLOCK(); + if (priv == NULL) { + DRM_ERROR("can't find authenticator\n"); + return EINVAL; + } /* Find unique magic */ if (priv->magic) { diff --git a/bsd-core/drm_context.c b/bsd-core/drm_context.c index d785d361..714edccc 100644 --- a/bsd-core/drm_context.c +++ b/bsd-core/drm_context.c @@ -229,15 +229,12 @@ int drm_context_switch_complete(drm_device_t *dev, int new) int drm_resctx(DRM_IOCTL_ARGS) { drm_ctx_res_t res; - drm_ctx_t ctx; int i; DRM_COPY_FROM_USER_IOCTL( res, (drm_ctx_res_t *)data, sizeof(res) ); if ( res.count >= DRM_RESERVED_CONTEXTS ) { - bzero(&ctx, sizeof(ctx)); for ( i = 0 ; i < DRM_RESERVED_CONTEXTS ; i++ ) { - ctx.handle = i; if ( DRM_COPY_TO_USER( &res.contexts[i], &i, sizeof(i) ) ) return DRM_ERR(EFAULT); @@ -269,8 +266,11 @@ int drm_addctx(DRM_IOCTL_ARGS) return DRM_ERR(ENOMEM); } - if (dev->context_ctor && ctx.handle != DRM_KERNEL_CONTEXT) + if (dev->context_ctor && ctx.handle != DRM_KERNEL_CONTEXT) { + DRM_LOCK(); dev->context_ctor(dev, ctx.handle); + DRM_UNLOCK(); + } DRM_COPY_TO_USER_IOCTL( (drm_ctx_t *)data, ctx, sizeof(ctx) ); @@ -330,8 +330,11 @@ int drm_rmctx(DRM_IOCTL_ARGS) DRM_DEBUG( "%d\n", ctx.handle ); if ( ctx.handle != DRM_KERNEL_CONTEXT ) { - if (dev->context_dtor) + if (dev->context_dtor) { + DRM_LOCK(); dev->context_dtor(dev, ctx.handle); + DRM_UNLOCK(); + } drm_ctxbitmap_free(dev, ctx.handle); } diff --git a/bsd-core/drm_drv.c b/bsd-core/drm_drv.c index 7880f76c..fb9313af 100644 --- a/bsd-core/drm_drv.c +++ b/bsd-core/drm_drv.c @@ -809,9 +809,17 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags, drm_ioctl_desc_t *ioctl; int (*func)(DRM_IOCTL_ARGS); int nr = DRM_IOCTL_NR(cmd); + int is_driver_ioctl = 0; drm_file_t *priv; + DRMFILE filp = (DRMFILE)(uintptr_t)DRM_CURRENTPID; - DRM_GET_PRIV_WITH_RETURN(priv, (DRMFILE)(uintptr_t)DRM_CURRENTPID); + DRM_LOCK(); + priv = drm_find_file_by_proc(dev, p); + DRM_UNLOCK(); + if (priv == NULL) { + DRM_ERROR("can't find authenticator\n"); + return EINVAL; + } atomic_inc( &dev->counts[_DRM_STAT_IOCTLS] ); ++priv->ioctl_count; @@ -868,6 +876,7 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags, return EINVAL; } ioctl = &dev->driver_ioctls[nr]; + is_driver_ioctl = 1; } func = ioctl->func; @@ -879,7 +888,11 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags, !priv->authenticated)) return EACCES; - retcode = func(kdev, cmd, data, flags, p, (void *)(uintptr_t)DRM_CURRENTPID); + if (is_driver_ioctl) + DRM_LOCK(); + retcode = func(kdev, cmd, data, flags, p, filp); + if (is_driver_ioctl) + DRM_UNLOCK(); if (retcode != 0) DRM_DEBUG(" returning %d\n", retcode); diff --git a/bsd-core/drm_ioctl.c b/bsd-core/drm_ioctl.c index 829a99fe..d40a7c90 100644 --- a/bsd-core/drm_ioctl.c +++ b/bsd-core/drm_ioctl.c @@ -64,40 +64,53 @@ int drm_setunique(DRM_IOCTL_ARGS) DRM_DEVICE; drm_unique_t u; int domain, bus, slot, func, ret; - - if (dev->unique_len || dev->unique) - return DRM_ERR(EBUSY); + char *busid; DRM_COPY_FROM_USER_IOCTL( u, (drm_unique_t *)data, sizeof(u) ); + /* Check and copy in the submitted Bus ID */ if (!u.unique_len || u.unique_len > 1024) return DRM_ERR(EINVAL); - dev->unique_len = u.unique_len; - dev->unique = malloc(u.unique_len + 1, M_DRM, M_NOWAIT); - - if (dev->unique == NULL) + busid = malloc(u.unique_len + 1, M_DRM, M_WAITOK); + if (busid == NULL) return DRM_ERR(ENOMEM); - if (DRM_COPY_FROM_USER(dev->unique, u.unique, dev->unique_len)) + if (DRM_COPY_FROM_USER(busid, u.unique, u.unique_len)) { + free(busid, M_DRM); return DRM_ERR(EFAULT); - - dev->unique[dev->unique_len] = '\0'; + } + busid[u.unique_len] = '\0'; /* Return error if the busid submitted doesn't match the device's actual * busid. */ - ret = sscanf(dev->unique, "PCI:%d:%d:%d", &bus, &slot, &func); - if (ret != 3) + ret = sscanf(busid, "PCI:%d:%d:%d", &bus, &slot, &func); + if (ret != 3) { + free(busid, M_DRM); return DRM_ERR(EINVAL); + } domain = bus >> 8; bus &= 0xff; if ((domain != dev->pci_domain) || (bus != dev->pci_bus) || (slot != dev->pci_slot) || - (func != dev->pci_func)) + (func != dev->pci_func)) { + free(busid, M_DRM); return DRM_ERR(EINVAL); + } + + /* Actually set the device's busid now. */ + DRM_LOCK(); + if (dev->unique_len || dev->unique) { + DRM_UNLOCK(); + return DRM_ERR(EBUSY); + } + + dev->unique_len = u.unique_len; + dev->unique = busid; + DRM_UNLOCK(); return 0; } @@ -107,17 +120,25 @@ static int drm_set_busid(drm_device_t *dev) { - if (dev->unique != NULL) + DRM_LOCK(); + + if (dev->unique != NULL) { + DRM_UNLOCK(); return EBUSY; + } dev->unique_len = 20; dev->unique = malloc(dev->unique_len + 1, M_DRM, M_NOWAIT); - if (dev->unique == NULL) + if (dev->unique == NULL) { + DRM_UNLOCK(); return ENOMEM; + } snprintf(dev->unique, dev->unique_len, "pci:%04x:%02x:%02x.%1x", dev->pci_domain, dev->pci_bus, dev->pci_slot, dev->pci_func); + DRM_UNLOCK(); + return 0; } @@ -264,9 +285,6 @@ int drm_setversion(DRM_IOCTL_ARGS) if (sv.drm_dd_major != dev->driver_major || sv.drm_dd_minor < 0 || sv.drm_dd_minor > dev->driver_minor) return EINVAL; -#ifdef DRIVER_SETVERSION - DRIVER_SETVERSION(dev, &sv); -#endif } return 0; } diff --git a/bsd-core/drm_irq.c b/bsd-core/drm_irq.c index eb32bac4..422f7222 100644 --- a/bsd-core/drm_irq.c +++ b/bsd-core/drm_irq.c @@ -73,25 +73,22 @@ int drm_irq_install(drm_device_t *dev) if (dev->irq == 0 || dev->dev_private == NULL) return DRM_ERR(EINVAL); + DRM_DEBUG( "%s: irq=%d\n", __FUNCTION__, dev->irq ); + DRM_LOCK(); if (dev->irq_enabled) { DRM_UNLOCK(); return DRM_ERR(EBUSY); } dev->irq_enabled = 1; - DRM_UNLOCK(); - - DRM_DEBUG( "%s: irq=%d\n", __FUNCTION__, dev->irq ); dev->context_flag = 0; - dev->dma->next_buffer = NULL; - dev->dma->this_buffer = NULL; - DRM_SPININIT(dev->irq_lock, "DRM IRQ lock"); /* Before installing handler */ dev->irq_preinstall(dev); + DRM_UNLOCK(); /* Install handler */ #ifdef __FreeBSD__ @@ -125,7 +122,9 @@ int drm_irq_install(drm_device_t *dev) #endif /* After installing handler */ + DRM_LOCK(); dev->irq_postinstall(dev); + DRM_UNLOCK(); return 0; err: @@ -143,9 +142,6 @@ err: return retcode; } -/* XXX: This function needs to be called with the device lock held. In some - * cases it isn't, so far. - */ int drm_irq_uninstall(drm_device_t *dev) { int irqrid; @@ -162,8 +158,10 @@ int drm_irq_uninstall(drm_device_t *dev) dev->irq_uninstall(dev); #ifdef __FreeBSD__ + DRM_UNLOCK(); bus_teardown_intr(dev->device, dev->irqr, dev->irqh); bus_release_resource(dev->device, SYS_RES_IRQ, irqrid, dev->irqr); + DRM_LOCK(); #elif defined(__NetBSD__) || defined(__OpenBSD__) pci_intr_disestablish(&dev->pa.pa_pc, dev->irqh); #endif @@ -242,7 +240,9 @@ int drm_wait_vblank(DRM_IOCTL_ARGS) #endif ret = EINVAL; } else { + DRM_LOCK(); ret = dev->vblank_wait(dev, &vblwait.request.sequence); + DRM_UNLOCK(); microtime(&now); vblwait.reply.tval_sec = now.tv_sec; diff --git a/bsd-core/drm_scatter.c b/bsd-core/drm_scatter.c index 03e82439..7a035af5 100644 --- a/bsd-core/drm_scatter.c +++ b/bsd-core/drm_scatter.c @@ -36,7 +36,6 @@ void drm_sg_cleanup(drm_sg_mem_t *entry) { free(entry->virtual, M_DRM); - free(entry->busaddr, M_DRM); free(entry, M_DRM); } @@ -56,7 +55,7 @@ int drm_sg_alloc(DRM_IOCTL_ARGS) DRM_COPY_FROM_USER_IOCTL(request, (drm_scatter_gather_t *)data, sizeof(request) ); - entry = malloc(sizeof(*entry), M_DRM, M_NOWAIT | M_ZERO); + entry = malloc(sizeof(*entry), M_DRM, M_WAITOK | M_ZERO); if ( !entry ) return ENOMEM; @@ -66,16 +65,15 @@ int drm_sg_alloc(DRM_IOCTL_ARGS) entry->pages = pages; entry->busaddr = malloc(pages * sizeof(*entry->busaddr), M_DRM, - M_NOWAIT | M_ZERO); + M_WAITOK | M_ZERO); if ( !entry->busaddr ) { - free(entry, M_DRM); + drm_sg_cleanup(entry); return ENOMEM; } entry->virtual = malloc(pages << PAGE_SHIFT, M_DRM, M_WAITOK | M_ZERO); if ( !entry->virtual ) { - free(entry->busaddr, M_DRM); - free(entry, M_DRM); + drm_sg_cleanup(entry); return ENOMEM; } @@ -90,12 +88,16 @@ int drm_sg_alloc(DRM_IOCTL_ARGS) request, sizeof(request) ); + DRM_LOCK(); + if (dev->sg) { + DRM_UNLOCK(); + drm_sg_cleanup(entry); + return EINVAL; + } dev->sg = entry; + DRM_UNLOCK(); return 0; - - drm_sg_cleanup(entry); - return ENOMEM; } int drm_sg_free(DRM_IOCTL_ARGS) @@ -107,8 +109,10 @@ int drm_sg_free(DRM_IOCTL_ARGS) DRM_COPY_FROM_USER_IOCTL( request, (drm_scatter_gather_t *)data, sizeof(request) ); + DRM_LOCK(); entry = dev->sg; dev->sg = NULL; + DRM_UNLOCK(); if ( !entry || entry->handle != request.handle ) return EINVAL; diff --git a/bsd-core/drm_vm.c b/bsd-core/drm_vm.c index f3558b9e..1b5d4d3f 100644 --- a/bsd-core/drm_vm.c +++ b/bsd-core/drm_vm.c @@ -25,35 +25,6 @@ #include "drmP.h" #include "drm.h" -#if defined(__FreeBSD__) && __FreeBSD_version >= 500102 -static int drm_dma_mmap(struct cdev *kdev, vm_offset_t offset, - vm_paddr_t *paddr, int prot) -#elif defined(__FreeBSD__) -static int drm_dma_mmap(dev_t kdev, vm_offset_t offset, int prot) -#elif defined(__NetBSD__) || defined(__OpenBSD__) -static paddr_t drm_dma_mmap(dev_t kdev, vm_offset_t offset, int prot) -#endif -{ - DRM_DEVICE; - drm_device_dma_t *dma = dev->dma; - unsigned long physical; - unsigned long page; - - if (dma == NULL || dma->pagelist == NULL) - return -1; - - page = offset >> PAGE_SHIFT; - physical = dma->pagelist[page]; - - DRM_DEBUG("0x%08lx (page %lu) => 0x%08lx\n", (long)offset, page, physical); -#if defined(__FreeBSD__) && __FreeBSD_version >= 500102 - *paddr = physical; - return 0; -#else - return atop(physical); -#endif -} - #if defined(__FreeBSD__) && __FreeBSD_version >= 500102 int drm_mmap(struct cdev *kdev, vm_offset_t offset, vm_paddr_t *paddr, int prot) @@ -67,20 +38,40 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot) drm_local_map_t *map = NULL; drm_map_list_entry_t *listentry = NULL; drm_file_t *priv; + drm_map_type_t type; - DRM_GET_PRIV_WITH_RETURN(priv, (DRMFILE)(uintptr_t)DRM_CURRENTPID); + DRM_LOCK(); + priv = drm_find_file_by_proc(dev, DRM_CURPROC); + DRM_UNLOCK(); + if (priv == NULL) { + DRM_ERROR("can't find authenticator\n"); + return EINVAL; + } if (!priv->authenticated) return DRM_ERR(EACCES); - if (dev->dma - && offset >= 0 - && offset < ptoa(dev->dma->page_count)) + DRM_SPINLOCK(&dev->dma_lock); + if (dev->dma && offset >= 0 && offset < ptoa(dev->dma->page_count)) { + drm_device_dma_t *dma = dev->dma; + + if (dma->pagelist != NULL) { + unsigned long page = offset >> PAGE_SHIFT; + unsigned long phys = dma->pagelist[page]; + #if defined(__FreeBSD__) && __FreeBSD_version >= 500102 - return drm_dma_mmap(kdev, offset, paddr, prot); + *paddr = phys; + DRM_SPINUNLOCK(&dev->dma_lock); + return 0; #else - return drm_dma_mmap(kdev, offset, prot); -#endif + return atop(phys); +#endif + } else { + DRM_SPINUNLOCK(&dev->dma_lock); + return -1; + } + } + DRM_SPINUNLOCK(&dev->dma_lock); /* A sequential search of a linked list is fine here because: 1) there will only be @@ -89,23 +80,27 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot) once, so it doesn't have to be optimized for performance, even if the list was a bit longer. */ + DRM_LOCK(); TAILQ_FOREACH(listentry, dev->maplist, link) { map = listentry->map; -/* DRM_DEBUG("considering 0x%x..0x%x\n", map->offset, map->offset + map->size - 1);*/ - if (offset >= map->offset - && offset < map->offset + map->size) break; + if (offset >= map->offset && offset < map->offset + map->size) + break; } if (!listentry) { + DRM_UNLOCK(); DRM_DEBUG("can't find map\n"); return -1; } if (((map->flags&_DRM_RESTRICTED) && DRM_SUSER(DRM_CURPROC))) { + DRM_UNLOCK(); DRM_DEBUG("restricted map\n"); return -1; } + type = map->type; + DRM_UNLOCK(); - switch (map->type) { + switch (type) { case _DRM_FRAME_BUFFER: case _DRM_REGISTERS: case _DRM_AGP: -- cgit v1.2.3