Hi guys,

By means of introduction: I'm the guy attempting to port the DRM so that
it'll run on OpenBSD. So far things are going fairly well (I don't know
how many hours i've sunk into this though). The code isn't all in our
cvs tree since it's currently locked for release. In fact the difference
between the cvs tree and what i'm using locally is rather large.
However, I thought it was about time i started sending patches back
(i've been putting it off since things weren't 100%). As a first patch,
this fixes the freebsd codepath a bit by removing some deadlocks, the
functions affected only touch the lock on a few of their exit points,
this makes sure that the lock is dealt with properly on all of them.

This isn't tested on freebsd (I don't have a system running it), also
the current drm tree wouldn't compile on freebsd since the vblank
changes I'm debugging some patches to fix that, too. Patch is attached
inline after the rest of this mail.

For anyone interested, here's the current status of DRI on openbsd:
- SiS apparently works (i've not heard from the guy testing it for a while
though).
- 965 (G965 and GM965) is mostly ok, but there are a few instabilities
(the hardware registers get out of sync and X crashes). There's
obviously a deadlock there also,  since also sometimes things get
utterly buggered and X locks up trying to get the hardware lock (the
mouse moves, but all events get dropped as being out of order). Both of
these can be triggered by multiple renderers, and moving the application
around a lot.

However, it works well enough to things like glxgears to work most of
the time, and i've spent several hours playing quake2 in gl mode without
problems. Also glxgears intermittenly segfaults at startup (in memcpy
from realloc from emit_op3fn. I can provide backtraces to anyone who
wants it).
- some other intels work.
- r200 works when forced to pci mode. In agp mode it loops in
wait_for_cp_idle on X startup. I've not tested this in a while, but that
certainly seems to be the case most times.
- radeon (r100), starts but i'm told glxgears always crashes (in
choose_emit_func) every time. as with other glx apps. Again this was
pci-mode.
- r300 seems to work well so far, I've an PCIE X800SE that i've not been
able to crash yet. However I only started playing with it recently and
haven't tried anything heavy on it yet.

I haven't even started on the TTM yet, but it's on my todo list.

Regards,

Owain
([EMAIL PROTECTED])
-- 
The goal of science is to build better mousetraps.
The goal of nature is to build better mice.
>From 427430ef790f80431f372d0340c7c710c8bee1ae Mon Sep 17 00:00:00 2001
From: Owain Ainsworth <[EMAIL PROTECTED]>
Date: Thu, 6 Mar 2008 17:52:27 +0000
Subject: [PATCH] Fix up some locking issues in the bsd port. Kill some 
deadlocks.

