(At least in my opinion) the DRM template code is somewhat ugly and difficult to
modify since the code is all spread out and a small change usually leads
to modifications is several places. This is surely result of some
groing pain since actually there is no real reason to be like that since
it's possible to clearly define modules. Actually the modules are
more-or-less already defined, and it's mostly a matter to put some
things back where they belong.
You can see in the attached patch [at least part of] what I'd like to
do, exemplified in the drm_scatter.h template [or module]. Basically it
consists in:
- move the structure definitions and function declarations in the
drm_*.h
- move the function definitions into drm_*_tmp.h
- crealy split the ioctls from the actual worker functions
- give the '_ioctl' suffix to ioctls (no more confusing naming such as
'sg_alloc' vs 'alloc_sg' vs 'do_sg_alloc' vs ...)
- add a little more documentation
- implement those thin OS wrappers as inline/macros instead of
functions (not shown here)
As you see, there isn't much brainwork going here (hence the
'janitorial' in the subject), but I think this would provide a much
saner codebase for some of the DRM work which has to be done. Note that
this also won't affect binary compatability, and require only small
changes to the *_drv.c driver source files.
I would also like to discuss the possibility of:
- dropping the DRM(my_func)() for drm_my_func(). If I'm not mistaken,
these symbols aren't exported to the rest of the kernel so there
isn't any conflict when several DRM's are loaded simulatenously on
the kernel (or is there?) Besides of reducing some visual clutter
this would allow to a better parity between the source code and the
Doxygen generated documentation: at the moment those DRM(...) tags
have to be eliminated before feeding into Doxygen to allow
cross-referecing.
- (I had something else to mention here, but forgot it...)
A more pratical issue (assuming that everbody is OK with what I'm
proposing here) is about the commiting procedure:
- should I committing each module as I write, perhaps posting before
review before commiting? (my favorite)
- should I setup a seperate branch (plz no - I'm sick of branching and
merging and branching again and... 8( )
- or should I just setup a big patch once everything is done and do it
in one go?
Jos� Fonseca
Index: drmP.h
===================================================================
RCS file:
/cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v
retrieving revision 1.73
diff -u -r1.73 drmP.h
--- drmP.h 27 May 2003 00:37:32 -0000 1.73
+++ drmP.h 2 Jun 2003 23:54:43 -0000
@@ -86,6 +86,10 @@
#include "drm_os_linux.h"
+typedef struct drm_device drm_device_t;
+
+#include "drm_scatter.h"
+
/***********************************************************************/
/** \name DRM template customization defaults */
@@ -606,17 +610,6 @@
} drm_agp_head_t;
#endif
-/**
- * Scatter-gather memory.
- */
-typedef struct drm_sg_mem {
- unsigned long handle;
- void *virtual;
- int pages;
- struct page **pagelist;
- dma_addr_t *busaddr;
-} drm_sg_mem_t;
-
typedef struct drm_sigdata {
int context;
drm_hw_lock_t *lock;
@@ -646,7 +639,7 @@
/**
* DRM device structure.
*/
-typedef struct drm_device {
+struct drm_device {
const char *name; /**< Simple driver name */
char *unique; /**< Unique identifier: e.g., busid */
int unique_len; /**< Length of unique field */
@@ -761,7 +754,7 @@
void *dev_private; /**< device private data */
drm_sigdata_t sigdata; /**< For block_all_signals */
sigset_t sigmask;
-} drm_device_t;
+};
/******************************************************************/
@@ -997,15 +990,6 @@
extern int DRM(proc_cleanup)(int minor,
struct proc_dir_entry *root,
struct proc_dir_entry *dev_root);
-
-#if __HAVE_SG
- /* Scatter Gather Support (drm_scatter.h) */
-extern void DRM(sg_cleanup)(drm_sg_mem_t *entry);
-extern int DRM(sg_alloc)(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg);
-extern int DRM(sg_free)(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg);
-#endif
/* ATI PCIGART support (ati_pcigart.h) */
extern int DRM(ati_pcigart_init)(drm_device_t *dev,
Index: drm_drv.h
===================================================================
RCS file:
/cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_drv.h,v
retrieving revision 1.37
diff -u -r1.37 drm_drv.h
--- drm_drv.h 27 May 2003 00:37:32 -0000 1.37
+++ drm_drv.h 2 Jun 2003 23:54:45 -0000
@@ -236,8 +236,8 @@
#endif
#if __HAVE_SG
- [DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC)] = { DRM(sg_alloc), 1, 1 },
- [DRM_IOCTL_NR(DRM_IOCTL_SG_FREE)] = { DRM(sg_free), 1, 1 },
+ [DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC)] = { DRM(sg_alloc_ioctl), 1, 1 },
+ [DRM_IOCTL_NR(DRM_IOCTL_SG_FREE)] = { DRM(sg_free_ioctl), 1, 1 },
#endif
#if __HAVE_VBL_IRQ
@@ -486,10 +486,7 @@
* isn't defined.
*/
#if __HAVE_SG
- if(dev->sg) {
- DRM(sg_cleanup)(dev->sg);
- dev->sg = NULL;
- }
+ DRM(sg_cleanup)(dev);
#endif
break;
}
Index: drm_scatter.h
===================================================================
RCS file:
/cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_scatter.h,v
retrieving revision 1.10
diff -u -r1.10 drm_scatter.h
--- drm_scatter.h 27 May 2003 00:37:33 -0000 1.10
+++ drm_scatter.h 2 Jun 2003 23:54:45 -0000
@@ -6,8 +6,6 @@
*/
/*
- * Created: Mon Dec 18 23:20:54 2000 by [EMAIL PROTECTED]
- *
* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
* All Rights Reserved.
*
@@ -31,199 +29,37 @@
* DEALINGS IN THE SOFTWARE.
*/
-#define __NO_VERSION__
-#include <linux/config.h>
-#include <linux/vmalloc.h>
-#include "drmP.h"
-
-#define DEBUG_SCATTER 0
-
-void DRM(sg_cleanup)( drm_sg_mem_t *entry )
-{
- struct page *page;
- int i;
-
- for ( i = 0 ; i < entry->pages ; i++ ) {
- page = entry->pagelist[i];
- if ( page )
- ClearPageReserved( page );
- }
-
- vfree( entry->virtual );
-
- DRM(free)( entry->busaddr,
- entry->pages * sizeof(*entry->busaddr),
- DRM_MEM_PAGES );
- DRM(free)( entry->pagelist,
- entry->pages * sizeof(*entry->pagelist),
- DRM_MEM_PAGES );
- DRM(free)( entry,
- sizeof(*entry),
- DRM_MEM_SGLISTS );
-}
-
-int DRM(sg_alloc)( struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg )
-{
- drm_file_t *priv = filp->private_data;
- drm_device_t *dev = priv->dev;
- drm_scatter_gather_t request;
- drm_sg_mem_t *entry;
- unsigned long pages, i, j;
-
- DRM_DEBUG( "%s\n", __FUNCTION__ );
-
- if ( dev->sg )
- return -EINVAL;
-
- if ( copy_from_user( &request,
- (drm_scatter_gather_t *)arg,
- sizeof(request) ) )
- return -EFAULT;
-
- entry = DRM(alloc)( sizeof(*entry), DRM_MEM_SGLISTS );
- if ( !entry )
- return -ENOMEM;
-
- memset( entry, 0, sizeof(*entry) );
-
- pages = (request.size + PAGE_SIZE - 1) / PAGE_SIZE;
- DRM_DEBUG( "sg size=%ld pages=%ld\n", request.size, pages );
-
- entry->pages = pages;
- entry->pagelist = DRM(alloc)( pages * sizeof(*entry->pagelist),
- DRM_MEM_PAGES );
- if ( !entry->pagelist ) {
- DRM(free)( entry, sizeof(*entry), DRM_MEM_SGLISTS );
- return -ENOMEM;
- }
-
- memset(entry->pagelist, 0, pages * sizeof(*entry->pagelist));
-
- entry->busaddr = DRM(alloc)( pages * sizeof(*entry->busaddr),
- DRM_MEM_PAGES );
- if ( !entry->busaddr ) {
- DRM(free)( entry->pagelist,
- entry->pages * sizeof(*entry->pagelist),
- DRM_MEM_PAGES );
- DRM(free)( entry,
- sizeof(*entry),
- DRM_MEM_SGLISTS );
- return -ENOMEM;
- }
- memset( (void *)entry->busaddr, 0, pages * sizeof(*entry->busaddr) );
-
- entry->virtual = vmalloc_32( pages << PAGE_SHIFT );
- if ( !entry->virtual ) {
- DRM(free)( entry->busaddr,
- entry->pages * sizeof(*entry->busaddr),
- DRM_MEM_PAGES );
- DRM(free)( entry->pagelist,
- entry->pages * sizeof(*entry->pagelist),
- DRM_MEM_PAGES );
- DRM(free)( entry,
- sizeof(*entry),
- DRM_MEM_SGLISTS );
- return -ENOMEM;
- }
-
- /* This also forces the mapping of COW pages, so our page list
- * will be valid. Please don't remove it...
- */
- memset( entry->virtual, 0, pages << PAGE_SHIFT );
-
- entry->handle = (unsigned long)entry->virtual;
-
- DRM_DEBUG( "sg alloc handle = %08lx\n", entry->handle );
- DRM_DEBUG( "sg alloc virtual = %p\n", entry->virtual );
-
- for ( i = entry->handle, j = 0 ; j < pages ; i += PAGE_SIZE, j++ ) {
- entry->pagelist[j] = vmalloc_to_page((void *)i);
- if (!entry->pagelist[j])
- goto failed;
- SetPageReserved(entry->pagelist[j]);
- }
-
- request.handle = entry->handle;
-
- if ( copy_to_user( (drm_scatter_gather_t *)arg,
- &request,
- sizeof(request) ) ) {
- DRM(sg_cleanup)( entry );
- return -EFAULT;
- }
-
- dev->sg = entry;
-
-#if DEBUG_SCATTER
- /* Verify that each page points to its virtual address, and vice
- * versa.
- */
- {
- int error = 0;
-
- for ( i = 0 ; i < pages ; i++ ) {
- unsigned long *tmp;
-
- tmp = page_address( entry->pagelist[i] );
- for ( j = 0 ;
- j < PAGE_SIZE / sizeof(unsigned long) ;
- j++, tmp++ ) {
- *tmp = 0xcafebabe;
- }
- tmp = (unsigned long *)((u8 *)entry->virtual +
- (PAGE_SIZE * i));
- for( j = 0 ;
- j < PAGE_SIZE / sizeof(unsigned long) ;
- j++, tmp++ ) {
- if ( *tmp != 0xcafebabe && error == 0 ) {
- error = 1;
- DRM_ERROR( "Scatter allocation error, "
- "pagelist does not match "
- "virtual mapping\n" );
- }
- }
- tmp = page_address( entry->pagelist[i] );
- for(j = 0 ;
- j < PAGE_SIZE / sizeof(unsigned long) ;
- j++, tmp++) {
- *tmp = 0;
- }
- }
- if (error == 0)
- DRM_ERROR( "Scatter allocation matches pagelist\n" );
- }
-#endif
- return 0;
+#ifndef _DRM_SCATTER_H_
+#define _DRM_SCATTER_H_
- failed:
- DRM(sg_cleanup)( entry );
- return -ENOMEM;
-}
-
-int DRM(sg_free)( struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg )
-{
- drm_file_t *priv = filp->private_data;
- drm_device_t *dev = priv->dev;
- drm_scatter_gather_t request;
- drm_sg_mem_t *entry;
-
- if ( copy_from_user( &request,
- (drm_scatter_gather_t *)arg,
- sizeof(request) ) )
- return -EFAULT;
-
- entry = dev->sg;
- dev->sg = NULL;
-
- if ( !entry || entry->handle != request.handle )
- return -EINVAL;
- DRM_DEBUG( "sg free virtual = %p\n", entry->virtual );
+/**
+ * Scatter/gather memory entry.
+ *
+ * This structure is used by sg_alloc() sg_free() to store all the necessary
+ * information about a scatter/gather memory block.
+ *
+ * Also one instance of this structure is used to hold the user-space
+ * allocation done via the sg_alloc_ioctl() and sg_free_ioctl() ioctl's.
+ */
+typedef struct drm_sg_mem {
+ unsigned long handle;
+ void *virtual; /**< virtual address */
+ int pages;
+ struct page **pagelist;
+ dma_addr_t *busaddr; /**< bus address */
+} drm_sg_mem_t;
+
+
+/** \name Prototypes */
+/[EMAIL PROTECTED]/
+extern void DRM(sg_free)( drm_sg_mem_t *entry );
+extern drm_sg_mem_t * DRM(sg_alloc)( unsigned long size );
+extern int DRM(sg_alloc_ioctl)(struct inode *inode, struct file *filp, unsigned int
cmd, unsigned long arg);
+extern int DRM(sg_free_ioctl)(struct inode *inode, struct file *filp, unsigned int
cmd, unsigned long arg);
+extern void DRM(sg_cleanup)(drm_device_t *dev);
+/[EMAIL PROTECTED]/
- DRM(sg_cleanup)( entry );
- return 0;
-}
+#endif
Index: r128_drv.c
===================================================================
RCS file:
/cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/r128_drv.c,v
retrieving revision 1.44
diff -u -r1.44 r128_drv.c
--- r128_drv.c 5 Jul 2002 08:31:10 -0000 1.44
+++ r128_drv.c 2 Jun 2003 23:54:45 -0000
@@ -52,4 +52,4 @@
#include "drm_proc.h"
#include "drm_vm.h"
#include "drm_stub.h"
-#include "drm_scatter.h"
+#include "drm_scatter_tmp.h"
Index: radeon_drv.c
===================================================================
RCS file:
/cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/radeon_drv.c,v
retrieving revision 1.18
diff -u -r1.18 radeon_drv.c
--- radeon_drv.c 27 May 2003 00:37:33 -0000 1.18
+++ radeon_drv.c 2 Jun 2003 23:54:45 -0000
@@ -53,4 +53,4 @@
#include "drm_proc.h"
#include "drm_vm.h"
#include "drm_stub.h"
-#include "drm_scatter.h"
+#include "drm_scatter_tmp.h"
/**
* \file drm_scatter_tmp.h
* IOCTLs to manage scatter/gather memory
*
* \author Gareth Hughes <[EMAIL PROTECTED]>
*/
/*
* 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.
*/
#if HAVE_SG
#define __NO_VERSION__
#include <linux/config.h>
#include <linux/vmalloc.h>
#include "drmP.h"
/**
* Define to non-zero to verify that each page points to its virtual address,
* and vice versa, in sg_alloc().
*/
#define DEBUG_SCATTER 0
/**
* Free scatter/gather memory.
*
* \param entry scatter/gather memory entry to free, as returned by sg_alloc().
*/
void DRM(sg_free)( drm_sg_mem_t *entry )
{
struct page *page;
int i;
for ( i = 0 ; i < entry->pages ; i++ ) {
page = entry->pagelist[i];
if ( page )
ClearPageReserved( page );
}
vfree( entry->virtual );
DRM(free)( entry->busaddr,
entry->pages * sizeof(*entry->busaddr),
DRM_MEM_PAGES );
DRM(free)( entry->pagelist,
entry->pages * sizeof(*entry->pagelist),
DRM_MEM_PAGES );
DRM(free)( entry,
sizeof(*entry),
DRM_MEM_SGLISTS );
}
/**
* Allocate scatter/gather memory.
*
* \param size size of memory to allocate.
* \return pointer to a drm_sg_mem structure on success or NULL on failure.
*/
drm_sg_mem_t * DRM(sg_alloc)( unsigned long size )
{
drm_sg_mem_t *entry;
unsigned long pages, i, j;
DRM_DEBUG( "%s\n", __FUNCTION__ );
entry = DRM(alloc)( sizeof(*entry), DRM_MEM_SGLISTS );
if ( !entry )
return NULL;
memset( entry, 0, sizeof(*entry) );
pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
DRM_DEBUG( "sg size=%ld pages=%ld\n", size, pages );
entry->pages = pages;
entry->pagelist = DRM(alloc)( pages * sizeof(*entry->pagelist),
DRM_MEM_PAGES );
if ( !entry->pagelist ) {
DRM(free)( entry, sizeof(*entry), DRM_MEM_SGLISTS );
return NULL;
}
memset(entry->pagelist, 0, pages * sizeof(*entry->pagelist));
entry->busaddr = DRM(alloc)( pages * sizeof(*entry->busaddr),
DRM_MEM_PAGES );
if ( !entry->busaddr ) {
DRM(free)( entry->pagelist,
entry->pages * sizeof(*entry->pagelist),
DRM_MEM_PAGES );
DRM(free)( entry,
sizeof(*entry),
DRM_MEM_SGLISTS );
return NULL;
}
memset( (void *)entry->busaddr, 0, pages * sizeof(*entry->busaddr) );
entry->virtual = vmalloc_32( pages << PAGE_SHIFT );
if ( !entry->virtual ) {
DRM(free)( entry->busaddr,
entry->pages * sizeof(*entry->busaddr),
DRM_MEM_PAGES );
DRM(free)( entry->pagelist,
entry->pages * sizeof(*entry->pagelist),
DRM_MEM_PAGES );
DRM(free)( entry,
sizeof(*entry),
DRM_MEM_SGLISTS );
return NULL;
}
/* This also forces the mapping of COW pages, so our page list
* will be valid. Please don't remove it...
*/
memset( entry->virtual, 0, pages << PAGE_SHIFT );
entry->handle = (unsigned long)entry->virtual;
DRM_DEBUG( "sg alloc handle = %08lx\n", entry->handle );
DRM_DEBUG( "sg alloc virtual = %p\n", entry->virtual );
for ( i = entry->handle, j = 0 ; j < pages ; i += PAGE_SIZE, j++ ) {
entry->pagelist[j] = vmalloc_to_page((void *)i);
if (!entry->pagelist[j])
goto failed;
SetPageReserved(entry->pagelist[j]);
}
#if DEBUG_SCATTER
{
int error = 0;
for ( i = 0 ; i < pages ; i++ ) {
unsigned long *tmp;
tmp = page_address( entry->pagelist[i] );
for ( j = 0 ;
j < PAGE_SIZE / sizeof(unsigned long) ;
j++, tmp++ ) {
*tmp = 0xcafebabe;
}
tmp = (unsigned long *)((u8 *)entry->virtual +
(PAGE_SIZE * i));
for( j = 0 ;
j < PAGE_SIZE / sizeof(unsigned long) ;
j++, tmp++ ) {
if ( *tmp != 0xcafebabe && error == 0 ) {
error = 1;
DRM_ERROR( "Scatter allocation error, "
"pagelist does not match "
"virtual mapping\n" );
}
}
tmp = page_address( entry->pagelist[i] );
for(j = 0 ;
j < PAGE_SIZE / sizeof(unsigned long) ;
j++, tmp++) {
*tmp = 0;
}
}
if (error == 0)
DRM_ERROR( "Scatter allocation matches pagelist\n" );
}
#endif
return entry;
failed:
DRM(sg_free)( entry );
return NULL;
}
/**********************************************************************/
/** \name Ioctl's
*
* These expose the scatter/gather memory functions to user-space,
* doing the necessary parameter verification and book-keeping to avoid memory
* leaks.
*
* In the current implementation only one scatter/gather memory block can be
* allocated per device at a given instance.
*
* The information about the currently allocated scatter/gather memory block is
* stored in drm_device_t::sg.
*/
/[EMAIL PROTECTED]/
/**
* Wrapper ioctl around sg_alloc().
*
* \param inode device inode.
* \param filp file pointer.
* \param cmd command.
* \param arg user argument, pointing to a drm_scatter_gather structure.
* \return zero on success or a negative number on failure.
*/
int DRM(sg_alloc_ioctl)( struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg )
{
drm_file_t *priv = filp->private_data;
drm_device_t *dev = priv->dev;
drm_scatter_gather_t request;
drm_sg_mem_t *entry;
DRM_DEBUG( "%s\n", __FUNCTION__ );
if ( dev->sg )
return -EINVAL;
if ( copy_from_user( &request,
(drm_scatter_gather_t *)arg,
sizeof(request) ) )
return -EFAULT;
entry = DRM(sg_alloc)( request.size );
if ( !entry )
return -ENOMEM;
request.handle = entry->handle;
if ( copy_to_user( (drm_scatter_gather_t *)arg,
&request,
sizeof(request) ) ) {
DRM(sg_free)( entry );
return -EFAULT;
}
dev->sg = entry;
return 0;
}
/** Wrapper around sg_free().
*
* \param inode device inode.
* \param filp file pointer.
* \param cmd command.
* \param arg user argument, pointing to a drm_scatter_gather structure.
* \return zero on success or a negative number on failure.
*/
int DRM(sg_free_ioctl)( struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg )
{
drm_file_t *priv = filp->private_data;
drm_device_t *dev = priv->dev;
drm_scatter_gather_t request;
drm_sg_mem_t *entry;
if ( copy_from_user( &request,
(drm_scatter_gather_t *)arg,
sizeof(request) ) )
return -EFAULT;
entry = dev->sg;
dev->sg = NULL;
if ( !entry || entry->handle != request.handle )
return -EINVAL;
DRM_DEBUG( "sg free virtual = %p\n", entry->virtual );
DRM(sg_free)( entry );
return 0;
}
/[EMAIL PROTECTED]/
/**
* Called by takedown() to free the scatter/gather memory resources associated
* with a given device, i.e., to free drm_device_t::sg.
*/
void DRM(sg_cleanup)(drm_device_t *dev)
{
if ( dev->sg ) {
DRM(sg_free)( dev->sg );
dev->sg = NULL;
}
}
#endif
