Re: [PATCH 1/1] New Framebuffer for Intel based Macs

From: Antonino A. Daplas
Date: Tue Jun 20 2006 - 11:13:38 EST

Edgar Hucek wrote:
> This patch add a new framebuffer driver for the Intel Based macs.
> This framebuffer is needed when booting from EFI to get something
> out the box ;)
>

A few comments below, but I don't think they are serious enough to
prevent this driver from inclusion.

> Signed-off-by: Edgar Hucek <hostmaster@xxxxxxxxxx>
>
>
> ------------------------------------------------------------------------
>
> diff -uNr linux-2.6.17.orig/drivers/video/imacfb.c linux-2.6.17/drivers/video/imacfb.c
> --- linux-2.6.17.orig/drivers/video/imacfb.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.17/drivers/video/imacfb.c 2006-06-18 11:46:13.000000000 +0200
> @@ -0,0 +1,436 @@
> +/*
> + * framebuffer driver for Intel Based Mac's
> + *
> + * (c) 2006 Edgar Hucek <gimli@xxxxxxxxxxxxxx>
> + * Original vesa driver written by Gerd Knorr <kraxel@xxxxxxxxxxxxxxxxxxxxx>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/tty.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +
> +#include <video/vga.h>
> +#include <asm/io.h>
> +#include <asm/mtrr.h>
> +
> +#define dac_reg (0x3c8)
> +#define dac_val (0x3c9)
> +
> +typedef enum _MAC_TAPE {
> + M_I17,
> + M_I20,
> + M_MINI,
> + M_MACBOOK,
> + M_NEW
> +} MAC_TAPE;
> +
> +/* --------------------------------------------------------------------- */
> +
> +static struct fb_var_screeninfo imacfb_defined __initdata = {
> + .activate = FB_ACTIVATE_NOW,
> + .height = -1,
> + .width = -1,
> + .right_margin = 32,
> + .upper_margin = 16,
> + .lower_margin = 4,
> + .vsync_len = 4,
> + .vmode = FB_VMODE_NONINTERLACED,
> +};
> +
> +static struct fb_fix_screeninfo imacfb_fix __initdata = {
> + .id = "IMAC VGA",
> + .type = FB_TYPE_PACKED_PIXELS,
> + .accel = FB_ACCEL_NONE,
> +};
> +
> +static int inverse = 0;
> +static int mtrr = 0; /* disable mtrr */
> +static int vram_remap __initdata = 0; /* Set amount of memory to be used */
> +static int vram_total __initdata = 0; /* Set total amount of memory */
> +static int depth;
> +static int model = M_NEW;
> +static int manual_height = 0;
> +static int manual_width = 0;
> +
> +/* --------------------------------------------------------------------- */
> +
> +static int imacfb_setcolreg(unsigned regno, unsigned red, unsigned green,
> + unsigned blue, unsigned transp,
> + struct fb_info *info)
> +{
> + /*
> + * Set a single color register. The values supplied are
> + * already rounded down to the hardware's capabilities
> + * (according to the entries in the `var' structure). Return
> + * != 0 for invalid regno.
> + */
> +
> + if (regno >= info->cmap.len)
> + return 1;
> +
> + if (regno < 16 && info->var.bits_per_pixel != 8) {
> + switch (info->var.bits_per_pixel) {
> + case 16:
> + if (info->var.red.offset == 10) {
> + /* 1:5:5:5 */
> + ((u32*) (info->pseudo_palette))[regno] =
> + ((red & 0xf800) >> 1) |
> + ((green & 0xf800) >> 6) |
> + ((blue & 0xf800) >> 11);
> + } else {
> + /* 0:5:6:5 */
> + ((u32*) (info->pseudo_palette))[regno] =
> + ((red & 0xf800) ) |
> + ((green & 0xfc00) >> 5) |
> + ((blue & 0xf800) >> 11);
> + }
> + break;
> + case 24:
> + case 32:
> + red >>= 8;
> + green >>= 8;
> + blue >>= 8;
> + ((u32 *)(info->pseudo_palette))[regno] =
> + (red << info->var.red.offset) |
> + (green << info->var.green.offset) |
> + (blue << info->var.blue.offset);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct fb_ops imacfb_ops = {
> + .owner = THIS_MODULE,
> + .fb_setcolreg = imacfb_setcolreg,
> + .fb_fillrect = cfb_fillrect,
> + .fb_copyarea = cfb_copyarea,
> + .fb_imageblit = cfb_imageblit,
> +};
> +
> +static int __init imacfb_setup(char *options)
> +{
> + char *this_opt;
> +
> + if (!options || !*options)
> + return 0;
> +
> + while ((this_opt = strsep(&options, ",")) != NULL) {
> + if (!*this_opt) continue;
> +
> + if (! strcmp(this_opt, "inverse"))
> + inverse=1;
> + else if (! strncmp(this_opt, "mtrr:", 5))
> + mtrr = simple_strtoul(this_opt+5, NULL, 0);
> + else if (! strcmp(this_opt, "nomtrr"))
> + mtrr=0;
> + else if (! strncmp(this_opt, "vtotal:", 7))
> + vram_total = simple_strtoul(this_opt+7, NULL, 0);
> + else if (! strncmp(this_opt, "vremap:", 7))
> + vram_remap = simple_strtoul(this_opt+7, NULL, 0);
> + else if (! strcmp(this_opt, "i17"))
> + model = M_I17;
> + else if (! strcmp(this_opt, "i20"))
> + model = M_I20;
> + else if (! strcmp(this_opt, "mini"))
> + model = M_MINI;
> + else if (! strcmp(this_opt, "macbook"))
> + model = M_MACBOOK;> +}
> +
> +static struct platform_driver imacfb_driver = {
> + .probe = imacfb_probe,
> + .driver = {
> + .name = "imacfb",
> + },
> +};
> +
> +static struct platform_device imacfb_device = {
> + .name = "imacfb",
> +};
> +
> +static int __init imacfb_init(void)
> +{
> + int ret;
> + char *option = NULL;
> +
> + /* ignore error return of fb_get_options */
> + fb_get_options("imacfb", &option);
> + imacfb_setup(option);
> + ret = platform_driver_register(&imacfb_driver);
> +
> + if (!ret) {
> + ret = platform_device_register(&imacfb_device);
> + if (ret)
> + platform_driver_unregister(&imacfb_driver);
> + }
> + return ret;
> +}
> +module_init(imacfb_init);
> +
> +MODULE_LICENSE("GPL");
> diff -uNr linux-2.6.17.orig/drivers/video/Kconfig linux-2.6.17/drivers/video/Kconfig
> --- linux-2.6.17.orig/drivers/video/Kconfig 2006-06-18 11:45:31.000000000 +0200
> +++ linux-2.6.17/drivers/video/Kconfig 2006-06-18 11:46:03.000000000 +0200
> @@ -483,6 +483,15 @@
> You will get a boot time penguin logo at no additional cost. Please
> read <file:Documentation/fb/vesafb.txt>. If unsure, say Y.
>
> +config FB_IMAC
> + bool "Intel Based Macs FB"
> + depends on (FB = y) && X86
> + select FB_CFB_FILLRECT
> + select FB_CFB_COPYAREA
> + select FB_CFB_IMAGEBLIT
> + help
> + This is the frame buffer device driver for the Inel Based Mac's
> +
> config VIDEO_SELECT
> bool
> depends on FB_VESA
> diff -uNr linux-2.6.17.orig/drivers/video/Makefile linux-2.6.17/drivers/video/Makefile
> --- linux-2.6.17.orig/drivers/video/Makefile 2006-06-18 11:45:31.000000000 +0200
> +++ linux-2.6.17/drivers/video/Makefile 2006-06-18 11:46:03.000000000 +0200
> @@ -97,6 +97,7 @@
>
> # Platform or fallback drivers go here
> obj-$(CONFIG_FB_VESA) += vesafb.o
> +obj-$(CONFIG_FB_IMAC) += imacfb.o
> obj-$(CONFIG_FB_VGA16) += vga16fb.o vgastate.o
> obj-$(CONFIG_FB_OF) += offb.o
>


> + else if (! strncmp(this_opt, "height:", 7))
> + manual_height = simple_strtoul(this_opt+7, NULL, 0);
> + else if (! strncmp(this_opt, "width:", 6))
> + manual_width = simple_strtoul(this_opt+6, NULL, 0);
> + }
> + return 0;
> +}
> +
> +#define DEFAULT_FB_MEM 1024*1024*16
> +
> +static int __init imacfb_probe(struct platform_device *dev)
> +{
> + struct fb_info *info;
> + int err;
> + unsigned int size_vmode;
> + unsigned int size_remap;
> + unsigned int size_total;
> +
> + screen_info.lfb_depth = 32;
> + screen_info.lfb_size = DEFAULT_FB_MEM / 0x10000;
> + screen_info.pages=1;
> + screen_info.blue_size = 8;
> + screen_info.blue_pos = 0;
> + screen_info.green_size = 8;
> + screen_info.green_pos = 8;
> + screen_info.red_size = 8;
> + screen_info.red_pos = 16;
> + screen_info.rsvd_size = 8;
> + screen_info.rsvd_pos = 24;
> +
> + switch(model) {
> + case M_I17:
> + screen_info.lfb_width = 1440;
> + screen_info.lfb_height = 900;
> + screen_info.lfb_linelength = 1472 * 4;
> + screen_info.lfb_base = 0x80010000;
> + break;
> + case M_NEW:
> + case M_I20:
> + screen_info.lfb_width = 1680;
> + screen_info.lfb_height = 1050;
> + screen_info.lfb_linelength = 1728 * 4;
> + screen_info.lfb_base = 0x80010000;
> + break;
> + case M_MINI:
> + screen_info.lfb_width = 1024;
> + screen_info.lfb_height = 768;
> + screen_info.lfb_linelength = 2048 * 4;
> + screen_info.lfb_base = 0x80000000;
> + break;
> + case M_MACBOOK:
> + screen_info.lfb_width = 1280;
> + screen_info.lfb_height = 800;
> + screen_info.lfb_linelength = 2048 * 4;
> + screen_info.lfb_base = 0x80000000;
> + break;
> + }
> +
> + /* if the user wants to manually specify height/width,
> + we will override the defaults */
> + /* TODO: eventually get auto-detection working */
> + if(manual_height > 0)
> + screen_info.lfb_height = manual_height;
> + if(manual_width > 0)
> + screen_info.lfb_width = manual_width;
> +
> + /*
> + static void *videomemory;
> + static u_long videomemorysize = (64*1024*1024);
> + videomemory = ioremap(0x80000000,videomemorysize);
> + memset(videomemory, 0x99, videomemorysize);
> + */
> +
> + imacfb_fix.smem_start = screen_info.lfb_base;
> + imacfb_defined.bits_per_pixel = screen_info.lfb_depth;
> + if (15 == imacfb_defined.bits_per_pixel)
> + imacfb_defined.bits_per_pixel = 16;
> + imacfb_defined.xres = screen_info.lfb_width;
> + imacfb_defined.yres = screen_info.lfb_height;
> + imacfb_fix.line_length = screen_info.lfb_linelength;
> + imacfb_fix.visual = (imacfb_defined.bits_per_pixel == 8) ?
> + FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;

If the hardware/driver does not have the capability to set the
hardware palette at 8 bpp, change the visual to STATIC_PSEUDOCOLOR
instead.

And in imacfb_setcolreg(), when the current visual is STATIC_PSEUDOCOLOR
then return nonzero.

(I did notice that everything is currently hard-coded at 32-bpp, so I'm
assuming that this will change in the future. If it does not, then this
driver has a lot of dead code.)

> +
> + /* size_vmode -- that is the amount of memory needed for the
> + * used video mode, i.e. the minimum amount of
> + * memory we need. */
> + size_vmode = imacfb_defined.yres * imacfb_fix.line_length;
> +
> + /* size_total -- all video memory we have. Used for mtrr
> + * entries, ressource allocation and bounds
> + * checking. */
> + size_total = screen_info.lfb_size * 65536;
> + if (vram_total)
> + size_total = vram_total * 1024 * 1024;
> + if (size_total < size_vmode)
> + size_total = size_vmode;
> +
> + /* size_remap -- the amount of video memory we are going to
> + * use for imacfb. With modern cards it is no
> + * option to simply use size_total as that
> + * wastes plenty of kernel address space. */
> + size_remap = size_vmode * 2;
> + if (vram_remap)
> + size_remap = vram_remap * 1024 * 1024;
> + if (size_remap < size_vmode)
> + size_remap = size_vmode;
> + if (size_remap > size_total)
> + size_remap = size_total;
> + imacfb_fix.smem_len = size_remap;
> +
> +#ifndef __i386__
> + screen_info.imacpm_seg = 0;
> +#endif
> +
> + if (!request_mem_region(imacfb_fix.smem_start, size_total, "imacfb")) {
> + printk(KERN_WARNING
> + "imacfb: cannot reserve video memory at 0x%lx\n",
> + imacfb_fix.smem_start);
> + /* We cannot make this fatal. Sometimes this comes from magic
> + spaces our resource handlers simply don't know about */
> + }
> +
> + info = framebuffer_alloc(sizeof(u32) * 256, &dev->dev);

sizeof(u32) * 256 is too much. You can use sizeof(u32) * 16.

> + if (!info) {
> + release_mem_region(imacfb_fix.smem_start, size_total);
> + return -ENOMEM;
> + }
> + info->pseudo_palette = info->par;
> + info->par = NULL;
> +
> + info->screen_base = ioremap(imacfb_fix.smem_start, imacfb_fix.smem_len);
> + if (!info->screen_base) {
> + printk(KERN_ERR
> + "imacfb: abort, cannot ioremap video memory 0x%x @ 0x%lx\n",
> + imacfb_fix.smem_len, imacfb_fix.smem_start);
> + err = -EIO;
> + goto err;
> + }
> +
> + printk(KERN_INFO "imacfb: framebuffer at 0x%lx, mapped to 0x%p, "
> + "using %dk, total %dk\n",
> + imacfb_fix.smem_start, info->screen_base,
> + size_remap/1024, size_total/1024);
> + printk(KERN_INFO "imacfb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
> + imacfb_defined.xres, imacfb_defined.yres, imacfb_defined.bits_per_pixel, imacfb_fix.line_length, screen_info.pages);
> +
> + imacfb_defined.xres_virtual = imacfb_defined.xres;
> + imacfb_defined.yres_virtual = imacfb_fix.smem_len / imacfb_fix.line_length;
> + printk(KERN_INFO "imacfb: scrolling: redraw\n");
> + imacfb_defined.yres_virtual = imacfb_defined.yres;
> +
> + /* some dummy values for timing to make fbset happy */
> + imacfb_defined.pixclock = 10000000 / imacfb_defined.xres * 1000 / imacfb_defined.yres;
> + imacfb_defined.left_margin = (imacfb_defined.xres / 8) & 0xf8;
> + imacfb_defined.hsync_len = (imacfb_defined.xres / 8) & 0xf8;
> +
> + imacfb_defined.red.offset = screen_info.red_pos;
> + imacfb_defined.red.length = screen_info.red_size;
> + imacfb_defined.green.offset = screen_info.green_pos;
> + imacfb_defined.green.length = screen_info.green_size;
> + imacfb_defined.blue.offset = screen_info.blue_pos;
> + imacfb_defined.blue.length = screen_info.blue_size;
> + imacfb_defined.transp.offset = screen_info.rsvd_pos;
> + imacfb_defined.transp.length = screen_info.rsvd_size;
> +
> + if (imacfb_defined.bits_per_pixel <= 8) {
> + depth = imacfb_defined.green.length;
> + imacfb_defined.red.length =
> + imacfb_defined.green.length =
> + imacfb_defined.blue.length =
> + imacfb_defined.bits_per_pixel;
> + }
> +
> + printk(KERN_INFO "imacfb: %s: "
> + "size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
> + (imacfb_defined.bits_per_pixel > 8) ?
> + "Truecolor" : "Pseudocolor",
> + screen_info.rsvd_size,
> + screen_info.red_size,
> + screen_info.green_size,
> + screen_info.blue_size,
> + screen_info.rsvd_pos,
> + screen_info.red_pos,
> + screen_info.green_pos,
> + screen_info.blue_pos);
> +
> + imacfb_fix.ypanstep = 0;
> + imacfb_fix.ywrapstep = 0;
> +
> + /* request failure does not faze us, as vgacon probably has this
> + * region already (FIXME) */
> + request_region(0x3c0, 32, "imacfb");
> +
> +#ifdef CONFIG_MTRR
> + if (mtrr) {
> + unsigned int temp_size = size_total;
> + unsigned int type = 0;
> +
> + switch (mtrr) {
> + case 1:
> + type = MTRR_TYPE_UNCACHABLE;
> + break;
> + case 2:
> + type = MTRR_TYPE_WRBACK;
> + break;
> + case 3:
> + type = MTRR_TYPE_WRCOMB;
> + break;
> + case 4:
> + type = MTRR_TYPE_WRTHROUGH;
> + break;
> + default:
> + type = 0;
> + break;
> + }
> +
> + if (type) {
> + int rc;
> +
> + /* Find the largest power-of-two */
> + while (temp_size & (temp_size - 1))
> + temp_size &= (temp_size - 1);
> +
> + /* Try and find a power of two to add */
> + do {
> + rc = mtrr_add(imacfb_fix.smem_start, temp_size,
> + type, 1);
> + temp_size >>= 1;
> + } while (temp_size >= PAGE_SIZE && rc == -EINVAL);
> + }
> + }
> +#endif
> +
> + info->fbops = &imacfb_ops;
> + info->var = imacfb_defined;
> + info->fix = imacfb_fix;
> + info->flags = FBINFO_FLAG_DEFAULT;
> +
> + if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
> + err = -ENOMEM;
> + goto err;
> + }
> + if (register_framebuffer(info)<0) {
> + err = -EINVAL;
> + fb_dealloc_cmap(&info->cmap);
> + goto err;
> + }
> + printk(KERN_INFO "fb%d: %s frame buffer device\n",
> + info->node, info->fix.id);
> + return 0;
> +err:
> + framebuffer_release(info);
> + release_mem_region(imacfb_fix.smem_start, size_total);
> + return err;

Balance the error path. I don't see any iounmap(), mtrr_del() or
release_region().

vesafb suffers from the same problem, but this is a new driver :-)

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