drm_add_magic:
        remove superfluous locking calls (some bsds mutexes aren't
        recursive. we already call it locked.
drm_bufs.c:
        drm_addmap: we start the function call locked. make sure we leave it
        locked. While i'm here deal with failed ioremap();
        addbufs_*: make sure that we restore locking to what's expected upon
        return.
drm_drawable.c:
        prevent freeing NULL (i've seen this happen), in drm_update_draw, also
        add an unlock before one of the return conditions.
drm_vm.c
        make sure that we unlock before return. but remove one locking call that
        never will be reached.
---
 bsd-core/drm_auth.c     |    2 --
 bsd-core/drm_bufs.c     |   47 +++++++++++++++++++++++++++++++++++++----------
 bsd-core/drm_drawable.c |   12 ++++++++----
 bsd-core/drm_vm.c       |    3 +--
 4 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/bsd-core/drm_auth.c b/bsd-core/drm_auth.c
index aa8238c..fdc2d77 100644
--- a/bsd-core/drm_auth.c
+++ b/bsd-core/drm_auth.c
@@ -79,7 +79,6 @@ static int drm_add_magic(drm_device_t *dev, drm_file_t *priv, 
drm_magic_t magic)
        entry->priv  = priv;
        entry->next  = NULL;
 
-       DRM_LOCK();
        if (dev->magiclist[hash].tail) {
                dev->magiclist[hash].tail->next = entry;
                dev->magiclist[hash].tail       = entry;
@@ -87,7 +86,6 @@ static int drm_add_magic(drm_device_t *dev, drm_file_t *priv, 
drm_magic_t magic)
                dev->magiclist[hash].head       = entry;
                dev->magiclist[hash].tail       = entry;
        }
-       DRM_UNLOCK();
 
        return 0;
 }
diff --git a/bsd-core/drm_bufs.c b/bsd-core/drm_bufs.c
index 9b58c59..b72b9c1 100644
--- a/bsd-core/drm_bufs.c
+++ b/bsd-core/drm_bufs.c
@@ -149,8 +149,10 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
         * initialization necessary.
         */
        map = malloc(sizeof(*map), M_DRM, M_ZERO | M_NOWAIT);
-       if ( !map )
+       if ( !map ) {
+               DRM_LOCK();
                return ENOMEM;
+       }
 
        map->offset = offset;
        map->size = size;
@@ -160,6 +162,10 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
        switch ( map->type ) {
        case _DRM_REGISTERS:
                map->handle = drm_ioremap(dev, map);
+               if (!map->handle) {
+                       DRM_LOCK();
+                       return EINVAL;
+               }
                if (!(map->flags & _DRM_WRITE_COMBINING))
                        break;
                /* FALLTHROUGH */
@@ -173,6 +179,7 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
                           map->size, drm_order(map->size), map->handle );
                if ( !map->handle ) {
                        free(map, M_DRM);
+                       DRM_LOCK();
                        return ENOMEM;
                }
                map->offset = (unsigned long)map->handle;
@@ -180,7 +187,6 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
                        /* Prevent a 2nd X Server from creating a 2nd lock */
                        DRM_LOCK();
                        if (dev->lock.hw_lock != NULL) {
-                               DRM_UNLOCK();
                                free(map->handle, M_DRM);
                                free(map, M_DRM);
                                return EBUSY;
@@ -213,12 +219,14 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
                }
                if (!valid) {
                        free(map, M_DRM);
+                       DRM_LOCK();
                        return EACCES;
                }*/
                break;
        case _DRM_SCATTER_GATHER:
                if (!dev->sg) {
                        free(map, M_DRM);
+                       DRM_LOCK();
                        return EINVAL;
                }
                map->offset = map->offset + dev->sg->handle;
@@ -236,6 +244,7 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
                map->dmah = drm_pci_alloc(dev, map->size, align, 0xfffffffful);
                if (map->dmah == NULL) {
                        free(map, M_DRM);
+                       DRM_LOCK();
                        return ENOMEM;
                }
                map->handle = map->dmah->vaddr;
@@ -244,6 +253,7 @@ int drm_addmap(drm_device_t * dev, unsigned long offset, 
unsigned long size,
        default:
                DRM_ERROR("Bad map type %d\n", map->type);
                free(map, M_DRM);
+               DRM_LOCK();
                return EINVAL;
        }
 
@@ -784,12 +794,16 @@ int drm_addbufs_agp(drm_device_t *dev, drm_buf_desc_t 
*request)
 
        DRM_SPINLOCK(&dev->dma_lock);
 
-       if (request->count < 0 || request->count > 4096)
+       if (request->count < 0 || request->count > 4096) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
        
        order = drm_order(request->size);
-       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER)
+       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
 
        /* No more allocations after first buffer-using ioctl. */
        if (dev->buf_use != 0) {
@@ -815,15 +829,21 @@ int drm_addbufs_sg(drm_device_t *dev, drm_buf_desc_t 
*request)
 
        DRM_SPINLOCK(&dev->dma_lock);
 
-       if (!DRM_SUSER(DRM_CURPROC))
+       if (!DRM_SUSER(DRM_CURPROC)) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EACCES;
+       }
 
