Re: [PATCH] cirrusfb: it lives!

Re: [PATCH] cirrusfb: it lives!

[Jeff Garzik]

David Eger wrote:
> Dear Andrew,
> 
> This patch brings the cirrusfb driver up to date with 2.6.  cirrusfb
> has suffered bit rot like you wouldn't believe (last updated... 2.3.x era?).
> The driver will now compile again, and you can change to a high resolution 
> text mode with stty.  Known defects: doesn't play nice with X, nor fbset.
> Nonetheless, please apply to -mm and forward to mainline.

Groovy :)

If I am listed in MAINTAINERS somewhere, feel free to remove me... :)



> cirrusfb: port from Linux 2.4 to Linux 2.6 fb API
> 	aka The Big Overhaul(tm)
> 
> Signed-off-by: David Eger <eger@havoc.gtf.org>
> 
> diff -Nru a/drivers/video/Kconfig b/drivers/video/Kconfig
> --- a/drivers/video/Kconfig	2004-06-22 16:12:19 -04:00
> +++ b/drivers/video/Kconfig	2004-06-22 16:12:19 -04:00
> @@ -40,7 +40,7 @@
>  
>  config FB_CIRRUS
>  	tristate "Cirrus Logic support"
> -	depends on FB && (AMIGA || PCI) && BROKEN
> +	depends on FB && (AMIGA || PCI)
>  	---help---
>  	  This enables support for Cirrus Logic GD542x/543x based boards on
>  	  Amiga: SD64, Piccolo, Picasso II/II+, Picasso IV, or EGS Spectrum.
> diff -Nru a/drivers/video/Makefile b/drivers/video/Makefile
> --- a/drivers/video/Makefile	2004-06-22 16:12:19 -04:00
> +++ b/drivers/video/Makefile	2004-06-22 16:12:19 -04:00
> @@ -39,7 +39,7 @@
>  obj-$(CONFIG_FB_OF)               += offb.o cfbfillrect.o cfbimgblt.o cfbcopyarea.o
>  obj-$(CONFIG_FB_IMSTT)            += imsttfb.o cfbimgblt.o
>  obj-$(CONFIG_FB_RETINAZ3)         += retz3fb.o
> -obj-$(CONFIG_FB_CIRRUS)		  += cirrusfb.o
> +obj-$(CONFIG_FB_CIRRUS)		  += cirrusfb.o cfbfillrect.o cfbimgblt.o cfbcopyarea.o
>  obj-$(CONFIG_FB_TRIDENT)	  += tridentfb.o cfbfillrect.o cfbimgblt.o cfbcopyarea.o
>  obj-$(CONFIG_FB_S3TRIO)           += S3triofb.o
>  obj-$(CONFIG_FB_TGA)              += tgafb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o 
> diff -Nru a/drivers/video/cirrusfb.c b/drivers/video/cirrusfb.c
> --- a/drivers/video/cirrusfb.c	2004-06-22 16:12:19 -04:00
> +++ b/drivers/video/cirrusfb.c	2004-06-22 16:12:20 -04:00
> @@ -1,10 +1,13 @@
>  /*
> - * drivers/video/clgenfb.c - driver for Cirrus Logic chipsets
> + * drivers/video/cirrusfb.c - driver for Cirrus Logic chipsets
>   *
>   * Copyright 1999-2001 Jeff Garzik <jgarzik@pobox.com>
>   *
>   * Contributors (thanks, all!)
>   *
> + * 	David Eger:
> + * 	Overhaul for Linux 2.6
> + *
>   *      Jeff Rugen:
>   *      Major contributions;  Motorola PowerStack (PPC and PCI) support,
>   *      GD54xx, 1280x1024 mode support, change MCLK based on VCLK.
> @@ -15,9 +18,9 @@
>   *	Lars Hecking:
>   *	Amiga updates and testing.
>   *
> - * Original clgenfb author:  Frank Neumann
> + * Original cirrusfb author:  Frank Neumann
>   *
> - * Based on retz3fb.c and clgen.c:
> + * Based on retz3fb.c and cirrusfb.c:
>   *      Copyright (C) 1997 Jes Sorensen
>   *      Copyright (C) 1996 Frank Neumann
>   *
> @@ -31,7 +34,7 @@
>   *
>   */
>  
> -#define CLGEN_VERSION "1.9.9.1"
> +#define CIRRUSFB_VERSION "2.0-pre2"
>  
>  #include <linux/config.h>
>  #include <linux/module.h>
> @@ -63,15 +66,8 @@
>  #define isPReP 0
>  #endif
>  
> -#include <video/fbcon.h>
> -#include <video/fbcon-mfb.h>
> -#include <video/fbcon-cfb8.h>
> -#include <video/fbcon-cfb16.h>
> -#include <video/fbcon-cfb24.h>
> -#include <video/fbcon-cfb32.h>
> -
> -#include "clgenfb.h"
> -#include "vga.h"
> +#include "video/vga.h"
> +#include "video/cirrus.h"

should be <> not "", no?


>  /*****************************************************************
> @@ -81,20 +77,20 @@
>   */
>  
>  /* enable debug output? */
> -/* #define CLGEN_DEBUG 1 */
> +/* #define CIRRUSFB_DEBUG 1 */
>  
>  /* disable runtime assertions? */
> -/* #define CLGEN_NDEBUG */
> +/* #define CIRRUSFB_NDEBUG */
>  
>  /* debug output */
> -#ifdef CLGEN_DEBUG
> +#ifdef CIRRUSFB_DEBUG
>  #define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
>  #else
>  #define DPRINTK(fmt, args...)
>  #endif
>  
>  /* debugging assertions */
> -#ifndef CLGEN_NDEBUG
> +#ifndef CIRRUSFB_NDEBUG

