Re: [PATCH 3/4] fbdev: uvesafb driver
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2007-06-23 18:36:26
Also in:
lkml
On Sat, 23 Jun 2007 12:52:43 +0200 Michal Januszewski [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Add the uvesafb driver, an enhanced version of vesafb that makes use of a userspace helper to run x86 Video BIOS code. Signed-off-by: Michal Januszewski <spock@gentoo.org> --- drivers/video/Kconfig | 18 + drivers/video/Makefile | 1 + drivers/video/uvesafb.c | 1902 +++++++++++++++++++++++++++++++++++++++++++++++ include/video/Kbuild | 2 +- include/video/uvesafb.h | 208 ++++++ 5 files changed, 2130 insertions(+), 1 deletions(-)diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 403dac7..5cc03f9 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig@@ -585,6 +585,24 @@ config FB_TGA Say Y if you have one of those. +config FB_UVESA + tristate "Userspace VESA VGA graphics support" + depends on FB && CONNECTOR
These dependencies are insufficient.
quoted hunk ↗ jump to hunk
...--- /dev/null +++ b/drivers/video/uvesafb.c@@ -0,0 +1,1902 @@ +/* + * A framebuffer driver for VBE 2.0+ compliant video cards + * + * (c) 2007 Michal Januszewski <spock@gentoo.org> + * Loosely based upon the vesafb driver. + * + */ +#include <linux/init.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/skbuff.h> +#include <linux/timer.h> +#include <linux/completion.h> +#include <linux/connector.h> +#include <linux/random.h> +#include <linux/platform_device.h> +#include <linux/limits.h> +#include <linux/fb.h> +#include <video/edid.h> +#include <video/vga.h> +#include <video/uvesafb.h> +#include <asm/io.h> +#include <asm/mtrr.h>
Only x86 has mtrr.h: you just broke allmodconfig on all other architectures.
quoted hunk ↗ jump to hunk
+#include "edid.h" + +static struct cb_id uvesafb_cn_id = { + .idx = CN_IDX_V86D, + .val = CN_VAL_V86D_UVESAFB +}; +static struct sock *nls; +static char v86d_path[PATH_MAX] = "/sbin/v86d";
Remove the PATH_MAX, save some memory. Oh, it gets set via sysfs. hrm.
+static char v86d_started = 0; /* has v86d been started by uvesafb? */
Unneeded initialisation-to-zero. scripts/checkpatch.pl would have reported this. It in fact generates 93 warnings against this patch and from a quick scan, they all look legitimate.
quoted hunk ↗ jump to hunk
+static void uvesafb_cn_callback(void *data) +{ + struct cn_msg *msg = (struct cn_msg *)data;
unneeded cast of void*
quoted hunk ↗ jump to hunk
+ struct uvesafb_task *utask = (struct uvesafb_task *)msg->data; + struct uvesafb_ktask *task; + + if (msg->seq >= UVESAFB_TASKS_MAX) + return; + + task = uvfb_tasks[msg->seq]; + + if (!task || msg->ack != task->ack) + return; + + memcpy(&task->t, utask, sizeof(struct uvesafb_task)); + + if (task->t.buf_len && task->buf) + memcpy(task->buf, ((u8*)utask) + sizeof(struct uvesafb_task), + task->t.buf_len);
Isn't this memcpy(task->buf, utask + 1, task->t.buf_len); ?
quoted hunk ↗ jump to hunk
+ + complete(task->done); + return; +} + +static int uvesafb_helper_start(void) +{ + char *envp[] = { + "HOME=/", + "PATH=/sbin:/bin", + NULL, + }; + + char *argv[] = { + v86d_path, + NULL, + }; + + return call_usermodehelper(v86d_path, argv, envp, 1); +}
Now I'm wondering what this code actually does. It would be nice to have an overall design and implementation description in the changelog so we don't have to reverse-engineer your thoughts from your implementation... The comment over uvesafb_exec() helps.
quoted hunk ↗ jump to hunk
+static struct uvesafb_ktask *uvesafb_prep(void) +{ + struct uvesafb_ktask *task; + + task = kzalloc(sizeof(struct uvesafb_ktask), GFP_ATOMIC); + if (task) { + task->done = kzalloc(sizeof(struct completion), GFP_ATOMIC); + if (!task->done) { + kfree(task); + task = NULL; + } + } + return task; +}
GFP_ATOMIC is unreliable and should be strenuously avoided. I suspect all callers can use GFP_KERNEL here. If not, we should at least pass the gfp_t into this function as an argument so that we can use GFP_KERNEL from as many callsites as possible.
quoted hunk ↗ jump to hunk
+/* + * Execute a uvesafb task. + * + * Returns 0 if the task is executed successfully. + * + * A message sent to the userspace consists of the uvesafb_task + * struct and (optionally) a buffer. The uvesafb_task struct is + * a simplified version of uvesafb_ktask (its kernel counterpart) + * containing only the register values, flags and the length of + * the buffer. + * + * Each message is assigned a sequence number (increased linearly) + * and a random ack number. The sequence number is used as a key + * for the uvfb_tasks array which holds pointers to uvesafb_ktask + * structs for all requests. + */ +static int uvesafb_exec(struct uvesafb_ktask *task) +{ + static int seq = 0; + struct cn_msg *m; + int err; + int len = sizeof(task->t) + task->t.buf_len; + + /* Check whether the message isn't longer than the maximum + * allowed by connector */ + if (sizeof(*m) + len > CONNECTOR_MAX_MSG_SIZE) { + printk(KERN_WARNING "uvesafb: message too long (%d), " + "can't exec task\n", (int)(sizeof(*m) + len)); + return -E2BIG; + } + + m = kmalloc(sizeof(*m) + len, GFP_ATOMIC);
More GFP_ATOMIC badness
quoted hunk ↗ jump to hunk
+ if (!m) + return -ENOMEM; + + init_completion(task->done); + + memset(m, 0, sizeof(*m) + len);
kzalloc()
quoted hunk ↗ jump to hunk
+ memcpy(&m->id, &uvesafb_cn_id, sizeof(m->id)); + m->seq = seq; + m->len = len; + m->ack = random32(); + + /* uvesafb_task structure */ + memcpy(m + 1, &task->t, sizeof(task->t)); + + /* Buffer */ + memcpy((u8*)m + sizeof(task->t) + sizeof(*m), + task->buf, task->t.buf_len); + + /* Save the message ack number so that we can find the kernel + * part of this task when a reply is received from userspace. */ + task->ack = m->ack; + + /* If all slots are taken -- bail out. */ + if (uvfb_tasks[seq]) + return -EBUSY; + + /* Save a pointer to the kernel part of the task struct. */ + uvfb_tasks[seq] = task; + + err = cn_netlink_send(m, 0, gfp_any()); + if (err == -ESRCH) { + /* Try to start the userspace helper if sending + * the request failed the first time. */ + err = uvesafb_helper_start(); + if (err) { + printk(KERN_ERR "uvesafb: failed to execute %s\n", + v86d_path); + printk(KERN_ERR "uvesafb: make sure that the v86d" + "helper is installed and executable\n"); + } else { + v86d_started = 1; + err = cn_netlink_send(m, 0, gfp_any()); + } + } + kfree(m); + + if (!err && !(task->t.flags & TF_EXIT)) + err = !wait_for_completion_timeout(task->done, + msecs_to_jiffies(UVESAFB_TIMEOUT));
If you can run wait_for_completion() here then you can use GFP_KERNEL up there.
+ uvfb_tasks[seq] = NULL;
+
+ seq++;
+ if (seq >= UVESAFB_TASKS_MAX)
+ seq = 0;
+
+ return err;
+}
...
+static int __devinit uvesafb_vbe_getinfo(struct uvesafb_ktask *task,
+ struct uvesafb_par *par)
+{
+ int err;
+
+ task->t.regs.eax = 0x4f00;
+ task->t.flags = TF_VBEIB;
+ task->t.buf_len = sizeof(struct vbe_ib);
+ task->buf = (u8*) &par->vbe_ib;
+ strncpy(par->vbe_ib.vbe_signature, "VBE2", 4);
+
+ err = uvesafb_exec(task);
+ if (err || (task->t.regs.eax & 0xffff) != 0x004f) {
+ printk(KERN_ERR "uvesafb: Getting VBE info block failed "
+ "(eax=0x%x, err=%x)\n", (u32)task->t.regs.eax,
+ err);
+ return -EINVAL;
+ }
+
+ if (par->vbe_ib.vbe_version < 0x0200) {
+ printk(KERN_ERR "uvesafb: Sorry, pre-VBE 2.0 cards are "
+ "not supported.\n");
+ return -EINVAL;
+ }
+
+ if (!par->vbe_ib.mode_list_ptr) {
+ printk(KERN_ERR "uvesafb: Missing mode list!\n");
+ return -EINVAL;whitespace went wonky here.
quoted hunk ↗ jump to hunk
+ } + + printk(KERN_INFO "uvesafb: "); + + /* Convert string pointers and the mode list pointer into + * usable addresses. Print informational messages about the + * video adapter and its vendor. */
Commenting style is /* Convert string pointers and the mode list pointer into * usable addresses. Print informational messages about the * video adapter and its vendor. */ or /* * Convert string pointers and the mode list pointer into * usable addresses. Print informational messages about the * video adapter and its vendor. */
quoted hunk ↗ jump to hunk
+ if (par->vbe_ib.oem_vendor_name_ptr) + printk("%s, ", + (char*)(task->buf + par->vbe_ib.oem_vendor_name_ptr)); + + if (par->vbe_ib.oem_product_name_ptr) + printk("%s, ", + (char*)(task->buf + par->vbe_ib.oem_product_name_ptr)); + + if (par->vbe_ib.oem_product_rev_ptr) + printk("%s, ", + (char*)(task->buf + par->vbe_ib.oem_product_rev_ptr)); + + if (par->vbe_ib.oem_string_ptr) + printk("OEM: %s, ", + (char*)(task->buf + par->vbe_ib.oem_string_ptr)); + + printk("VBE v%d.%d\n", ((par->vbe_ib.vbe_version & 0xff00) >> 8), + par->vbe_ib.vbe_version & 0xff); + + return 0; +} + +static int __devinit uvesafb_vbe_getmodes(struct uvesafb_ktask *task, + struct uvesafb_par *par) +{ + int off = 0, err; + u16 *mode; + + par->vbe_modes_cnt = 0; + + /* Count available modes. */ + mode = (u16*) (((u8*)&par->vbe_ib) + par->vbe_ib.mode_list_ptr); + while (*mode != 0xffff) { + par->vbe_modes_cnt++; + mode++; + } + + par->vbe_modes = kzalloc(sizeof(struct vbe_mode_ib) * + par->vbe_modes_cnt, GFP_KERNEL); + if (!par->vbe_modes) + return -ENOMEM; + + /* Get info about all available modes. */ + mode = (u16*) (((u8*)&par->vbe_ib) + par->vbe_ib.mode_list_ptr); + while (*mode != 0xffff) { + struct vbe_mode_ib *mib; + + uvesafb_reset(task); + task->t.regs.eax = 0x4f01; + task->t.regs.ecx = (u32) *mode; + task->t.flags = TF_BUF_RET | TF_BUF_ESDI; + task->t.buf_len = sizeof(struct vbe_mode_ib); + task->buf = (u8*) (par->vbe_modes + off); + + err = uvesafb_exec(task); + if (err || (task->t.regs.eax & 0xffff) != 0x004f) { + printk(KERN_ERR "uvesafb: Getting mode info block " + "for mode 0x%x failed (eax=0x%x, err=%x)\n", + *mode, (u32)task->t.regs.eax, err); + return -EINVAL; + } + + mib = (struct vbe_mode_ib*)task->buf;
Perhaps uvesafb_ktask.buf should be void* so we can lose some typecasts.
quoted hunk ↗ jump to hunk
+ mib->mode_id = *mode; + + /* We only want modes that are supported with the current + * hardware configuration, color, graphics and that have + * support for the LFB. */ + if ((mib->mode_attr & VBE_MODE_MASK) == VBE_MODE_MASK && + mib->bits_per_pixel >= 8) { + off++; + } else { + par->vbe_modes_cnt--; + } + mode++; + mib->depth = mib->red_len + mib->green_len + mib->blue_len; + + /* Handle 8bpp modes and modes with broken color component + * lengths. */ + if (mib->depth == 0 || (mib->depth == 24 && + mib->bits_per_pixel == 32)) + mib->depth = mib->bits_per_pixel; + } + + return 0; +} + +#ifdef __i386__
CONFIG_X86 would be more typical. Be aware that CONFIG_X86 is true for both i386 and x86_64. You don't state whether this code works on x86_64. If it can, it should.
quoted hunk ↗ jump to hunk
+static int __devinit uvesafb_vbe_getpmi(struct uvesafb_ktask *task, + struct uvesafb_par *par) +{ + int i, err; + + uvesafb_reset(task); + task->t.regs.eax = 0x4f0a; + task->t.regs.ebx = 0x0; + err = uvesafb_exec(task); + + if ((task->t.regs.eax & 0xffff) != 0x4f || task->t.regs.es < 0xc000) { + par->pmi_setpal = par->ypan = 0; + } else { + par->pmi_base = (u16*)phys_to_virt(((u32)task->t.regs.es << 4) + + task->t.regs.edi); + par->pmi_start = (void*)((char*)par->pmi_base + + par->pmi_base[1]); + par->pmi_pal = (void*)((char*)par->pmi_base + + par->pmi_base[2]); + printk(KERN_INFO "uvesafb: protected mode interface info at " + "%04x:%04x\n", + (u16)task->t.regs.es, (u16)task->t.regs.edi); + printk(KERN_INFO "uvesafb: pmi: set display start = %p, " + "set palette = %p\n", par->pmi_start, + par->pmi_pal); + + if (par->pmi_base[3]) { + printk(KERN_INFO "uvesafb: pmi: ports = "); + for (i = par->pmi_base[3]/2; + par->pmi_base[i] != 0xffff; i++) + printk("%x ", par->pmi_base[i]); + printk("\n"); + + if (par->pmi_base[i] != 0xffff) { + printk(KERN_INFO "uvesafb: can't handle memory" + " requests, pmi disabled\n"); + par->ypan = par->pmi_setpal = 0; + } + } + } + return 0; +} +#endif
You might care to cc "H. Peter Anvin" [off-list ref] on the next version of this patch - he's a whizz at this sort of low-level x86 bios communication stuff.
quoted hunk ↗ jump to hunk
+/* Check whether a video mode is supported by the Video BIOS and is + * compatible with the monitor limits. */ +static int __devinit uvesafb_is_valid_mode(struct fb_videomode *mode, + struct fb_info *info) +{ + if (info->monspecs.gtf) { + fb_videomode_to_var(&info->var, mode); + if (fb_validate_mode(&info->var, info)) + return -1; + } + + if (uvesafb_vbe_find_mode(info->par, mode->xres, mode->yres, 8, + UVESAFB_NEED_EXACT_RES) == -1) + return -1; + + return 0; +} + +static int __devinit uvesafb_vbe_getedid(struct uvesafb_ktask *task, + struct fb_info *info) +{ + struct uvesafb_par *par = (struct uvesafb_par *)info->par;
fb_info.par has type void* hence all casts such as the above can and should be removed.
quoted hunk ↗ jump to hunk
+ int err = 0; + + if (noedid || par->vbe_ib.vbe_version < 0x0300) + return -EINVAL; + + task->t.regs.eax = 0x4f15; + task->t.regs.ebx = 0; + task->t.regs.ecx = 0; + task->t.buf_len = 0; + task->t.flags = 0; + + err = uvesafb_exec(task); + + if ((task->t.regs.eax & 0xffff) != 0x004f || err) + return -EINVAL; + + if ((task->t.regs.ebx & 0x3) == 3) { + printk(KERN_INFO "uvesafb: VBIOS/hardware supports both " + "DDC1 and DDC2 transfers\n"); + } else if ((task->t.regs.ebx & 0x3) == 2) { + printk(KERN_INFO "uvesafb: VBIOS/hardware supports DDC2 " + "transfers\n"); + } else if ((task->t.regs.ebx & 0x3) == 1) { + printk(KERN_INFO "uvesafb: VBIOS/hardware supports DDC1 " + "transfers\n"); + } else { + printk(KERN_INFO "uvesafb: VBIOS/hardware doesn't support " + "DDC transfers\n"); + return -EINVAL; + } + + task->t.regs.eax = 0x4f15; + task->t.regs.ebx = 1; + task->t.regs.ecx = task->t.regs.edx = 0; + task->t.flags = TF_BUF_RET | TF_BUF_ESDI; + task->t.buf_len = EDID_LENGTH; + task->buf = kzalloc(EDID_LENGTH, GFP_KERNEL); + + err = uvesafb_exec(task); + + if ((task->t.regs.eax & 0xffff) == 0x004f && !err) { + fb_edid_to_monspecs(task->buf, &info->monspecs); + + if (info->monspecs.vfmax && info->monspecs.hfmax) { + /* If the maximum pixel clock wasn't specified in + * the EDID block, set it to 300 MHz. */ + if (info->monspecs.dclkmax == 0) + info->monspecs.dclkmax = 300 * 1000000; + info->monspecs.gtf = 1; + } + } else { + err = -EINVAL; + } + + kfree(task->buf); + return err; +} + +static void __devinit uvesafb_vbe_getmonspecs(struct uvesafb_ktask *task, + struct fb_info *info) +{ + struct uvesafb_par *par = info->par; + int i; + memset(&info->monspecs, 0, sizeof(struct fb_monspecs));
Missing a newline above.
quoted hunk ↗ jump to hunk
+ /* If we don't get all necessary data from the EDID block, + * mark it as incompatible with the GTF. */ + if (uvesafb_vbe_getedid(task, info)) + info->monspecs.gtf = 0; + + /* Kernel command line overrides. */ + if (maxclk) + info->monspecs.dclkmax = maxclk * 1000000; + if (maxvf) + info->monspecs.vfmax = maxvf; + if (maxhf) + info->monspecs.hfmax = maxhf * 1000; + + /* In case DDC transfers are not supported the user can provide + * monitor limits manually. Lower limits are set to "safe" values. */ + if (info->monspecs.gtf == 0 && maxclk && maxvf && maxhf) { + info->monspecs.dclkmin = 0; + info->monspecs.vfmin = 60; + info->monspecs.hfmin = 29000; + info->monspecs.gtf = 1; + } + + if (info->monspecs.gtf) + printk(KERN_INFO + "uvesafb: monitor limits: vf = %d Hz, hf = %d kHz, " + "clk = %d MHz\n", info->monspecs.vfmax, + (int)(info->monspecs.hfmax / 1000), + (int)(info->monspecs.dclkmax / 1000000)); + else + printk(KERN_INFO "uvesafb: no monitor limits have been set\n"); + + /* Add VBE modes to the modelist. */ + for (i = 0; i < par->vbe_modes_cnt; i++) { + struct fb_var_screeninfo var; + struct vbe_mode_ib *mode; + struct fb_videomode vmode; + + mode = &par->vbe_modes[i]; + memset(&var, 0, sizeof(var)); + + var.xres = mode->x_res; + var.yres = mode->y_res; + + fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60, &var, info); + fb_var_to_videomode(&vmode, &var); + fb_add_videomode(&vmode, &info->modelist); + } + + /* Add valid VESA modes to our modelist. */ + for (i = 0; i < VESA_MODEDB_SIZE; i++) { + if (!uvesafb_is_valid_mode((struct fb_videomode*) + &vesa_modes[i], info)) + fb_add_videomode(&vesa_modes[i], &info->modelist); + } + + for (i = 0; i < info->monspecs.modedb_len; i++) { + if (!uvesafb_is_valid_mode(&info->monspecs.modedb[i], info)) + fb_add_videomode(&info->monspecs.modedb[i], + &info->modelist); + } + + return; +} + +static void __devinit uvesafb_vbe_getstatesize(struct uvesafb_ktask *task, + struct uvesafb_par *par) +{ + int err; + uvesafb_reset(task);
Here also.
quoted hunk ↗ jump to hunk
+ /* Get the VBE state buffer size. We want all available + * hardware state data (CL = 0x0f). */ + task->t.regs.eax = 0x4f04; + task->t.regs.ecx = 0x000f; + task->t.regs.edx = 0x0000; + task->t.flags = 0; + + err = uvesafb_exec(task); + + if (err || (task->t.regs.eax & 0xffff) != 0x004f) { + printk(KERN_WARNING "uvesafb: VBE state buffer size " + "cannot be determined (eax=0x%x, err=%d)\n", + task->t.regs.eax, err); + par->vbe_state_size = 0; + return; + } + + par->vbe_state_size = 64 * (task->t.regs.ebx & 0xffff); +} + +static int __devinit uvesafb_vbe_init(struct fb_info *info) +{ + struct uvesafb_ktask *task = NULL; + struct uvesafb_par *par = info->par; + int err; + + task = uvesafb_prep(); + if (!task) + return -ENOMEM; + + if ((err = uvesafb_vbe_getinfo(task, par)))
checkpatch.pl will do my work here..
quoted hunk ↗ jump to hunk
+ goto out; + if ((err = uvesafb_vbe_getmodes(task, par))) + goto out; + par->nocrtc = nocrtc; +#ifdef __i386__ + par->pmi_setpal = pmi_setpal; + par->ypan = ypan; + + if (par->pmi_setpal || par->ypan) + uvesafb_vbe_getpmi(task, par); +#else + /* The protected mode interface is not available on non-x86. */ + par->pmi_setpal = par->ypan = 0; +#endif + + INIT_LIST_HEAD(&info->modelist); + uvesafb_vbe_getmonspecs(task, info); + uvesafb_vbe_getstatesize(task, par); + +out: uvesafb_free(task); + return err; +} + +static int __devinit uvesafb_vbe_init_mode(struct fb_info *info) +{ + struct list_head *pos; + struct fb_modelist *modelist; + struct fb_videomode *mode; + struct uvesafb_par *par = info->par; + int i, modeid; + + /* Has the user requested a specific VESA mode? */ + if (vbemode) { + for (i = 0; i < par->vbe_modes_cnt; i++) { + if (par->vbe_modes[i].mode_id == vbemode) { + fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60, + &info->var, info); + /* With pixclock set to 0, the default BIOS + * timings will be used in set_par(). */ + info->var.pixclock = 0; + modeid = i; + goto gotmode; + } + } + printk(KERN_INFO "uvesafb: requested VBE mode 0x%x is " + "unavailable\n", vbemode); + vbemode = 0; + } + + i = 0; + list_for_each(pos, &info->modelist) { + i++; + }
Unneeded braces
+ mode = kzalloc(i * sizeof(*mode), GFP_KERNEL);
Check for and handle the allocation failure, please.
quoted hunk ↗ jump to hunk
+ i = 0; + list_for_each(pos, &info->modelist) { + modelist = list_entry(pos, struct fb_modelist, list); + mode[i] = modelist->mode; + i++; + } + + if (!mode_option) + mode_option = UVESAFB_DEFAULT_MODE; + + i = fb_find_mode(&info->var, info, mode_option, mode, i, + NULL, 8); + + kfree(mode); + + /* fb_find_mode() failed */ + if (i == 0 || i >= 3) { + info->var.xres = 640; + info->var.yres = 480; + mode = (struct fb_videomode*) + fb_find_best_mode(&info->var, &info->modelist); + + if (mode) { + fb_videomode_to_var(&info->var, mode); + } else { + modeid = par->vbe_modes[0].mode_id; + fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60, + &info->var, info); + goto gotmode; + } + } + + /* Look for a matching VBE mode. */ + modeid = uvesafb_vbe_find_mode(par, info->var.xres, info->var.yres, + info->var.bits_per_pixel, UVESAFB_NEED_EXACT_RES); + + if (modeid == -1) + return -EINVAL; + +gotmode: + uvesafb_setup_var(&info->var, info, &par->vbe_modes[modeid]); + + /* If we are not VBE3.0+ compliant, we're done -- the BIOS will + * ignore our mode timings anyway. */ + if (par->vbe_ib.vbe_version < 0x0300) + fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60, + &info->var, info); + + return modeid; +} + +static int uvesafb_setpalette(struct uvesafb_pal_entry *entries, int count, + int start, struct fb_info *info) +{ + struct uvesafb_ktask *task; + struct uvesafb_par *par = info->par; + int i = par->mode_idx; + int err = 0; + + /* We support palette modifications for 8 bpp modes only, so + * there can never be more than 256 entries. */ + if (start + count > 256) + return -EINVAL; + + /* Use VGA registers if mode is VGA-compatible. */ + if (i >= 0 && i < par->vbe_modes_cnt && + par->vbe_modes[i].mode_attr & VBE_MODE_VGACOMPAT) { + for (i = 0; i < count; i++) { + outb_p(start + i, dac_reg); + outb_p(entries[i].red, dac_val); + outb_p(entries[i].green, dac_val); + outb_p(entries[i].blue, dac_val); + } + } else if (par->pmi_setpal) { + __asm__ __volatile__( + "call *(%%esi)" + : /* no return value */ + : "a" (0x4f09), /* EAX */ + "b" (0), /* EBX */ + "c" (count), /* ECX */ + "d" (start), /* EDX */ + "D" (entries), /* EDI */ + "S" (&par->pmi_pal)); /* ESI */
uh, so we're CONFIG_X86_32-only, yes?
+ } else {
+ task = uvesafb_prep();
+ if (!task)
+ return -ENOMEM;
+
+ task->t.regs.eax = 0x4f09;
+ task->t.regs.ebx = 0x0;
+ task->t.regs.ecx = count;
+ task->t.regs.edx = start;
+ task->t.flags = TF_BUF_ESDI;
+ task->t.buf_len = sizeof(struct uvesafb_pal_entry) * count;
+ task->buf = (u8*) entries;
+
+ err = uvesafb_exec(task);
+ if ((task->t.regs.eax & 0xffff) != 0x004f)
+ err = 1;
+
+ uvesafb_free(task);
+ }
+ return err;
+}
+
...
+
+static struct device_attribute device_attrs[] = {
+ __ATTR(vbe_version, S_IRUGO, uvesafb_show_vbe_ver, NULL),
+ __ATTR(vbe_modes, S_IRUGO, uvesafb_show_vbe_modes, NULL),
+ __ATTR(oem_vendor, S_IRUGO, uvesafb_show_vendor, NULL),
+ __ATTR(oem_product_name, S_IRUGO, uvesafb_show_product_name, NULL),
+ __ATTR(oem_product_rev, S_IRUGO, uvesafb_show_product_rev, NULL),
+ __ATTR(oem_string, S_IRUGO, uvesafb_show_oem_string, NULL),
+ __ATTR(nocrtc, S_IRUGO | S_IWUSR, uvesafb_show_nocrtc,
+ uvesafb_store_nocrtc),
+};Can we use an attribute group here?
quoted hunk ↗ jump to hunk
+static void __devinit uvesafb_create_sysfs(struct device *dev, + struct fb_info *info) +{ + int i, err = 0; + + for (i = 0; i < ARRAY_SIZE(device_attrs); i++) { + err |= device_create_file(dev, &device_attrs[i]); + }
Unneeded braces
quoted hunk ↗ jump to hunk
+ if (err != 0) + printk(KERN_WARNING "fb%d: failed to register attributes\n", + info->node); +} + +static void __devinit uvesafb_remove_sysfs(struct device *dev) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(device_attrs); i++) { + device_remove_file(dev, &device_attrs[i]); + }
ditto
quoted hunk ↗ jump to hunk
+} + +static int __devinit uvesafb_probe(struct platform_device *dev) +{ + struct fb_info *info; + struct vbe_mode_ib *mode = NULL; + struct uvesafb_par *par; + int err = 0, i; + + info = framebuffer_alloc(sizeof(*par) + sizeof(u32) * 256, &dev->dev); + if (!info) + return -ENOMEM; + + par = info->par; + + if ((err = uvesafb_vbe_init(info))) { + printk(KERN_ERR "uvesafb: vbe_init() failed with %d\n", err); + goto out; + } + + info->fbops = &uvesafb_ops; + + i = uvesafb_vbe_init_mode(info); + if (i < 0) { + err = -EINVAL; + goto out; + } else { + mode = &par->vbe_modes[i]; + } + + if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) { + err = -ENXIO; + goto out; + } + + uvesafb_init_info(info, mode); + + if (!request_mem_region(info->fix.smem_start, info->fix.smem_len, + "uvesafb")) { + printk(KERN_WARNING "uvesafb: cannot reserve video memory at " + "0x%lx\n", info->fix.smem_start); + /* We cannot make this fatal. Sometimes this comes from magic + spaces our resource handlers simply don't know about. */
so... what happens? The driver starts altering mrmory regions which it doesn't own?
+ }
+
+ info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
+
+ if (!info->screen_base) {
+ printk(KERN_ERR
+ "uvesafb: abort, cannot ioremap 0x%x bytes of video "
+ "memory at 0x%lx\n",
+ info->fix.smem_len, info->fix.smem_start);
+ err = -EIO;
+ goto out_mem;
+ }
+
+ if (!request_region(0x3c0, 32, "uvesafb")) {
+ printk(KERN_ERR "uvesafb: request region 0x3c0-0x3e0 failed\n");
+ err = -EIO;
+ goto out_unmap;
+ }
+
+ uvesafb_init_mtrr(info);
+ platform_set_drvdata(dev, info);
+
+ if (register_framebuffer(info) < 0) {
+ printk(KERN_ERR
+ "uvesafb: failed to register framebuffer device\n");
+ err = -EINVAL;
+ goto out_reg;
+ }
+
+ printk(KERN_INFO "uvesafb: framebuffer at 0x%lx, mapped to 0x%p, "
+ "using %dk, total %dk\n", info->fix.smem_start,
+ info->screen_base, info->fix.smem_len/1024,
+ par->vbe_ib.total_memory * 64);
+ printk(KERN_INFO "fb%d: %s frame buffer device\n", info->node,
+ info->fix.id);
+
+ uvesafb_create_sysfs(&dev->dev, info);
+ return 0;
+
+out_reg:
+ release_region(0x3c0, 32);
+out_unmap:
+ iounmap(info->screen_base);
+out_mem:
+ release_mem_region(info->fix.smem_start, info->fix.smem_len);
+ if (!list_empty(&info->modelist))
+ fb_destroy_modelist(&info->modelist);
+ fb_destroy_modedb(info->monspecs.modedb);
+ fb_dealloc_cmap(&info->cmap);
+out:
+ if (par->vbe_modes)
+ kfree(par->vbe_modes);
+
+ framebuffer_release(info);
+ return err;
+}
...
+#ifndef MODULE
+static int __devinit uvesafb_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, "redraw"))
+ ypan = 0;
+ else if (!strcmp(this_opt, "ypan"))
+ ypan = 1;
+ else if (!strcmp(this_opt, "ywrap"))
+ ypan = 2;
+ else if (!strcmp(this_opt, "vgapal"))
+ pmi_setpal = 0;
+ else if (!strcmp(this_opt, "pmipal"))
+ pmi_setpal = 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 (!strcmp(this_opt, "nocrtc"))
+ nocrtc = 1;
+ else if (!strcmp(this_opt, "noedid"))
+ noedid = 1;
+ else if (!strcmp(this_opt, "noblank"))
+ blank = 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 (!strncmp(this_opt, "maxhf:", 6))
+ maxhf = simple_strtoul(this_opt + 6, NULL, 0);
+ else if (!strncmp(this_opt, "maxvf:", 6))
+ maxvf = simple_strtoul(this_opt + 6, NULL, 0);
+ else if (!strncmp(this_opt, "maxclk:", 7))
+ maxclk = simple_strtoul(this_opt + 7, NULL, 0);
+ else if (!strncmp(this_opt, "vbemode:", 8))
+ vbemode = simple_strtoul(this_opt + 8, NULL,0);
+ else if (this_opt[0] >= '0' && this_opt[0] <= '9') {
+ mode_option = this_opt;
+ } else {
+ printk(KERN_WARNING
+ "uvesafb: unrecognized option %s\n", this_opt);
+ }
+ }
+
+ return 0;
+}
+#endif /* !MODULE */The #ifdef MODULE is unpleasing. Can we use module_param() here? That'll work for both modular and non-modular case.
quoted hunk ↗ jump to hunk
+#define uvesafb_free(task) \ +do { \ + if (task) { \ + if (task->done) \ + kfree(task->done); \ + kfree(task); \ + task = NULL; \ + } \ +} while(0)
I don't think this need to be a macro? Code should be written in C unless there is some reason why it _must_ be a macro. Oh, it sneakily zeroes a variable within the caller's context. Ow. Can we not do that please? Someone who is reading this code will see a call to a "function" called uvesafb_free() and the last thing they will expect is that "function" magically zeroed out a local variable.
quoted hunk ↗ jump to hunk
+#define uvesafb_reset(task) \ +do { \ + struct completion *cpl = task->done; \ + memset(task, 0, sizeof(struct uvesafb_ktask)); \ + task->done = cpl; \ +} while(0) \
This can be a regular C function.
quoted hunk ↗ jump to hunk
+static int uvesafb_exec(struct uvesafb_ktask *tsk); + +#define UVESAFB_NEED_EXACT_RES 1 +#define UVESAFB_NEED_EXACT_DEPTH 2 + +struct uvesafb_par { + struct vbe_ib vbe_ib; /* VBE Info Block */ + struct vbe_mode_ib *vbe_modes; /* list of supported VBE modes */ + int vbe_modes_cnt; + + u8 nocrtc; + u8 ypan; /* 0 - nothing, 1 - ypan, 2 - ywrap */ + u8 pmi_setpal; /* pmi for palette changes */ + u16 *pmi_base; /* protected mode interface location */ + void (*pmi_start)(void); + void (*pmi_pal)(void); + + u8 *vbe_state_orig; /* original hardware state, before the driver was loaded */ + u8 *vbe_state_saved; /* state saved by fb_save_state */ + int vbe_state_size; + atomic_t ref_count; + + int mode_idx; + struct vbe_crtc_ib crtc; +}; + +#endif
A comment on the endif would be nice.
+#endif /* _UVESAFB_H */
------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/