-       if (request->count < 0 || request->count > 4096)
+       if (request->count < 0 || request->count > 4096) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
        
        order = drm_order(request->size);
-       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER)
+       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
 
        /* No more allocations after first buffer-using ioctl. */
        if (dev->buf_use != 0) {
@@ -849,15 +869,21 @@ int drm_addbufs_pci(drm_device_t *dev, drm_buf_desc_t 
*request)
 
        DRM_SPINLOCK(&dev->dma_lock);
 
-       if (!DRM_SUSER(DRM_CURPROC))
+       if (!DRM_SUSER(DRM_CURPROC)) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EACCES;
+       }
 
-       if (request->count < 0 || request->count > 4096)
+       if (request->count < 0 || request->count > 4096) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
        
        order = drm_order(request->size);
-       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER)
+       if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
+       }
 
        /* No more allocations after first buffer-using ioctl. */
        if (dev->buf_use != 0) {
@@ -960,6 +986,7 @@ int drm_markbufs(drm_device_t *dev, void *data, struct 
drm_file *file_priv)
        DRM_SPINLOCK(&dev->dma_lock);
        if (request->low_mark > dma->bufs[order].buf_count ||
            request->high_mark > dma->bufs[order].buf_count) {
+               DRM_SPINUNLOCK(&dev->dma_lock);
                return EINVAL;
        }
 
diff --git a/bsd-core/drm_drawable.c b/bsd-core/drm_drawable.c
index fb318d4..1fb9c57 100644
--- a/bsd-core/drm_drawable.c
+++ b/bsd-core/drm_drawable.c
@@ -122,9 +122,11 @@ int drm_update_draw(drm_device_t *dev, void *data, struct 
drm_file *file_priv)
        case DRM_DRAWABLE_CLIPRECTS:
                DRM_SPINLOCK(&dev->drw_lock);
                if (update->num != info->num_rects) {
-                       drm_free(info->rects,
-                           sizeof(*info->rects) * info->num_rects,
-                           DRM_MEM_DRAWABLE);
+                       if (info->rects) {
+                               drm_free(info->rects,
+                                   sizeof(*info->rects) * info->num_rects,
+                                   DRM_MEM_DRAWABLE);
+                       }
                        info->rects = NULL;
                        info->num_rects = 0;
                }
@@ -135,8 +137,10 @@ int drm_update_draw(drm_device_t *dev, void *data, struct 
drm_file *file_priv)
                if (info->rects == NULL) {
                        info->rects = drm_alloc(sizeof(*info->rects) *
                            update->num, DRM_MEM_DRAWABLE);
-                       if (info->rects == NULL)
+                       if (info->rects == NULL) {
+                               DRM_SPINUNLOCK(&dev->drw_lock);
                                return ENOMEM;
+                       }
                        info->num_rects = update->num;
                }
                /* For some reason the pointer arg is unsigned long long. */
diff --git a/bsd-core/drm_vm.c b/bsd-core/drm_vm.c
index fea31f5..e331f6e 100644
--- a/bsd-core/drm_vm.c
+++ b/bsd-core/drm_vm.c
@@ -67,9 +67,9 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot)
                        unsigned long page = offset >> PAGE_SHIFT;
                        unsigned long phys = dma->pagelist[page];
 
+                       DRM_SPINUNLOCK(&dev->dma_lock);
 #if defined(__FreeBSD__) && __FreeBSD_version >= 500102
                        *paddr = phys;
-                       DRM_SPINUNLOCK(&dev->dma_lock);
                        return 0;
 #else
                        return atop(phys);
@@ -78,7 +78,6 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot)
                        DRM_SPINUNLOCK(&dev->dma_lock);
                        return -1;
                }
-               DRM_SPINUNLOCK(&dev->dma_lock);
        }
 
                                /* A sequential search of a linked list is
-- 
1.5.4.1

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Reply via email to