IMO it would be nice to split up your patch into one that does all the 
cosmetic renames, and one that does the "real stuff".  Makes it far 
easier to review.

Re: [PATCH] cirrusfb: it lives!

[Andrew Morton]

David Eger <eger@havoc.gtf.org> wrote:
>
> This patch brings the cirrusfb driver up to date with 2.6.

Olé

> cirrusfb
> has suffered bit rot like you wouldn't believe (last updated... 2.3.x era?).

My more venerable testbox has Cirrus hardware.  The problems have been noted ;)

> The driver will now compile again, and you can change to a high resolution 
> text mode with stty.  Known defects: doesn't play nice with X, nor fbset.
> Nonetheless, please apply to -mm and forward to mainline.

Shall do, thanks.

Re: [PATCH] cirrusfb: it lives!

[David Eger]

On Tue, Jun 22, 2004 at 04:36:53PM -0400, Jeff Garzik wrote:
> David Eger wrote:
> >Dear Andrew,
> >
> >This patch brings the cirrusfb driver up to date with 2.6.  cirrusfb
> >has suffered bit rot like you wouldn't believe (last updated... 2.3.x 
> >era?).
...

> >-
> >-#include "clgenfb.h"
> >-#include "vga.h"
> >+#include "video/vga.h"
> >+#include "video/cirrus.h"
> 
> should be <> not "", no?

My bad.  I keep thinking <> is for those system headers -- obviously not
the little project I'm working on ;-)

> > /* debugging assertions */
> >-#ifndef CLGEN_NDEBUG
> >+#ifndef CIRRUSFB_NDEBUG
> 
> IMO it would be nice to split up your patch into one that does all the 
> cosmetic renames, and one that does the "real stuff".  Makes it far 
> easier to review.

Porting the driver to 2.6 did involve a lot of search-and-replace.
Sorry that it makes checking the diff so hard. I'll try to explain what's
changed and why,  but splitting the patch into cosmetic+functional isn't
really something I want to do...  Read on for details.

C = Cosmetic change
L = Logical change
A = API change
W = register Writing change

(1-CA) fb_info and cirrusfb_info:
   - mostly cosmetic, but a lot less confusing, and no more nasty casting.

  It used to be stylish to embed struct fb_info_gen (now struct fb_info)
  as the first member of struct clgenfb_info (now struct cirrusfb_info), 
  and then you'd cast to the deisred struct.
  Now we pass the size of our data structure to framebuffer_alloc(), 
  and we make fb_info and cirrusfb_info reference each other with 
  pointers (as in radeonfb).

  In the old code, there two declarations were common:

  clgenfb_info *fb_info;
  clgenfb_info *info;

  since there's also a 'struct fb_info' now, I found this really confusing,
  and unified usage as:

  cirrusfb_info *cinfo;
  fb_info *info;

  This accounts for a lot of the search and replace cosmetic upgrade.

(2-A) All of the FB knowledge of FB internals is gone in 2.6

(3-LW) revised maxclock numbers (cirrusfb_board_info_rec.maxclock[5])

	In my quest to get fbset to work, I borrowed some maxclock data
	from the X.Org tree for various chipsets.  It didn't really seem 
	to help.  oh well.

(3-LA) upgraded PCI registration

	Instead of doing PCI walking from the driver, we hand off a
	pci_device_id table to the PCI subsystem and just get called when
	it finds a relevant board.

(4-L) striking lots of __init and __initdata specifiers

	I was running into some things not working when I moved the call to
	init_vgachip() from the driver registration to set_par().  I thought
	perhaps this was due to some things being marked __init accidentally
	so I axed said annotations.  Turns out it was something else.  See 5.

(5-LA) delayed chip initialization, nasty double-set_par() pseudo-bug
  
  Tony says that the fb drivers shouldn't do any chipset initialization
  until they get a set_par() call.  This way, fb modules can be safely
  unloaded if no one gets around to using them, and the vga_con -> fbcon
  hand off is smoother, as fbcon can still grab the back-scroll data from 
  vga_con...

  In any case, moving the calls to init_vgachip() and fbgen_do_set_var()
  from driver initialization to set_par() revealed that the cirrus 
  register-writing function needs to be called twice for a mode change to
  work.  I don't understand why.

(6-LA) split clgen_decode_var() into the bits that check the var and the
  bits that actually generate register information (par/regs) to write

  Adding modedb hooks here might actually fix fbset, i think...

(7-LW) No longer write the palette in init_vgachip() nor in set_par().
  Someone else (fbcon?) seems to be making its own calls to setcolreg()
  for us.

(8-LW) setcolreg() -- while removing all of the console cruft, I had
  to try to reconstitute the palette code. I think I got this right, 
  but I could be off -- the penguin boots in the correct colors at least ;-)

(9-L) pan_display() - we don't do wrap, silly. that's only on the amiga.

(10-L) pan+BLT - to make pan play nicely with copyarea()/fillrect() 
   I had to add a couple of calls to cirrusfb_WaitBLT() to make sure
	the engine is idle.

(11-LW) cirrusfb_blank() - I upgraded the switch here to use the new
  VESA_* blanking mode constants.  I think I translated the right
  logic for the right blanking levels.


-dte