lkml.org 
[lkml]   [2005]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Linux-fbdev-devel] [PATCH 1/1 2.6.13-rc4] framebuffer: new driver for cyberblade/i1 core
Knut Petersen wrote:
> Currently tridenfb claims to support the cyberblade/i1 graphics core.
> This is of very limited truth. There is a great number of bugs, even
> vesafb is faster and provides more working modes and much better
> quality of the video signal.
>
> Tridentfb seems to be unmaintained,and documentation for most of the
> supported chips is not available. Fixing cyberblade/i1 support inside
> of tridentfb was not an option, it would have caused numerous
> if(CYBERBLADEi1) else ... cases and would have rendered the code
> to be almost unmaintainable.
>
> This code does support the graphics core of a single north bridge
> and has been tested and developed on a system equipped with that
> chip. It cannot break anything but the broken cyberblade/i1 support
> of tridentfb, and even if that would be the case, there is still
> vesafb as a working alternative. On the other hand it provides
> significant improvements. Because of this I believe that there is
> no reason to keep it out of 2.6.13 just because it is presented a
> bit late in the development cycle.
>
>
> Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
>
> diff -urN linux-2.6.13-rc4/Documentation/fb/cyblafb.txt
> linux-2.6.13-rc4-tfix/Documentation/fb/cyblafb.txt
> --- linux-2.6.13-rc4/Documentation/fb/cyblafb.txt 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.13-rc4-tfix/Documentation/fb/cyblafb.txt 2005-07-31
> 09:38:44.000000000 +0200
> @@ -0,0 +1,354 @@
> +CyBlaFB is a framebuffer driver for the Cyberblade/i1 graphics core
> integrated
> +into the VIA Apollo PLE133 (aka vt8601) south bridge. It is developed and
> +tested using a VIA EPIA 5000 board.

Nice docs :-)

<snip>

> +
> config FB_TRIDENT
> tristate "Trident support"
> depends on FB && PCI
> @@ -1193,8 +1219,12 @@
> but also on some motherboards. For more information, read
> <file:Documentation/fb/tridentfb.txt>
>
> + Attention: Cyberblade/i1 support has been removed, choose the
> + cyblafb driver instead.
> +

Is it really necessary to remove it from tridentfb so soon? Maybe you
can add a warning first, then remove it after a few development cycles.

<snip>

> +
> +#include <linux/config.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <asm/types.h>
> +#include <video/cyblafb.h>
> +
> +#define VERSION "0.50"
> +
> +struct cyblafb_par {
> + void __iomem * io_virt; //iospace virtual memory address
> +};
> +
> +static struct cyblafb_par default_par;
> +static struct fb_info fb_info;

Do this in the beginning of cybla_pci_probe()

struct fb_info *info;

info = framebuffer_alloc(sizeof(struct cyblafb_par), dev->dev);
if (info)
exit;

Then do a corresponding framebuffer_dealloc() on exit/unload.

This way, you need not statically allocate memory for the par
and info, which increases the kernel image size unnecessarily.

And so you don't lose your info structure, do this:

pci_set_drvdata(dev, info) at the end of cybla_pci_probe()

To get your info back (ie. in cybla_pci_remove()), do this:

struct fb_info *info = pci_get_drvdata(dev);

> +static struct fb_var_screeninfo default_var;

You also don't need to allocate this. default_var is used only
during init. Why not use info->var? Or if you can't, mark it
__devinitdata.

> +static struct fb_fix_screeninfo cyblafb_fix = {
> + .id = "CyBla",
> + .type = FB_TYPE_PACKED_PIXELS,
> + .ypanstep = 1,
> + .visual = FB_VISUAL_PSEUDOCOLOR,
> + .accel = FB_ACCEL_NONE,
> +};

Same with the above. You can mark it __devinitdata and
don't reference it again after saving it in info->fix.

> +
> +static u32 pseudo_pal[16];
> +
> +static int displaytype;
> +
> +static char *mode = NULL;
> +static int bpp = 8;
> +static int ref = 75;
> +static int fp;
> +static int crt;
> +static int nativex;
> +static int center;
> +static int stretch;
> +static int pciwb = 1;
> +static int pcirb = 1;
> +static int pciwr = 1;
> +static int pcirr = 1;
> +static int memsize;
> +static int verbosity;
> +static int vesafb;

Any of the above that can be marked __devinitdata? For those
that you need after initialization, place them in info->par.

