tech-x11 archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: DRM scatter/gather mem allocation fix



On Sat, Apr 04, 2009 at 06:39:57AM +0000, Andrew Doran wrote:
> On Fri, Apr 03, 2009 at 09:19:09PM -0400, Rafal Boni wrote:
> 
> > -   entry = malloc(sizeof(*entry), M_DRM, M_WAITOK | M_ZERO);
> > +   entry = malloc(sizeof(*entry), M_DRM, M_ZERO | M_NOWAIT);
> ...
> > +       M_ZERO | M_NOWAIT);
> > +   dmah = malloc(sizeof(struct drm_dma_handle), M_DRM,
> > +       M_ZERO | M_NOWAIT);
> ...
> > +   ret = bus_dmamem_alloc(dmah->dmat, request->size, PAGE_SIZE, 0,
> > +       dmah->segs, 1, &nsegs, BUS_DMA_NOWAIT);
> ...
> > +   ret = bus_dmamem_map(dmah->dmat, dmah->segs, nsegs, request->size,
> > +        &dmah->addr, BUS_DMA_NOWAIT | BUS_DMA_COHERENT);
> ...
> > +   ret = bus_dmamap_create(dmah->dmat, request->size, pages, PAGE_SIZE,
> > +           0, BUS_DMA_NOWAIT | BUS_DMA_ALLOCNOW, &dmah->map);
> 
> - Do these need to be NOWAIT allocations? See kmem_alloc(9)
> - Can you use kmem_alloc+kmem_free?

I suspect not; I've converted things over to kmem_alloc(9) / kmem_free(9)
and make them all KM_SLEEP.  The NOWAITS / ALLOCNOW's came over from the
rev in FreeBSD; I'm not sure what their rationale for that was, but it
doesn't to be required / the right thing here.

I've also removed the ALLOCNOW since we do the bus_dma_load() immediately
after creating the map; it shouldn't matter if we get a failure in one
vs. the other.

Finally, I fixed up the sg_free() ioctl logic and removed the big XXX
since the incorrect input-only definition for the sg_alloc ioctl has
been fixed in our xsrc DRM includes (thanks mrg@ for helping find the
right copy of that file...).

Updated patch (and complete new drm_scatter.c) attached.. and is running
happily on my T43.

Thanks for the feedback!
--rafal

-- 
  Time is an illusion; lunchtime, doubly so.     |/\/\|           Rafal Boni
                   -- Ford Prefect               |\/\/|      
rafal%pobox.com@localhost
diff --git a/sys/dev/drm/drmP.h b/sys/dev/drm/drmP.h
index 3ffce87..9984f12 100644
--- a/sys/dev/drm/drmP.h
+++ b/sys/dev/drm/drmP.h
@@ -698,11 +698,13 @@ typedef struct drm_agp_head {
 } drm_agp_head_t;
 
 typedef struct drm_sg_mem {
-       unsigned long   handle;
-       void            *virtual;
-       int             pages;
-       dma_addr_t      *busaddr;
-       drm_dma_handle_t *dmah; /* Handle to PCI memory for ATI PCIGART table */
+       unsigned long             handle;
+       void                     *virtual;
+       int                       pages;
+       dma_addr_t               *busaddr;
+       struct drm_dma_handle    *sg_dmah;      /* Handle for sg_pages   */
+       struct drm_dma_handle    *dmah;         /* Handle to PCI memory  */
+                                               /* for ATI PCIGART table */
 } drm_sg_mem_t;
 
 typedef TAILQ_HEAD(drm_map_list, drm_local_map) drm_map_list_t;
@@ -1057,6 +1059,7 @@ int       drm_agp_unbind(drm_device_t *dev, 
drm_agp_binding_t *request);
 
 /* Scatter Gather Support (drm_scatter.c) */
 void   drm_sg_cleanup(drm_sg_mem_t *entry);
+int    drm_sg_alloc(struct drm_device *dev, struct drm_scatter_gather * 
request);
 
 #if defined(__FreeBSD__) || defined (__NetBSD__)
 /* sysctl support (drm_sysctl.h) */
@@ -1131,8 +1134,8 @@ int       drm_agp_unbind_ioctl(DRM_IOCTL_ARGS);
 int    drm_agp_bind_ioctl(DRM_IOCTL_ARGS);
 
 /* Scatter Gather Support (drm_scatter.c) */
