Re: [PATCH] cirrusfb: it lives!

[David Eger <eger@havoc.gtf.org>]

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


-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 - 
digital self defense, top technical experts, no vendor pitches, 
unmatched networking opportunities. Visit www.blackhat.com