[MPlayer-dev-eng] [PATCH] mga_vid for 2.6 - updated...again

Attila Kinali attila at kinali.ch
Sun Aug 8 10:36:40 CEST 2004


On Tue, Jul 13, 2004 at 10:50:10PM +0200, Ferdinand O. Tempel wrote:
> Attached you'll find a new and improved patch for getting mga_vid to
> work on 2.6 kernels. This time it won't break compiling on non-devfs 2.4
> kernels (this time I tested). Kudos to Reimar for that one. I can't
> vouch for 2.4 kernels *with* devfs, but I hear it already worked on
> that.
> This one is tested on:
> - 2.6.7
> - 2.6.7 + devfs
> - 2.4.26

The test wit 2.4.x + devfs is missing, see below.

Also you fixed only a few of the things i pointed out.

> --- main/drivers/mga_vid.c	2004-07-07 20:56:26.000000000 +0200
> +++ mga-work/mga_vid.c	2004-07-13 22:34:39.000000000 +0200
> @@ -1,15 +1,22 @@
> +#define MODULENAME "mga_vid"
> +
>  //#define CRTC2
>  
>  // YUY2 support (see config.format) added by A'rpi/ESP-team
>  // double buffering added by A'rpi/ESP-team
>  // brightness/contrast introduced by eyck
>  // multiple card support by Attila Kinali <attila at kinali.ch>
> +// ported to the 2.6 series kernels by F.O. Tempel
> +//  thankfully using the ground work done by Ed Sweetman (for the devfs work)
> +//  and Gergely Nagy for pushing into the right direction with his patch for 2.6.0-test1
>  
>  // Set this value, if autodetection fails! (video ram size in megabytes)
>  // #define MGA_MEMORY_SIZE 16
>  
>  //#define MGA_ALLOW_IRQ
>  
> +#define MGA_MAX_CARDS 16

Why do you move this define ?

> @@ -117,7 +134,8 @@
>  #ifndef min
>  #define min(x,y) (((x)<(y))?(x):(y))
>  #endif
> -
> +// These functions are provided by the 2.6.0 kernel these days.
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,4,0)

This is redundand, if the second if matches, the first will
never match.
If you care abou the inclusion of simple_strtol at line 150
you should remove there the check for >2.5.5 (yes, remove,
no need to support a testing kernel)

>  #include <linux/ctype.h>
>  
> @@ -348,17 +366,30 @@
>  #define ICLEAR	    0x1e18
>  #define STATUS      0x1e14
>  
> -
> -// global devfs handle for /dev/mga_vid
> +/* Global handles for cdev and devfs */
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +static struct cdev *mga_vid_cdev;
> +static dev_t mga_cdev_handle;
> +#ifdef CONFIG_DEVFS_FS 
> +typedef struct devfs_entry *devfs_handle_t;
> +devfs_handle_t dev_handle = NULL;

This should be static.

> +#endif
> +#else
>  #ifdef CONFIG_DEVFS_FS
> -static devfs_handle_t dev_handle = NULL;
> +	devfs_handle_t dev_handle = NULL;

Don't remove the static here.

> +#endif
>  #endif
>  
>  // card local config
>  typedef struct mga_card_s {
>  
>  // local devfs handle for /dev/mga_vidX
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
>  #ifdef CONFIG_DEVFS_FS
> +	struct devfs_entry *devfs_handle_t;
> +#endif
> +#endif
> +#if CONFIG_DEVFS_FS
>  	devfs_handle_t dev_handle;
>  #endif
>  
> @@ -397,7 +428,7 @@
>  	int next_frame; 
>  } mga_card_t;
>  
> -#define MGA_MAX_CARDS 16
> +static int mga_max_cards = MGA_MAX_CARDS;
>  // this is used as init value for the parameter arrays
>  // it should have exactly MGA_MAX_CARDS elements
>  #define MGA_MAX_CARDS_INIT_ARRAY {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
> @@ -411,11 +442,19 @@
>  static int mga_contrast[MGA_MAX_CARDS] = MGA_MAX_CARDS_INIT_ARRAY;
>  static int mga_top_reserved[MGA_MAX_CARDS] = MGA_MAX_CARDS_INIT_ARRAY;
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +module_param_array(mga_ram_size, int,  mga_max_cards, 0);
> +module_param_array(mga_top_reserved, int,  mga_max_cards, 0);
> +module_param_array(mga_brightness, int, mga_max_cards, 0);
> +module_param_array(mga_contrast, int, mga_max_cards, 0);

