2004-06-22 20:47:05

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] cirrusfb: it lives!

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 <[email protected]>
>
> 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 <[email protected]>
> *
> * 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.



2004-06-23 03:19:25

by David Eger

[permalink] [raw]
Subject: Re: [PATCH] cirrusfb: it lives!

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