Re: [PATCH] cirrusfb: it lives!

On Tue, Jun 22, 2004 at 04:36:53PM -0400, Jeff Garzik wrote:

David Eger wrote:

quoted

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?).

...

quoted

-
-#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 ;-)

quoted

/* 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