Is there a reason why you use here a variable instead of the
define (aka MGA_MAX_CARDS) ? It's redundand and just usefull
for code obfuscation.

> +module_param(major, int, 0);
> +#else
>  MODULE_PARM(mga_ram_size, "1-" __MODULE_STRING(MGA_MAX_CARDS) "i");
>  MODULE_PARM(mga_top_reserved, "1-" __MODULE_STRING(MGA_MAX_CARDS) "i");
>  MODULE_PARM(mga_brightness, "1-" __MODULE_STRING(MGA_MAX_CARDS) "i");
>  MODULE_PARM(mga_contrast, "1-" __MODULE_STRING(MGA_MAX_CARDS) "i");
>  MODULE_PARM(major, "i");
> +#endif
>  
>  #ifdef CRTC2
>  static void crtc2_frame_sel(mga_card_t * card, int frame)
> @@ -1100,7 +1140,7 @@
>  
>  #ifdef MGA_ALLOW_IRQ
>  
> -static void enable_irq(mga_card_t * card){
> +static void mga_enable_irq(mga_card_t * card){

Don't change the names of static functions.

>  	long int cc;
>  
>  	cc = readl(card->mmio_base + IEN);
> @@ -1116,16 +1156,18 @@
>  
>  }
>  
> -static void disable_irq(mga_card_t * card){
> +static void mga_disable_irq(mga_card_t * card){

Don't change the names of static functions.

>  
>  	writeb(0x11, card->mmio_base + CRTCX);
>  	writeb(0x20, card->mmio_base + CRTCD);  /* clear 0, enable off */
>  
>  }
>  
> -static void mga_handle_irq(int irq, void *dev_id, struct pt_regs *pregs) {
> +static int mga_handle_irq(int irq, void *dev_id, struct pt_regs *pregs) {

void -> int change requires a kernel version check.

>  //	static int frame=0;
> -//	static int counter=0;
> +#ifdef MP_DEBUG
> +	static int counter=0;
> +#endif
>  	long int cc;
>  	mga_card_t * card = dev_id;
>  
> @@ -1136,7 +1178,7 @@
>  	// check whether the interrupt is really for us (irq sharing)
>  	if ( irq != -1 ) {
>  		cc = readl(card->mmio_base + STATUS);
> -		if ( ! (cc & 0x10) ) return;  /* vsyncpen */
> +		if ( ! (cc & 0x10) ) return 0;  /* vsyncpen */

Use the defined value as return value instead of 0.

>  // 		debug_irqcnt++;
>  	} 
>  
> @@ -1152,15 +1194,15 @@
>  // i han echt kei ahnig was das obe heisse s?ll
>  	crtc2_frame_sel(card->next_frame);
>  #endif
> -	
> -#if 0
> +
> +#ifdef MP_DEBUG
>  	++counter;
>  	if(!(counter&63)){
>  	    printk("mga irq counter = %d\n",counter);
>  	}
>  #endif

Dont change this #if 0 to #ifdev MP_DEBUG, it may have negative
effects on system, beside flooding the kernel log.

>  
> -//    } else {
> +	//    } else {
>  //	debug_irqignore = 1;
>  //    }
>  
> @@ -1173,7 +1215,7 @@
>  //	writel( card->regs.besglobctl, card->mmio_base + BESGLOBCTL);
>  
>  
> -	return;
> +	return 0;

Again return the correct value instead of 0.

>  
>  }
>  
> @@ -1257,7 +1299,7 @@
>  				mga_vid_write_regs(card, 0);
>  			}
>  #ifdef MGA_ALLOW_IRQ
> -			if ( card->irq != -1 ) enable_irq(card);
> +			if ( card->irq != -1 ) mga_enable_irq(card);
>  #endif
>  			card->next_frame=0;
>  		break;
> @@ -1268,7 +1310,7 @@
>  #endif			
>  			card->vid_src_ready = 0;   
>  #ifdef MGA_ALLOW_IRQ
> -			if ( card->irq != -1 ) disable_irq(card);
> +			if (card->irq != -1 ) mga_disable_irq(card);

Two times unnecessary name change.

>  #endif
>  			card->regs.besctl &= ~1;
>                          card->regs.besglobctl &= ~(1<<6);  // UYVY format selected
  
> -	return(0);
> +	return 0;

Unnecessary cosmetic.

>  }
>  
>  static int mga_vid_release(struct inode *inode, struct file *file)
> @@ -1539,8 +1591,10 @@
>  		return(-EBUSY);
>  
>  	card->vid_in_use = 1;
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
>  	MOD_INC_USE_COUNT;
> -	return(0);
> +#endif
> +	return 0;

Unnecessary cosmetic.

>  }
>  
>  #if LINUX_VERSION_CODE >= 0x020400
> @@ -1573,10 +1627,6 @@
>  static void cards_init(mga_card_t * card, struct pci_dev * dev, int card_number, int is_g400)
>  {
>  	unsigned int card_option;
> -// temp buffer for device filename creation used only by devfs
> -#ifdef CONFIG_DEVFS_FS
> -	char buffer[16];
> -#endif
>  
>  	memset(card,0,sizeof(mga_card_t));
>  	card->irq = -1;
> @@ -1647,10 +1697,11 @@
>  //		    case 0x13:  card->ram_size = 8; break;
>  		    default: card->ram_size = 8;
>  		}
> -	    } 
> +} 
> +
>  #if 0
>  //	    printk("List resources -----------\n");
> -	    for(temp=0;temp<DEVICE_COUNT_RESOURCE;temp++){
> +	    for(int temp=0;temp<DEVICE_COUNT_RESOURCE;temp++){

for(int blah... may not compile with older compilers.
Leave this as it was.

>  	        struct resource *res=&dev->resource[temp];
>  	        if(res->flags){
>  	          int size=(1+res->end-res->start)>>20;
> @@ -1665,7 +1716,6 @@
>  #endif
>          }
>  
> -
>  #ifdef MGA_ALLOW_IRQ
>  	if ( card->irq != -1 ) {
>  		int tmp = request_irq(card->irq, mga_handle_irq, SA_INTERRUPT | SA_SHIRQ, "Syncfb Time Base", card);
> @@ -1683,16 +1733,15 @@
>  		printk(KERN_INFO "syncfb (mga): IRQ disabled in mga_vid.c\n");
>  		card->irq=-1;
>  #endif
> -
> -	// register devfs, let the kernel give us major and minor numbers
> -#ifdef CONFIG_DEVFS_FS
> -	snprintf(buffer, 16, "mga_vid%d", card_number);
> +// register devfs, let the kernel give us major and minor numbers
> +#if CONFIG_DEVFS_FS && LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
> +	char buffer[16];
> +	snprintf(buffer, 16, "%s%d", MODULENAME, card_number);
>  	card->dev_handle = devfs_register(NULL, buffer, DEVFS_FL_AUTO_DEVNUM,
>  					0, 0,
>  					S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IFCHR,
>  					&mga_vid_fops, card);
>  #endif

Where is the code for 2.4 devfs ?
This will only work on 2.6 and is thus a los on functionality.

> -
>  }
>  
>  /* 
> @@ -1701,73 +1750,108 @@
>  
>  static int mga_vid_initialize(void)
>  {
> - 	int i;
> -
> -//	printk(KERN_INFO "Matrox MGA G200/G400 YUV Video interface v0.01 (c) Aaron Holtzman \n");
> + 		int i;
>  	printk(KERN_INFO "Matrox MGA G200/G400/G450/G550 YUV Video interface v2.01 (c) Aaron Holtzman & A'rpi\n");
> -
> -	for(i = 0; i < MGA_MAX_CARDS; i++)
> +	
> +	if(mga_vid_find_card()) 
>  	{
> -		if (mga_ram_size[i]) {
> -			if (mga_ram_size[i]<4 || mga_ram_size[i]>64) {
> -				printk(KERN_ERR "mga_vid: invalid RAMSIZE: %d MB\n", mga_ram_size[i]);
> -				return -EINVAL;
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +		/* Have the kernel generate a major device number */
> +		if(major == 0) 
> +		{
> +			if(!alloc_chrdev_region(&mga_cdev_handle, 0, mga_cards_num, "mga_vid")) {
> +				major = MAJOR(mga_cdev_handle);
> +				printk(KERN_INFO "mga_vid: using major: %d (dynamically alocated!)\n", major);
> +			}
> +		} 
> +		else
> +		{
> +			mga_cdev_handle = MKDEV(major,0);
> +			if(!register_chrdev_region(mga_cdev_handle, mga_cards_num, "mga_vid"))
> +				printk(KERN_INFO "mga_vid: using major: %d (assigned or default!)\n", major);
> +		}
> +		/* Allocate a cdev for this character device, and fill in some parameters it needs */
> +		mga_vid_cdev = cdev_alloc();
> +		mga_vid_cdev->owner = THIS_MODULE;
> +		strcpy(mga_vid_cdev->kobj.name, MODULENAME);
> +		mga_vid_cdev->ops = &mga_vid_fops;
> +		/* Add this character device to the system */
> +		cdev_add(mga_vid_cdev, mga_cdev_handle, mga_cards_num);
> +#endif
> +		for(i = 0; i < mga_cards_num; i++)
> +		{
> +#if CONFIG_DEVFS_FS && LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +			/* Create the device, and register a symlink for the first card found. 
> +			 * Lets not break default behaviour, eh? */
> +			devfs_mk_cdev(MKDEV(major,i), S_IFCHR | S_IRUSR | S_IRGRP | S_IWUSR, "video/mga_vid%d", i);

Is there a reason why you put the device files into video/ ?
Is there a standard somewhere ?
And if, imho video/ is missleading, this is a graphics card driver, not
a video device driver.

> +			if( i == 0 ) {
> +				devfs_mk_symlink(MODULENAME,"video/mga_vid0");
> +			}
> +#endif
> +			if (mga_ram_size[i]) {
> +				if (mga_ram_size[i]<4 || mga_ram_size[i]>64) {
> +					printk(KERN_ERR "mga_vid: invalid RAMSIZE: %d MB\n", mga_ram_size[i]);
> +					return -EINVAL;
> +				}
>  			}
>  		}
> -	}
>  	
> -	if(register_chrdev(major, "mga_vid", &mga_vid_fops))
> -	{
> -		printk(KERN_ERR "mga_vid: unable to get major: %d\n", major);
> -		return -EIO;
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
> +		if(register_chrdev(major, MODULENAME, &mga_vid_fops))
> +		{
> +			printk(KERN_ERR "mga_vid: unable to get major: %d\n", major);
> +			return -EIO;
> +		}
> +#endif
> +		return 0;
>  	}
> -
> -	if (!mga_vid_find_card())
> +	else
>  	{
> -		printk(KERN_ERR "mga_vid: no supported devices found\n");
> -		unregister_chrdev(major, "mga_vid");
> -		return -EINVAL;
> +		return -EFAULT;
>  	}
> -#ifdef CONFIG_DEVFS_FS
> -	  else {
> -		// we assume that this always succeedes
> -		dev_handle = devfs_register(NULL, "mga_vid", DEVFS_FL_AUTO_DEVNUM,
> -		                            0,0,
> -		                            S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IFCHR,
> -		                            &mga_vid_fops, mga_cards[0]);
> -	}
> -#endif
> -
> -	return(0);
>  }
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +module_init(mga_vid_initialize);
> +#else
>  int init_module(void)
>  {
>  	return mga_vid_initialize();
>  }
> +#endif
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +static void mga_cleanup_module(void)
> +#else
>  void cleanup_module(void)
> +#endif

What is the sense of this #if ? it's a static function and thus
only visible in this file anyways. Beside you pass the address
of the function to the cernel by module_exit() anyways.

>  {
>  	int i;
>  	mga_card_t * card;
>  
> +	printk(KERN_INFO "mga_vid: Cleaning up module\n");
>  	for (i = 0; i < MGA_MAX_CARDS; i++)
>  	{
>  		card = mga_cards[i];
>  		if(card)
>  		{
>  #ifdef MGA_ALLOW_IRQ
> -			if (card->irq != -1)
> -				free_irq(card->irq, &(card->irq));
> +			if ( card->irq != -1)
> +//				free_irq(card->irq, &(card->irq));
> +					free_irq(card->irq, card);

If free_irq changed with the 2.6 kernel, then you need here an other
#if. Although i must admit, that the previous version looks bogus.
Have to check that.

>  #endif
>  
>  			if(card->mmio_base)
>  				iounmap(card->mmio_base);
>  			if(card->param_buff)
>  				kfree(card->param_buff);
> -#ifdef CONFIG_DEVFS_FS
> +#ifdef CONFIG_DEVFS_FS 
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +			devfs_remove("video/%s%d",MODULENAME, i);
> +#else
>  			if(card->dev_handle) devfs_unregister(card->dev_handle);
>  #endif
> +#endif
>  
>  			kfree(card);
>  			mga_cards[i]=NULL;


Do you want to continue with this patch or shall i clean it up ?



			Attila Kinali




More information about the MPlayer-dev-eng mailing list