-int    drm_sg_alloc(DRM_IOCTL_ARGS);
-int    drm_sg_free(DRM_IOCTL_ARGS);
+int    drm_sg_alloc_ioctl(DRM_IOCTL_ARGS);
+int    drm_sg_free_ioctl(DRM_IOCTL_ARGS);
 
 /* consistent PCI memory functions (drm_pci.c) */
 drm_dma_handle_t *drm_pci_alloc(drm_device_t *dev, size_t size, size_t align,
diff --git a/sys/dev/drm/drm_drv.c b/sys/dev/drm/drm_drv.c
index 046f9c8..31a908b 100644
--- a/sys/dev/drm/drm_drv.c
+++ b/sys/dev/drm/drm_drv.c
@@ -114,8 +114,8 @@ static drm_ioctl_desc_t               drm_ioctls[256] = {
        [DRM_IOCTL_NR(DRM_IOCTL_AGP_BIND)]      = { drm_agp_bind_ioctl, 
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
        [DRM_IOCTL_NR(DRM_IOCTL_AGP_UNBIND)]    = { drm_agp_unbind_ioctl, 
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
 
-       [DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC)]      = { drm_sg_alloc,    
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
-       [DRM_IOCTL_NR(DRM_IOCTL_SG_FREE)]       = { drm_sg_free,     
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
+       [DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC)]      = { drm_sg_alloc_ioctl, 
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
+       [DRM_IOCTL_NR(DRM_IOCTL_SG_FREE)]       = { drm_sg_free_ioctl, 
DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY },
 
        [DRM_IOCTL_NR(DRM_IOCTL_WAIT_VBLANK)]   = { drm_wait_vblank, 0 },
 };
diff --git a/sys/dev/drm/drm_scatter.c b/sys/dev/drm/drm_scatter.c
index 90326df..68c9f56 100644
--- a/sys/dev/drm/drm_scatter.c
+++ b/sys/dev/drm/drm_scatter.c
@@ -37,68 +37,102 @@ __KERNEL_RCSID(0, "$NetBSD: drm_scatter.c,v 1.7 2008/07/07 
00:33:23 mrg Exp $");
 __FBSDID("$FreeBSD: src/sys/dev/drm/drm_scatter.c,v 1.3 2006/05/17 06:29:36 
anholt Exp $");
 */
 
-#include "drmP.h"
-
-#define DEBUG_SCATTER 0
+/** @file drm_scatter.c
+ * Allocation of memory for scatter-gather mappings by the graphics chip.
+ *
+ * The memory allocated here is then made into an aperture in the card
+ * by drm_ati_pcigart_init().
+ */
 
-void drm_sg_cleanup(drm_sg_mem_t *entry)
-{
-       free((void *)entry->handle, M_DRM);
-       free(entry->busaddr, M_DRM);
-       free(entry, M_DRM);
-}
+#include "drmP.h"
 
-int drm_sg_alloc(DRM_IOCTL_ARGS)
+int
+drm_sg_alloc(struct drm_device *dev, struct drm_scatter_gather *request)
 {
-       DRM_DEVICE;
-       drm_scatter_gather_t request;
        drm_sg_mem_t *entry;
+       struct drm_dma_handle *dmah;
        unsigned long pages;
-       int i;
-
-       DRM_DEBUG( "%s\n", __FUNCTION__ );
-
-       if ( dev->sg )
-               return EINVAL;
-
-       DRM_COPY_FROM_USER_IOCTL(request, (drm_scatter_gather_t *)data,
-                            sizeof(request) );
+       int nsegs, ret, i;
 
-       entry = malloc(sizeof(*entry), M_DRM, M_WAITOK | M_ZERO);
+       entry = kmem_zalloc(sizeof(*entry), KM_SLEEP);
        if ( !entry )
                return ENOMEM;
 
-       pages = round_page(request.size) / PAGE_SIZE;
-       DRM_DEBUG( "sg size=%ld pages=%ld\n", request.size, pages );
+       pages = round_page(request->size) / PAGE_SIZE;
+       DRM_DEBUG( "sg size=%ld pages=%ld\n", request->size, pages );
 
        entry->pages = pages;
 
-       entry->busaddr = malloc(pages * sizeof(*entry->busaddr), M_DRM,
-           M_WAITOK | M_ZERO);
+       entry->busaddr = kmem_zalloc(pages * sizeof(*entry->busaddr), KM_SLEEP);
        if ( !entry->busaddr ) {
-               drm_sg_cleanup(entry);
+               kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }
 
-       entry->handle = (long)malloc(pages << PAGE_SHIFT, M_DRM,
-           M_WAITOK | M_ZERO);
-       if (entry->handle == 0) {
-               drm_sg_cleanup(entry);
+       dmah = kmem_zalloc(sizeof(struct drm_dma_handle), KM_SLEEP);
+       if (dmah == NULL) {
+               kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
+               kmem_free(entry, sizeof(*entry));
+               return ENOMEM;
+       }
+
+       dmah->dmat = dev->pa.pa_dmat;
+
+       ret = bus_dmamem_alloc(dmah->dmat, request->size, PAGE_SIZE, 0,
+           dmah->segs, 1, &nsegs, 0);
+       if (ret != 0) {
+               kmem_free(dmah, sizeof(struct drm_dma_handle));
+               kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
+               kmem_free(entry, sizeof(*entry));
+               return ENOMEM;
+       }
+
+       ret = bus_dmamem_map(dmah->dmat, dmah->segs, nsegs, request->size,
+            &dmah->addr, BUS_DMA_COHERENT);
+       if (ret != 0) {
+               bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
+               kmem_free(dmah, sizeof(struct drm_dma_handle));
+               kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
+               kmem_free(entry, sizeof(*entry));
+               return ENOMEM;
+       }
+
+       ret = bus_dmamap_create(dmah->dmat, request->size, pages, PAGE_SIZE,
+               0, 0, &dmah->map);
+
+       if (ret != 0) {
+               bus_dmamem_unmap(dmah->dmat, dmah->addr, request->size);
+               bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
+               kmem_free(dmah, sizeof(struct drm_dma_handle));
+               kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
+               kmem_free(entry, sizeof(*entry));
+               return ENOMEM;
+       }
+
+       ret = bus_dmamap_load(dmah->dmat, dmah->map, dmah->addr, request->size,
+           NULL, 0);
+       if (ret != 0) {
+               bus_dmamem_unmap(dmah->dmat, dmah->addr, request->size);
+               bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
+               kmem_free(dmah, sizeof(struct drm_dma_handle));
+               kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
+               kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }
 
-       for (i = 0; i < pages; i++) {
-               entry->busaddr[i] = vtophys(entry->handle + i * PAGE_SIZE);
+       DRM_DEBUG( "sg alloc nsegs = %d\n", dmah->map->dm_nsegs);
+
+        for (i = 0; i < pages; i++) {
+            entry->busaddr[i] = dmah->map->dm_segs[i].ds_addr;
        }
 
+       entry->handle = (unsigned long)dmah->addr;
+       entry->sg_dmah = dmah;
+       
        DRM_DEBUG( "sg alloc handle  = %08lx\n", entry->handle );
 
        entry->virtual = (void *)entry->handle;
-       request.handle = entry->handle;
-
-       DRM_COPY_TO_USER_IOCTL( (drm_scatter_gather_t *)data,
-                          request,
-                          sizeof(request) );
+       request->handle = entry->handle;
 
        DRM_LOCK();
        if (dev->sg) {
@@ -112,7 +146,47 @@ int drm_sg_alloc(DRM_IOCTL_ARGS)
        return 0;
 }
 
-int drm_sg_free(DRM_IOCTL_ARGS)
+int
+drm_sg_alloc_ioctl(DRM_IOCTL_ARGS)
+{
+       DRM_DEVICE;
+       drm_scatter_gather_t request;
+       int ret;
+
+       if ( dev->sg )
+               return EINVAL;
+
+       DRM_DEBUG( "%s\n", __FUNCTION__ );
+
+       DRM_COPY_FROM_USER_IOCTL(request, (drm_scatter_gather_t *)data,
+                            sizeof(request) );
+
+       ret = drm_sg_alloc(dev, &request);
+       if (ret != 0)
+               return ret;
+
+       DRM_COPY_TO_USER_IOCTL( (drm_scatter_gather_t *)data,
+                          request,
+                          sizeof(request) );
+
+       return 0;
+}
+
+void
+drm_sg_cleanup(drm_sg_mem_t *entry)
+{
+       struct drm_dma_handle *dmah = entry->sg_dmah;
+
+       bus_dmamap_unload(dmah->dmat, dmah->map);
+       bus_dmamem_unmap(dmah->dmat, dmah->addr, entry->pages << PAGE_SHIFT);
+       bus_dmamem_free(dmah->dmat, dmah->segs, 1);
+       kmem_free(dmah, sizeof(struct drm_dma_handle));
+       kmem_free(entry->busaddr, entry->pages * sizeof(*entry->busaddr));
+       kmem_free(entry, sizeof(*entry));
+}
+
+int
+drm_sg_free_ioctl(DRM_IOCTL_ARGS)
 {
        DRM_DEVICE;
        drm_scatter_gather_t request;
@@ -123,12 +197,14 @@ int drm_sg_free(DRM_IOCTL_ARGS)
 
        DRM_LOCK();
        entry = dev->sg;
+       if ( !entry || entry->handle != request.handle ) {
+               DRM_UNLOCK();
+               DRM_DEBUG( "sg free: error: %s handle\n", entry ? "invalid" : 
"no allocated");
+               return EINVAL;
+       }
        dev->sg = NULL;
        DRM_UNLOCK();
 
-       if ( !entry || entry->handle != request.handle )
-               return EINVAL;
-
        DRM_DEBUG( "sg free virtual  = 0x%lx\n", entry->handle );
 
        drm_sg_cleanup(entry);
/* $NetBSD: drm_scatter.c,v 1.7 2008/07/07 00:33:23 mrg Exp $ */

/* drm_scatter.h -- IOCTLs to manage scatter/gather memory -*- linux-c -*-
 * Created: Mon Dec 18 23:20:54 2000 by gareth%valinux.com@localhost */
/*-
 * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
 * All Rights Reserved.
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
 * to deal in the Software without restriction, including without limitation
 * the rights to use, copy, modify, merge, publish, distribute, sublicense,
 * and/or sell copies of the Software, and to permit persons to whom the
 * Software is furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice (including the next
 * paragraph) shall be included in all copies or substantial portions of the
 * Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
 * PRECISION INSIGHT AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
 * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
 * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 * DEALINGS IN THE SOFTWARE.
 *
 * Authors:
 *   Gareth Hughes <gareth%valinux.com@localhost>
 *   Eric Anholt <anholt%FreeBSD.org@localhost>
 *
 */

#include <sys/cdefs.h>
__KERNEL_RCSID(0, "$NetBSD: drm_scatter.c,v 1.7 2008/07/07 00:33:23 mrg Exp $");
/*
__FBSDID("$FreeBSD: src/sys/dev/drm/drm_scatter.c,v 1.3 2006/05/17 06:29:36 
anholt Exp $");
*/

/** @file drm_scatter.c
 * Allocation of memory for scatter-gather mappings by the graphics chip.
 *
 * The memory allocated here is then made into an aperture in the card
 * by drm_ati_pcigart_init().
 */

#include "drmP.h"

int
drm_sg_alloc(struct drm_device *dev, struct drm_scatter_gather *request)
{
        drm_sg_mem_t *entry;
        struct drm_dma_handle *dmah;
        unsigned long pages;
        int nsegs, ret, i;

        entry = kmem_zalloc(sizeof(*entry), KM_SLEEP);
        if ( !entry )
                return ENOMEM;

        pages = round_page(request->size) / PAGE_SIZE;
        DRM_DEBUG( "sg size=%ld pages=%ld\n", request->size, pages );

        entry->pages = pages;

        entry->busaddr = kmem_zalloc(pages * sizeof(*entry->busaddr), KM_SLEEP);
        if ( !entry->busaddr ) {
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        dmah = kmem_zalloc(sizeof(struct drm_dma_handle), KM_SLEEP);
        if (dmah == NULL) {
                kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        dmah->dmat = dev->pa.pa_dmat;

        ret = bus_dmamem_alloc(dmah->dmat, request->size, PAGE_SIZE, 0,
            dmah->segs, 1, &nsegs, 0);
        if (ret != 0) {
                kmem_free(dmah, sizeof(struct drm_dma_handle));
                kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        ret = bus_dmamem_map(dmah->dmat, dmah->segs, nsegs, request->size,
             &dmah->addr, BUS_DMA_COHERENT);
        if (ret != 0) {
                bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
                kmem_free(dmah, sizeof(struct drm_dma_handle));
                kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        ret = bus_dmamap_create(dmah->dmat, request->size, pages, PAGE_SIZE,
                0, 0, &dmah->map);

        if (ret != 0) {
                bus_dmamem_unmap(dmah->dmat, dmah->addr, request->size);
                bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
                kmem_free(dmah, sizeof(struct drm_dma_handle));
                kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        ret = bus_dmamap_load(dmah->dmat, dmah->map, dmah->addr, request->size,
            NULL, 0);
        if (ret != 0) {
                bus_dmamem_unmap(dmah->dmat, dmah->addr, request->size);
                bus_dmamem_free(dmah->dmat, dmah->segs, nsegs);
                kmem_free(dmah, sizeof(struct drm_dma_handle));
                kmem_free(entry->busaddr, pages * sizeof(*entry->busaddr));
                kmem_free(entry, sizeof(*entry));
                return ENOMEM;
        }

        DRM_DEBUG( "sg alloc nsegs = %d\n", dmah->map->dm_nsegs);

        for (i = 0; i < pages; i++) {
            entry->busaddr[i] = dmah->map->dm_segs[i].ds_addr;
        }

        entry->handle = (unsigned long)dmah->addr;
        entry->sg_dmah = dmah;
        
        DRM_DEBUG( "sg alloc handle  = %08lx\n", entry->handle );

        entry->virtual = (void *)entry->handle;
        request->handle = entry->handle;

        DRM_LOCK();
        if (dev->sg) {
                DRM_UNLOCK();
                drm_sg_cleanup(entry);
                return EINVAL;
        }
        dev->sg = entry;
        DRM_UNLOCK();

        return 0;
}

int
drm_sg_alloc_ioctl(DRM_IOCTL_ARGS)
{
        DRM_DEVICE;
        drm_scatter_gather_t request;
        int ret;

        if ( dev->sg )
                return EINVAL;

        DRM_DEBUG( "%s\n", __FUNCTION__ );

        DRM_COPY_FROM_USER_IOCTL(request, (drm_scatter_gather_t *)data,
                             sizeof(request) );

        ret = drm_sg_alloc(dev, &request);
        if (ret != 0)
                return ret;

        DRM_COPY_TO_USER_IOCTL( (drm_scatter_gather_t *)data,
                           request,
                           sizeof(request) );

        return 0;
}

void
drm_sg_cleanup(drm_sg_mem_t *entry)
{
        struct drm_dma_handle *dmah = entry->sg_dmah;

        bus_dmamap_unload(dmah->dmat, dmah->map);
        bus_dmamem_unmap(dmah->dmat, dmah->addr, entry->pages << PAGE_SHIFT);
        bus_dmamem_free(dmah->dmat, dmah->segs, 1);
        kmem_free(dmah, sizeof(struct drm_dma_handle));
        kmem_free(entry->busaddr, entry->pages * sizeof(*entry->busaddr));
        kmem_free(entry, sizeof(*entry));
}

int
drm_sg_free_ioctl(DRM_IOCTL_ARGS)
{
        DRM_DEVICE;
        drm_scatter_gather_t request;
        drm_sg_mem_t *entry;

        DRM_COPY_FROM_USER_IOCTL( request, (drm_scatter_gather_t *)data,
                             sizeof(request) );

        DRM_LOCK();
        entry = dev->sg;
        if ( !entry || entry->handle != request.handle ) {
                DRM_UNLOCK();
                DRM_DEBUG( "sg free: error: %s handle\n", entry ? "invalid" : 
"no allocated");
                return EINVAL;
        }
        dev->sg = NULL;
        DRM_UNLOCK();

        DRM_DEBUG( "sg free virtual  = 0x%lx\n", entry->handle );

        drm_sg_cleanup(entry);

        return 0;
}


Home | Main Index | Thread Index | Old Index