> +
> +module_param(mode, charp, 0);
> +module_param(bpp, int, 0);
> +module_param(ref, int, 0);
> +module_param(fp, int, 0);
> +module_param(crt, int, 0);
> +module_param(nativex, int, 0);
> +module_param(center, int, 0);
> +module_param(stretch, int, 0);
> +module_param(pciwb, int, 0);
> +module_param(pcirb, int, 0);
> +module_param(pciwr, int, 0);
> +module_param(pcirr, int, 0);
> +module_param(memsize, int, 0);
> +module_param(verbosity, int, 0);
> +module_param(vesafb, int, 0);
> +
> +
> +//=========================================
> +//
> +// Port access macros for memory mapped io
> +//
> +//=========================================
> +
> +#define out8(r,v) writeb(v,((struct cyblafb_par
> *)fb_info.par)->io_virt+r)
> +#define out32(r,v) writel(v,((struct cyblafb_par
> *)fb_info.par)->io_virt+r)
> +#define in8(r) readb(((struct cyblafb_par
> *)fb_info.par)->io_virt+r)
> +#define in32(r) readl(((struct cyblafb_par *)fb_info.par)->io_virt+r)

If you use framebuffer_alloc(), you can change the above macros to
(struct cyblafb_par *)info->par->io_virt+r, for example.

<snip>

> +//=====================================================================
> +//
> +// Although this is a .fb_sync function that could be enabled in
> +// cyblafb_ops, we do not include it there. We sync immediately before
> +// new GE operations to improve performance.

I wonder. I thought that avoiding to sync as much as possible improves
performance...

<snip>

> +static int cyblafb_check_var(struct fb_var_screeninfo *var,
> + struct fb_info *info)
> +{
> + int bpp = var->bits_per_pixel;
> + int s,t,maxvyres;
> +
> + //
> + // we try to support 8, 16, 24 and 32 bpp modes,
> + // default to 8
> + //
> + // there is a 24 bpp mode, but for now we change requests to 32 bpp
> + // (This is what tridentfb does ... will be changed in the future)
> + //
> + //
> + if ( bpp % 8 != 0 || bpp < 8 || bpp >32)
> + bpp = 8;
> + if (bpp == 24 )
> + bpp = var->bits_per_pixel = 32;
> +
> +
> + //
> + // interlaced modes are broken, fail if one is requested
> + //
> + if (var->vmode & FB_VMODE_INTERLACED)
> + return -EINVAL;

You are too strict :-) You can always clear FB_VMODE_INTERLACED and
return success. This is acceptable behavior.

> +
> + //
> + // fail if requested resolution is higher than physical
> + // flatpanel resolution
> + //
> + if (flatpanel && nativex && var->xres > nativex)
> + return -EINVAL;
> +

This is acceptable behavior, but you can always return
with var->xres = nativex.

> + //
> + // xres != xres_virtual is broken, fail if such an
> + // unusual mode is requested
> + //
> + if (var->xres != var->xres_virtual)
> + return -EINVAL;

Same here, var->xres = var->xres_virtual.

