Re: Update direct-rendering to current DRI CVS tree.

Re: Update direct-rendering to current DRI CVS tree.

[Dave Jones]

On Sun, Mar 30, 2003 at 06:34:37AM +0000, Linux Kernel wrote:

This bit seems to be backing out a memleak fix..
(takedown doesn't kfree 'device' & 'minor' that I can see.)

 > diff -Nru a/drivers/char/drm/drm_drv.h b/drivers/char/drm/drm_drv.h
 > --- a/drivers/char/drm/drm_drv.h	Sat Mar 29 23:12:35 2003
 > +++ b/drivers/char/drm/drm_drv.h	Sat Mar 29 23:12:35 2003
 > @@ -576,13 +578,9 @@
 >  		memset( (void *)dev, 0, sizeof(*dev) );
 >  		dev->count_lock = SPIN_LOCK_UNLOCKED;
 >  		sema_init( &dev->struct_sem, 1 );
 > -		init_timer( &dev->timer );
 > -		init_waitqueue_head( &dev->context_wait );
 >  
 > -		if ((DRM(minor)[i] = DRM(stub_register)(DRIVER_NAME, &DRM(fops),dev)) < 0) {
 > -			retcode = -EPERM;
 > -			goto fail_reg;
 > -		}
 > +		if ((DRM(minor)[i] = DRM(stub_register)(DRIVER_NAME, &DRM(fops),dev)) < 0)
 > +			return -EPERM;
 >  		dev->device = MKDEV(DRM_MAJOR, DRM(minor)[i] );
 >  		dev->name   = DRIVER_NAME;
 >  
 > @@ -591,8 +589,9 @@
 >  #if __MUST_HAVE_AGP
 >  		if ( dev->agp == NULL ) {
 >  			DRM_ERROR( "Cannot initialize the agpgart module.\n" );
 > -			retcode = -ENOMEM;
 > -			goto fail;
 > +			DRM(stub_unregister)(DRM(minor)[i]);
 > +			DRM(takedown)( dev );
 > +			return -ENOMEM;
 >  		}
 >  #endif
 >  #if __REALLY_HAVE_MTRR
 > @@ -608,7 +607,9 @@
 >  		retcode = DRM(ctxbitmap_init)( dev );
 >  		if( retcode ) {
 >  			DRM_ERROR( "Cannot allocate memory for context bitmap.\n" );
 > -			goto fail;
 > +			DRM(stub_unregister)(DRM(minor)[i]);
 > +			DRM(takedown)( dev );
 > +			return retcode;
 >  		}
 >  #endif
 >  		DRM_INFO( "Initialized %s %d.%d.%d %s on minor %d\n",
 > @@ -623,17 +624,6 @@
 >  	DRIVER_POSTINIT();
 >  
 >  	return 0;
 > -
 > -#if (__REALLY_HAVE_AGP && __MUST_HAVE_AGP) || __HAVE_CTX_BITMAP
 > -fail:
 > -	DRM(stub_unregister)(DRM(minor)[i]);
 > -	DRM(takedown)( dev );
 > -#endif
 > -
 > -fail_reg:
 > -	kfree (DRM(device));
 > -	kfree (DRM(minor));
 > -	return retcode;
 >  }
 >  
 >  /* drm_cleanup is called via cleanup_module at module unload time.

...

 > @@ -41,7 +42,7 @@
 >  
 >  /* malloc/free without the overhead of DRM(alloc) */
 >  #define DRM_MALLOC(x) kmalloc(x, GFP_KERNEL)
 > -#define DRM_FREE(x) kfree(x)
 > +#define DRM_FREE(x,size) kfree(x)

eww. wtf is this for ? Some cross-OS compataiblity gunk ?

 > diff -Nru a/drivers/char/drm/i830_dma.c b/drivers/char/drm/i830_dma.c
 > --- a/drivers/char/drm/i830_dma.c	Sat Mar 29 23:12:35 2003
 > +++ b/drivers/char/drm/i830_dma.c	Sat Mar 29 23:12:35 2003
 > @@ -46,8 +47,6 @@
 >  #define I830_BUF_UNMAPPED 0
 >  #define I830_BUF_MAPPED   1
 >  
 > -#define RING_LOCALS	unsigned int outring, ringmask; volatile char *virt;
 > -
 >  #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,4,2)
 >  #define down_write down
 >  #define up_write up

#if can go, like it did in other parts of the patch.

I would read through the other 10 billion lines, but I lost
enthusiasm..  More frequent merges may help here.
With the two trees in sync, would it help to have DRI cvs changes
tracked in a bk tree you could pull from ?

It could maybe even be scripted. Doing the opposite of
Larry's bk->cvs gizmo should be easier due to the linear
nature of cvs.

		Dave

Re: Update direct-rendering to current DRI CVS tree.

[Nicholas Wourms]

Dave Jones wrote:
>  >  #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,4,2)
>  >  #define down_write down
>  >  #define up_write up
> 
> #if can go, like it did in other parts of the patch.

What will replace it?  If you intend to keep the two 
projects in sync and easy to update, I'm afraid that it will 
call for putting all the later (>=2.4.16) #if's back, as 
well as adding gaurds for the current 2.5-specific 
scheduling/worqueue changes.  Unless, of course, the dri 
maintainers are willing to make TRUNK incompatible with 
linux-2.4...

Cheers,
Nicholas

Re: Update direct-rendering to current DRI CVS tree.

[Christoph Hellwig]

On Sun, Mar 30, 2003 at 09:14:58AM -0500, Nicholas Wourms wrote:
> Dave Jones wrote:
> >  >  #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,4,2)
> >  >  #define down_write down
> >  >  #define up_write up
> > 
> > #if can go, like it did in other parts of the patch.
> 
> What will replace it?

Nothing.  2.4.0/2.4.1/2.4.2 are too buggy to be used.

Re: Update direct-rendering to current DRI CVS tree.

[Linus Torvalds]

On Sun, 30 Mar 2003, Dave Jones wrote:
> 
> This bit seems to be backing out a memleak fix..

Well, yes and no. I looked at the code, and decided that the memleak fix 
was hottibly bogus.

Look at how the allocations are done inside the loop: they are INSIDE A 
LOOP.

Look at what happens with the memory leak fix if the loop fails after
having done <n> iterations: it will only unregister the last (failing)
iteration, but it will then free stuff that the earlier (successful)
iterations still depend on.

>  > @@ -41,7 +42,7 @@
>  >  
>  >  /* malloc/free without the overhead of DRM(alloc) */
>  >  #define DRM_MALLOC(x) kmalloc(x, GFP_KERNEL)
>  > -#define DRM_FREE(x) kfree(x)
>  > +#define DRM_FREE(x,size) kfree(x)
> 
> eww. wtf is this for ? Some cross-OS compataiblity gunk ?

I assume it's for *BSD.

>  >  #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,4,2)
>  >  #define down_write down
>  >  #define up_write up
> 
> #if can go, like it did in other parts of the patch.

Only if _you_ are willing to sync the DRM tree. 

Since I'm only doing the DRM syncs occasionally, I want to keep 
unnecessary differences between the kernel and DRM to a minimum. Which 
means that I do remove gunk that I find stupid and ugly (I've removed 
#ifdef's in the code before), but not stuff that looks like reasonable 
compatibility stuff.

> I would read through the other 10 billion lines, but I lost
> enthusiasm..

Ehh.. I _did_ go through the other lines, and was actually involved with 
one of the more spread-out changes (ie suggesting the fixed locking 
context thing). As far as I can tell the patch is good. I'd still like 
more eyes on the result, but the eyes should look a bit more closely..

		Linus