PATCH Proposal make FB_ACCEL_ private to the driver

PATCH Proposal make FB_ACCEL_ private to the driver

[Henk Vergonet]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

Hi gents,

This post was originally posted to the kernel-janitors list. After which
I learned there was an active development-list for linux-framebuffer stuff. 


-------------------------

I want to propose to localize the maintenance and registration of the
FB_ACCEL_ id's and remove it from the public fb.h include file.

Motivation:
- This removes an unneeded dependency to fb.h if an additional cards/chips are
  introduced. (These id's are only interesting for the driver anyhow. 
  Only fbset will print out the id, but I'll doubt this is used by anyone.)
- It relieves maintenance of the common frambuffer code, because we don't have
  to keep fb.h in sync.
- It also relieves the maintenance for the  driver developer
  because he can add new cards without having to request modifications
  to public framebuffer code.
  => cleaner fb.h

Could we make this a general guideline?


I have created a patch for the savage driver and fb.h to give an idea
how this would look like. It has no impact on the other fb drivers.

Best regards,

Henk Vergonet


[-- Attachment #2: patch-2.6.11-rc4-fb_h --]
[-- Type: text/plain, Size: 2943 bytes --]

diff -urpN linux-2.6.11-rc4-vanilla/include/linux/fb.h linux-2.6.11-rc4/include/linux/fb.h
--- linux-2.6.11-rc4-vanilla/include/linux/fb.h	2005-02-23 10:09:26.000000000 +0100
+++ linux-2.6.11-rc4/include/linux/fb.h	2005-02-24 11:49:55.000000000 +0100
@@ -60,6 +60,14 @@
 #define FB_VISUAL_DIRECTCOLOR		4	/* Direct color */
 #define FB_VISUAL_STATIC_PSEUDOCOLOR	5	/* Pseudo color readonly */
 
+/*
+   The maintenance and registration of a global FB_ACCEL_ accelerator table is
+   deprecated. The driver specific chip identifier should be local to the driver
+   and not be exported via public include files.
+   For userspace applications the use of the sys interface is recommended.
+*/
+#define FB_ACCEL_DUMMY		0xffff	/* Placeholder value until all global */
+					/* administration is removed.	*/
 #define FB_ACCEL_NONE		0	/* no hardware accelerator	*/
 #define FB_ACCEL_ATARIBLITT	1	/* Atari Blitter		*/
 #define FB_ACCEL_AMIGABLITT	2	/* Amiga Blitter                */
@@ -114,22 +122,6 @@
 #define FB_ACCEL_NEOMAGIC_NM2360 97	/* NeoMagic NM2360              */
 #define FB_ACCEL_NEOMAGIC_NM2380 98	/* NeoMagic NM2380              */
 
-#define FB_ACCEL_SAVAGE4        0x80	/* S3 Savage4                   */
-#define FB_ACCEL_SAVAGE3D       0x81	/* S3 Savage3D                  */
-#define FB_ACCEL_SAVAGE3D_MV    0x82	/* S3 Savage3D-MV               */
-#define FB_ACCEL_SAVAGE2000     0x83	/* S3 Savage2000                */
-#define FB_ACCEL_SAVAGE_MX_MV   0x84	/* S3 Savage/MX-MV              */
-#define FB_ACCEL_SAVAGE_MX      0x85	/* S3 Savage/MX                 */
-#define FB_ACCEL_SAVAGE_IX_MV   0x86	/* S3 Savage/IX-MV              */
-#define FB_ACCEL_SAVAGE_IX      0x87	/* S3 Savage/IX                 */
-#define FB_ACCEL_PROSAVAGE_PM   0x88	/* S3 ProSavage PM133           */
-#define FB_ACCEL_PROSAVAGE_KM   0x89	/* S3 ProSavage KM133           */
-#define FB_ACCEL_S3TWISTER_P    0x8a	/* S3 Twister                   */
-#define FB_ACCEL_S3TWISTER_K    0x8b	/* S3 TwisterK                  */
-#define FB_ACCEL_SUPERSAVAGE    0x8c    /* S3 Supersavage               */
-#define FB_ACCEL_PROSAVAGE_DDR  0x8d	/* S3 ProSavage DDR             */
-#define FB_ACCEL_PROSAVAGE_DDRK 0x8e	/* S3 ProSavage DDR-K           */
-
 struct fb_fix_screeninfo {
 	char id[16];			/* identification string eg "TT Builtin" */
 	unsigned long smem_start;	/* Start of frame buffer mem */
@@ -145,8 +137,10 @@ struct fb_fix_screeninfo {
 	unsigned long mmio_start;	/* Start of Memory Mapped I/O   */
 					/* (physical address) */
 	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
-	__u32 accel;			/* Indicate to driver which	*/
-					/*  specific chip/card we have	*/
+	__u32 accel;			/* OBSOLETE please use FB_ACCEL_DUMMY */
+					/* as a placeholder see explanation   */
+					/* above. (Used to indicate to driver */
+					/* which specific chip/card we had.)  */
 	__u16 reserved[3];		/* Reserved for future compatibility */
 };
 

[-- Attachment #3: patch-2.6.11-rc4-savage-accel --]
[-- Type: text/plain, Size: 2792 bytes --]

diff -urpN --exclude='*.o' linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb-i2c.c linux-2.6.11-rc4/drivers/video/savage/savagefb-i2c.c
--- linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb-i2c.c	2005-02-23 10:08:55.000000000 +0100
+++ linux-2.6.11-rc4/drivers/video/savage/savagefb-i2c.c	2005-02-23 17:51:17.000000000 +0100
@@ -183,7 +183,7 @@ void savagefb_create_i2c_busses(struct f
 	struct savagefb_par *par = (struct savagefb_par *)info->par;
 	par->chan.par	= par;
 
-	switch(info->fix.accel) {
+	switch(par->accel) {
 	case FB_ACCEL_PROSAVAGE_DDRK:
 	case FB_ACCEL_PROSAVAGE_PM:
 		par->chan.reg         = CR_SERIAL2;
diff -urpN --exclude='*.o' linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb.c linux-2.6.11-rc4/drivers/video/savage/savagefb.c
--- linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb.c	2004-12-24 22:34:45.000000000 +0100
+++ linux-2.6.11-rc4/drivers/video/savage/savagefb.c	2005-02-23 17:51:42.000000000 +0100
@@ -1793,9 +1793,10 @@ static int __devinit savage_init_fb_info
 	info->fix.xpanstep	   = 2;
 	info->fix.ypanstep	   = 1;
 	info->fix.ywrapstep   = 0;
-	info->fix.accel       = id->driver_data;
+	info->fix.accel       = FB_ACCEL_DUMMY;
 
-	switch (info->fix.accel) {
+	par->accel = id->driver_data;
+	switch (par->accel) {
 	case FB_ACCEL_SUPERSAVAGE:
 		par->chip = S3_SUPERSAVAGE;
 		snprintf (info->fix.id, 16, "SuperSavage");
diff -urpN --exclude='*.o' linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb.h linux-2.6.11-rc4/drivers/video/savage/savagefb.h
--- linux-2.6.11-rc4-vanilla/drivers/video/savage/savagefb.h	2004-12-24 22:35:50.000000000 +0100
+++ linux-2.6.11-rc4/drivers/video/savage/savagefb.h	2005-02-23 17:49:32.000000000 +0100
@@ -76,6 +76,24 @@ typedef enum {
   S3_LAST
 } savage_chipset;
 
+typedef enum {
+	FB_ACCEL_SAVAGE4 = 0x80,	/* S3 Savage4		*/
+	FB_ACCEL_SAVAGE3D,		/* S3 Savage3D		*/
+	FB_ACCEL_SAVAGE3D_MV,		/* S3 Savage3D-MV	*/
+	FB_ACCEL_SAVAGE2000,		/* S3 Savage2000	*/
+	FB_ACCEL_SAVAGE_MX_MV,		/* S3 Savage/MX-MV	*/
+	FB_ACCEL_SAVAGE_MX,		/* S3 Savage/MX		*/
+	FB_ACCEL_SAVAGE_IX_MV,		/* S3 Savage/IX-MV	*/
+	FB_ACCEL_SAVAGE_IX,		/* S3 Savage/IX		*/
+	FB_ACCEL_PROSAVAGE_PM,		/* S3 ProSavage PM133	*/
+	FB_ACCEL_PROSAVAGE_KM,		/* S3 ProSavage KM133	*/
+	FB_ACCEL_S3TWISTER_P,		/* S3 Twister		*/
+	FB_ACCEL_S3TWISTER_K,		/* S3 TwisterK		*/
+	FB_ACCEL_SUPERSAVAGE,		/* S3 Supersavage	*/
+	FB_ACCEL_PROSAVAGE_DDR,		/* S3 ProSavage DDR	*/
+	FB_ACCEL_PROSAVAGE_DDRK		/* S3 ProSavage DDR-K	*/
+} savage_accel;
+
 #define BIOS_BSIZE		     1024
 #define BIOS_BASE		     0xc0000
 
@@ -163,6 +181,7 @@ struct savagefb_i2c_chan {
 struct savagefb_par {
 	struct pci_dev *pcidev;
 	savage_chipset  chip;
+	savage_accel	accel;
 	struct savagefb_i2c_chan chan;
 	unsigned char   *edid;
 	u32 pseudo_palette[16];

Re: PATCH Proposal make FB_ACCEL_ private to the driver

[Geert Uytterhoeven]

On Sun, 27 Feb 2005, Henk Vergonet wrote:
> This post was originally posted to the kernel-janitors list. After which
> I learned there was an active development-list for linux-framebuffer stuff. 

I'm afraid you will never see this reply, as rememberme@systol.god.lan won't
resolve....

> I want to propose to localize the maintenance and registration of the
> FB_ACCEL_ id's and remove it from the public fb.h include file.
> 
> Motivation:
> - This removes an unneeded dependency to fb.h if an additional cards/chips are
>   introduced. (These id's are only interesting for the driver anyhow. 
>   Only fbset will print out the id, but I'll doubt this is used by anyone.)

Yes, it's used by userspace apps that need to know the graphics card type (e.g.
DirectFB and the good old XF68_FBDev).

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: PATCH Proposal make FB_ACCEL_ private to the driver

[Henk Vergonet]

On Sun, Feb 27, 2005 at 07:28:47PM +0100, Geert Uytterhoeven wrote:
> On Sun, 27 Feb 2005, Henk Vergonet wrote:
> I'm afraid you will never see this reply, as rememberme@systol.god.lan won't
> resolve....

It is intentional, but if it's a problem I can fix it.

> 
> > I want to propose to localize the maintenance and registration of the
> > FB_ACCEL_ id's and remove it from the public fb.h include file.
> > 
> > Motivation:
> > - This removes an unneeded dependency to fb.h if an additional cards/chips are
> >   introduced. (These id's are only interesting for the driver anyhow. 
> >   Only fbset will print out the id, but I'll doubt this is used by anyone.)
> 
> Yes, it's used by userspace apps that need to know the graphics card type (e.g.
> DirectFB and the good old XF68_FBDev).

My point is not that there are no userspace utils that use it. But that
we should lose this identifier in the public interface eventually because it introduces unneeded dependencies as stated in my earlier post.

If it's used by userspace in any other way than for pure informational purposes:
For example to make certain assumptions on hardware capabilities, locations of specific hardware registers ...) then that's ok but should be considered as a make-shift solution until we have found a better solution drivers/API.

I know this may be only a small issue to most, I am sure there are more pressing issues, but small cleanups in the fb interface will eventually pay of in the long run.

What do we think developers... 

Do we think it's a good idea to remove the FB_ACCEL register in the fb.h in the future, or is it a no-go?
If nobody thinks it's a good idea I'll shut up, (for now ;)

Best regards,

Henk


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: PATCH Proposal make FB_ACCEL_ private to the driver

[Henk Vergonet]

On Sun, Feb 27, 2005 at 07:28:47PM +0100, Geert Uytterhoeven wrote:
> On Sun, 27 Feb 2005, Henk Vergonet wrote:
> I'm afraid you will never see this reply, as rememberme@systol.god.lan won't
> resolve....

It is intentional, but if it's a problem I can fix it.

> 
> > I want to propose to localize the maintenance and registration of the
> > FB_ACCEL_ id's and remove it from the public fb.h include file.
> > 
> > Motivation:
> > - This removes an unneeded dependency to fb.h if an additional cards/chips are
> >   introduced. (These id's are only interesting for the driver anyhow. 
> >   Only fbset will print out the id, but I'll doubt this is used by anyone.)
> 
> Yes, it's used by userspace apps that need to know the graphics card type (e.g.
> DirectFB and the good old XF68_FBDev).

My point is not that there are no userspace utils that use it. But that
we should lose this identifier in the public interface eventually because it introduces unneeded dependencies as stated in my earlier post.

If it's used by userspace in any other way than for pure informational purposes:
For example to make certain assumptions on hardware capabilities, locations of specific hardware registers ...) then that's ok but should be considered as a make-shift solution until we have found a better solution drivers/API.

I know this may be only a small issue to most, I am sure there are more pressing issues, but small cleanups in the fb interface will eventually pay of in the long run.

What do we think developers... 

Do we think it's a good idea to remove the FB_ACCEL register in the fb.h in the future, or is it a no-go?
If nobody thinks it's a good idea I'll shut up, (for now ;)

Best regards,

Henk


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: PATCH Proposal make FB_ACCEL_ private to the driver

[Ville Syrjälä]

On Sun, Feb 27, 2005 at 11:17:15PM +0100, Henk Vergonet wrote:
> On Sun, Feb 27, 2005 at 07:28:47PM +0100, Geert Uytterhoeven wrote:
> > On Sun, 27 Feb 2005, Henk Vergonet wrote:
> > I'm afraid you will never see this reply, as rememberme@systol.god.lan won't
> > resolve....
> 
> It is intentional, but if it's a problem I can fix it.
> 
> > 
> > > I want to propose to localize the maintenance and registration of the
> > > FB_ACCEL_ id's and remove it from the public fb.h include file.
> > > 
> > > Motivation:
> > > - This removes an unneeded dependency to fb.h if an additional cards/chips are
> > >   introduced. (These id's are only interesting for the driver anyhow. 
> > >   Only fbset will print out the id, but I'll doubt this is used by anyone.)
> > 
> > Yes, it's used by userspace apps that need to know the graphics card type (e.g.
> > DirectFB and the good old XF68_FBDev).
> 
> My point is not that there are no userspace utils that use it. But that
> we should lose this identifier in the public interface eventually because it introduces unneeded dependencies as stated in my earlier post.
> 
> If it's used by userspace in any other way than for pure informational purposes:
> For example to make certain assumptions on hardware capabilities, locations of specific hardware registers ...) then that's ok but should be considered as a make-shift solution until we have found a better solution drivers/API.

And that is exactly how DirectFB uses it. Case closed :)

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_ide95&alloc_id\x14396&op=click