> +
> + //
> + // we do not allow vclk to exceed 230 MHz
> + //
> + if ((bpp==32 ? 200000000 : 100000000) / var->pixclock > 23000)
> + return -EINVAL;
> +
> + //
> + // calc max yres_virtual that would fit in memory
> + // and max yres_virtual that could be used for scrolling
> + // and use minimum of the results as maxvyres
> + //
> + // adjust vyres_virtual to maxvyres if necessary
> + // fail if requested yres is bigger than maxvyres
> + //
> + s = (0x1fffff / (var->xres * bpp/8)) + var->yres;
> + t = info->fix.smem_len / (var->xres * bpp/8);
> + maxvyres = t < s ? t : s;
> + if (maxvyres < var->yres_virtual)
> + var->yres_virtual=maxvyres;
> + if (maxvyres < var->yres)
> + return -EINVAL;
> +
> + switch (bpp) {
> + case 8:
> + var->red.offset = 0;
> + var->green.offset = 0;
> + var->blue.offset = 0;
> + var->red.length = 6;
> + var->green.length = 6;
> + var->blue.length = 6;
> + break;
> + case 16:
> + var->red.offset = 11;
> + var->green.offset = 5;
> + var->blue.offset = 0;
> + var->red.length = 5;
> + var->green.length = 6;
> + var->blue.length = 5;
> + break;
> + case 32:
> + var->red.offset = 16;
> + var->green.offset = 8;
> + var->blue.offset = 0;
> + var->red.length = 8;
> + var->green.length = 8;
> + var->blue.length = 8;
> + break;
> + default:
> + return -EINVAL;

Again, you can always round off bits_per_pixel to
ones that the driver can support.

<snip>

> +//==========================================================================
>
> +//
> +// getstartupmode() decides about the inital video mode
> +//
> +// There is no reason to use modedb, a lot of video modes there would
> +// need altered timings to display correctly. So I decided that it is much
> +// better to provide a limited optimized set of modes plus the option of
> +// using the mode in effect at startup time (might be selected using the
> +// vga=??? paramter). After that the user might use fbset to select any
> +// mode he likes, check_var will not try to alter geometry parameters as
> +// it would be necessary otherwise.
> +//
> +//==========================================================================

You can get the correct set of video modes from the EDID. If you can't add DDC/I2C
support, I will submit a patch that will grab the EDID from the BIOS. Should
be usable by all x86 drivers. This way you don't need to massage the timings.

<snip>

> +static int __devinit cybla_pci_probe(struct pci_dev * dev,
> + const struct pci_device_id * id)
> +{
> +
> + fb_info.par = &default_par;

As mentioned in the beginning, you don't need to do the above.

> +
> + if (pci_enable_device(dev)) {
> + output("could not enable device!\n");
> + goto errout_enable;
> + }
> +
> + // might already be requested by vga console or vesafb,
> + // so we do care about success
> + request_region(0x3c0, 32, "cyblafb");
> +
> + //
> + // Graphics Engine Registers
> + //
> + request_region(GEBase, 0x100, "cyblafb");
> +
> + regdump();
> +
> + enable_mmio();
> +
> + /* setup MMIO region */
> + cyblafb_fix.mmio_start = pci_resource_start(dev,1);
> + cyblafb_fix.mmio_len = 0x20000;
> +
> + if (!request_mem_region(cyblafb_fix.mmio_start,
> + cyblafb_fix.mmio_len, "cyblafb")) {
> + output("request_mem_region failed for mmio region!\n");
> + goto errout_mmio_reqmem;
> + }
> +
> + default_par.io_virt = ioremap_nocache(cyblafb_fix.mmio_start,
> + cyblafb_fix.mmio_len);
> +
> + if (!default_par.io_virt) {
> + output("ioremap failed for mmio region\n");
> + goto errout_mmio_remap;
> + }
> +
> + // setup framebuffer memory ... might already be requested
> + // by vesafb. Not to fail in case of an unsuccessful request
> + // is useful for the development cycle
> + cyblafb_fix.smem_start = pci_resource_start(dev,0);
> + cyblafb_fix.smem_len = get_memsize();
> +
> + if (!request_mem_region(cyblafb_fix.smem_start,
> + cyblafb_fix.smem_len, "cyblafb")) {
> + output("request_mem_region failed for smem region!\n");
> + if (!vesafb)

Instead of testing for the vesafb boot option, you can test for
screen_info.orig_video_isVGA == VIDEO_TYPE_VLFB. This is set when
screen is in VESA VGA graphics mode. See include/linux/tty.h.

> + goto errout_smem_req;
> + }
> +
> + fb_info.screen_base = ioremap_nocache(cyblafb_fix.smem_start,
> + cyblafb_fix.smem_len);
> +
> + if (!fb_info.screen_base) {
> + output("ioremap failed for smem region\n");
> + goto errout_smem_remap;
> + }
> +
> + displaytype = get_displaytype();
> +
> + if(flatpanel)
> + nativex = get_nativex();
> +
> + fb_info.fix = cyblafb_fix;
> + fb_info.fbops = &cyblafb_ops;
> +
> + //
> + // FBINFO_HWACCEL_YWRAP .... does not work (could be made to work)
> + // FBINFO_PARTIAL_PAN_OK .... is not ok
> + // FBINFO_READS_FAST .... is necessary for optimal scrolling
> + //
> + fb_info.flags = FBINFO_DEFAULT | FBINFO_HWACCEL_YPAN
> + | FBINFO_HWACCEL_COPYAREA | FBINFO_HWACCEL_FILLRECT
> + | FBINFO_HWACCEL_IMAGEBLIT | FBINFO_READS_FAST;
> +
> + fb_info.pseudo_palette = pseudo_pal;
> +
> + if(getstartupmode())
> + goto errout_findmode;
> +
> + fb_alloc_cmap(&fb_info.cmap,256,0);
> +
> + fb_info.var = default_var;
> + fb_info.device = &dev->dev;
> +
> + if (register_framebuffer(&fb_info)) {
> + output("Could not register CyBla framebuffer\n");
> + goto errout_register;
> + }
> +
> + return 0;
> +
> + //
> + // error paths
> + //
> +
> + errout_register:
> + errout_findmode:
> + iounmap(fb_info.screen_base);
> + errout_smem_remap:
> + release_mem_region(cyblafb_fix.smem_start,
> + cyblafb_fix.smem_len);
> + errout_smem_req:
> + iounmap(default_par.io_virt);
> + errout_mmio_remap:
> + release_mem_region(cyblafb_fix.mmio_start,
> + cyblafb_fix.mmio_len);
> + errout_mmio_reqmem:
> +// release_region(0x3c0, 32);
> + errout_enable:
> +

Don't forget fb_dealloc_cmap() in the exit path, especially if
you like to load/unload the driver. You can insert
framebuffer_dealloc() here also if you use framebuffer_alloc().

> + output("CyblaFB version %s aborting init.\n", VERSION);
> + return -ENODEV;
> +}
> +
> +static void __devexit cybla_pci_remove(struct pci_dev * dev)
> +{
> + struct cyblafb_par *par = (struct cyblafb_par*)fb_info.par;

or
struct fb_info *info = pci_get_drvdata(dev);
struct cyblafb_par *par = info->par;


And to please Linus, try to compile the driver with sparse checking
on.

Tony

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-07-31 13:58    [W:0.042 / U:1.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site