[Linux-fbdev-devel] [PATCH 6/9] ps3: Virtual Frame Buffer Driver
Bernhard Fischer
rep.dot.nop at gmail.com
Fri Jan 26 08:36:37 EST 2007
Just some minor cosmetics..
On Thu, Jan 25, 2007 at 06:50:44PM +0100, Geert Uytterhoeven wrote:
>--- /dev/null
>+++ ps3-linux/drivers/video/ps3fb.c
>@@ -0,0 +1,1266 @@
>+/*
>+ * linux/drivers/video/ps3fb.c -- PS3 GPU frame buffer device
>+ *
>+ * Copyright (C) 2006 Sony Computer Entertainment Inc.
>+ * Copyright (C) 2006-2007 Sony Corporation
>+ *
>+ * this file is based on :
>+ *
>+ * linux/drivers/video/vfb.c -- Virtual frame buffer device
>+ *
>+ * Copyright (C) 2002 James Simmons
>+ *
>+ * Copyright (C) 1997 Geert Uytterhoeven
One (C) has spaces, yours a tab.
>+struct ps3fb_priv {
>+ unsigned long videomemory_phys;
>+ unsigned long videomemorysize;
Is the naming with and without underscore intentional?
>+static int ps3fb_get_res_table(u32 xres, u32 yres)
>+{
>+ int i, full_mode;
unsigned?
>+ u32 x, y, f = 0;
>+
>+ full_mode = (ps3fb_mode & PS3FB_FULL_MODE_BIT) ? PS3FB_RES_FULL : 0;
>+ for (i = 0;; i++) {
>+ x = ps3fb_res[i].xres;
>+ y = ps3fb_res[i].yres;
>+ f = ps3fb_res[i].type;
Why did you set f=0 above?
>+
>+static const struct fb_videomode *ps3fb_default_mode(void)
>+{
>+ u32 mode = ps3fb_mode & PS3AV_MODE_MASK;
>+ u32 flags = ps3fb_mode & ~PS3AV_MODE_MASK;
>+
>+ if (mode < 1 || mode > 13)
>+ return NULL;
smaller if you set flags only after this check?
>+static int ps3fb_sync(u32 frame)
>+{
>+ int i;
>+ u32 xres, yres;
>+ u64 head, fb_ioif, offset;
#if defined HEAD_A || defined HEAD_B
u64 head;
#endif
Resp, remove it and pass the value in directly like you do in
ps3fb_set_sync()?
It's unlikely that neither HEAD_A nor HEAD_B are defined, i assume, so
the check is most likely not needed?
>+ u64 sync = 1;
you don't seem to use sync much, perhaps use just 1 below?
>+ int status;
I'd reuse status and drop i, perhaps.
>+
>+ i = ps3fb.res_index;
>+ xres = ps3fb_res[i].xres;
>+ yres = ps3fb_res[i].yres;
>+
>+ if (frame > ps3fb.num_frames - 1) {
>+ printk(KERN_WARNING "%s: invalid frame number (%u)\n",
>+ __FUNCTION__, frame);
>+ return -EINVAL;
>+ }
>+ offset = xres * yres * BPP * frame;
>+
>+ fb_ioif = GPU_IOIF + FB_OFF(i) + offset;
>+ status = lv1_gpu_context_attribute(ps3fb.context_handle,
>+ L1GPU_CONTEXT_ATTRIBUTE_FB_BLIT,
>+ offset, fb_ioif,
>+ (sync << 32) | (xres << 16) | yres,
>+ xres * BPP); /* line_length */
>+ if (status)
>+ printk(KERN_ERR "%s: lv1_gpu_context_attribute FB_BLIT failed: %d\n",
>+ __FUNCTION__, status);
>+#ifdef HEAD_A
>+ head = 0;
>+ status = lv1_gpu_context_attribute(ps3fb.context_handle,
>+ L1GPU_CONTEXT_ATTRIBUTE_DISPLAY_FLIP,
>+ head, offset, 0, 0);
>+ if (status)
>+ printk(KERN_ERR "%s: lv1_gpu_context_attribute FLIP failed: %d\n",
>+ __FUNCTION__, status);
>+#endif
>+#ifdef HEAD_B
>+ head = 1;
>+ status = lv1_gpu_context_attribute(ps3fb.context_handle,
>+ L1GPU_CONTEXT_ATTRIBUTE_DISPLAY_FLIP,
>+ head, offset, 0, 0);
>+ if (status)
>+ printk(KERN_ERR "%s: lv1_gpu_context_attribute FLIP failed: %d\n",
>+ __FUNCTION__, status);
>+#endif
>+ return 0;
>+}
[snip ps3fb_check_var that doesn't need a mode variable currently so i'd
drop it]
>+/* This routine actually sets the video mode. It's in here where we
>+ * the hardware state info->par and fix which can be affected by the
>+ * change in par. For this driver it doesn't do much.
>+ */
I can't parse the second sentence, something is missing..
>+static int ps3fb_set_par(struct fb_info *info)
>+{
>+ unsigned int mode;
>+ int i;
>+ unsigned long offset;
>+ static int first = 1;
>+
>+ DPRINTK("xres:%d xv:%d yres:%d yv:%d clock:%d\n",
>+ info->var.xres, info->var.xres_virtual,
>+ info->var.yres, info->var.yres_virtual, info->var.pixclock);
>+ i = ps3fb_get_res_table(info->var.xres, info->var.yres);
>+ ps3fb.res_index = i;
>+
>+ mode = ps3fb_find_mode(&info->var, &info->fix.line_length);
>+ if (!mode)
>+ return -EINVAL;
>+
>+ offset = FB_OFF(i) + VP_OFF(i);
>+ info->fix.smem_len = ps3fb.videomemorysize-offset;
spaces around the minus would make this more readable
>+ info->screen_base = (char __iomem *)ps3fb.xdr_ea + offset;
>+ memset(ps3fb.xdr_ea, 0, ps3fb.videomemorysize);
>+
>+ ps3fb.num_frames = ps3fb.videomemorysize/
>+ (ps3fb_res[i].xres*ps3fb_res[i].yres*BPP);
>+
>+ /* Keep the special bits we cannot set using fb_var_screeninfo */
>+ ps3fb_mode = (ps3fb_mode & ~PS3AV_MODE_MASK) | mode;
>+
>+ if (ps3av_set_video_mode(ps3fb_mode, first))
>+ return -EINVAL;
>+
>+ first = 0;
>+ return 0;
>+}
>+ /*
>+ * Most drivers don't need their own mmap function
>+ */
hm?
>+
>+static int ps3fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>+{
>+ unsigned long size = vma->vm_end - vma->vm_start;
>+ unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
>+ int i;
>+
>+ i = ps3fb_get_res_table(info->var.xres, info->var.yres);
>+ if (i == -1)
>+ return -EINVAL;
unsigned long size, offset;
if (ps3fb_get_res_table(info->var.xres, info->var.yres) == -1)
return -EINVAL;
size = vma->vm_end - vma->vm_start;
offset = vma->vm_pgoff << PAGE_SHIFT;
perhaps ?
>+
>+ if (offset + size > info->fix.smem_len)
>+ return -EINVAL;
>+ offset += ps3fb.videomemory_phys + FB_OFF(i) + VP_OFF(i);
>+ if (remap_pfn_range(vma, vma->vm_start, offset >> PAGE_SHIFT,
>+ size, vma->vm_page_prot))
>+ return -EAGAIN;
>+ printk(KERN_DEBUG "ps3fb: mmap framebuffer P(%lx)->V(%lx)\n", offset,
>+ vma->vm_start);
>+ return 0;
>+}
>+
>+ /*
>+ * Blank the display
>+ */
>+
>+static int ps3fb_blank(int blank, struct fb_info *info)
>+{
>+ int retval = 0;
Not sure why you set the retval here while ps3av_set_av_video_mute seem
to allways return 0 or -1 and retval will always be set below?
>+ DPRINTK("%s: blank:%d\n", __FUNCTION__, blank);
>+ switch (blank) {
>+ case FB_BLANK_POWERDOWN:
>+ case FB_BLANK_HSYNC_SUSPEND:
>+ case FB_BLANK_VSYNC_SUSPEND:
>+ case FB_BLANK_NORMAL:
>+ retval = ps3av_video_mute(1); /* mute on */
>+ if (!retval)
>+ ps3fb.is_blanked = 1;
>+ break;
>+ default: /* unblank */
>+ retval = ps3av_video_mute(0); /* mute off */
>+ if (!retval)
>+ ps3fb.is_blanked = 0;
>+ break;
>+ }
>+ return retval;
>+}
>+int ps3fb_wait_for_vsync(u32 crtc)
>+{
->+ int ret;
>+ u64 count;
>+
>+ count = ps3fb.vblank_count;
>+ ret = wait_event_interruptible_timeout(ps3fb.wait_vsync,
>+ count != ps3fb.vblank_count,
>+ HZ / 10);
>+ if (!ret)
>+ return -ETIMEDOUT;
>+
>+ return 0;
>+}
>+
>+EXPORT_SYMBOL(ps3fb_wait_for_vsync);
>+
>+int ps3fb_flip_ctl(int on)
void?
>+{
>+ if (on) {
>+ if (atomic_read(&ps3fb.ext_flip) > 0) {
>+ atomic_dec(&ps3fb.ext_flip);
>+ }
>+ } else {
>+ atomic_inc(&ps3fb.ext_flip);
>+ }
>+ return 0;
>+}
>+
>+EXPORT_SYMBOL(ps3fb_flip_ctl);
>+
>+ /*
>+ * ioctl
>+ */
>+
>+static int ps3fb_ioctl(struct fb_info *info, unsigned int cmd,
>+ unsigned long arg)
>+{
>+ void __user *argp = (void __user *)arg;
>+ u32 val, old_mode;
>+ int retval = 0;
>+
>+ switch (cmd) {
>+ case FBIOGET_VBLANK:
>+ {
>+ struct fb_vblank vblank;
>+ DPRINTK("FBIOGET_VBLANK:\n");
>+ retval = ps3fb_get_vblank(&vblank);
>+ if (!retval) {
>+ if (copy_to_user(argp, &vblank,
>+ sizeof(vblank))) {
>+ retval = -EFAULT;
>+ }
>+ }
>+ break;
>+ }
>+
>+ case FBIO_WAITFORVSYNC:
>+ {
>+ u32 crt;
>+ DPRINTK("FBIO_WAITFORVSYNC:\n");
>+ if (get_user(crt, (u32 __user *) arg)) {
>+ retval = -EFAULT;
>+ break;
>+ }
>+ retval = ps3fb_wait_for_vsync(crt);
>+ break;
>+ }
>+
>+ case PS3FB_IOCTL_SETMODE:
>+ {
>+ const struct fb_videomode *mode;
>+ struct fb_var_screeninfo var;
>+
>+ if (copy_from_user(&val, argp, sizeof(val))) {
>+ retval = -EFAULT;
>+ break;
>+ }
>+ DPRINTK("PS3FB_IOCTL_SETMODE:%x\n", val);
>+ retval = -EINVAL;
I'd default to -EFAULT and eventually set retval = 0 if !copy_from and if
all went fine (like for get_mode) but that's probably not the convention.
Didn't look recently.
>+static int __init ps3fb_init(void)
>+{
>+ int ret = 0;
ret is set here and then again set by platform_driver_register() but not
used for anything else until then. s/= 0//
>+ int status;
>+#ifndef MODULE
>+ int mode;
>+ char *option = NULL;
>+
>+ if (fb_get_options("ps3fb", &option))
>+ goto err;
>+#endif
>+
>+ if (!ps3fb_videomemory.address)
>+ goto err;
>+
>+ status = ps3av_dev_open();
>+ if (status) {
>+ printk(KERN_ERR "%s: ps3av_dev_open failed\n", __FUNCTION__);
>+ goto err;
>+ }
>+
>+ /* set gpu-driver internal video mode */
>+#ifdef HEAD_A
>+ status = lv1_gpu_context_attribute(0x0, L1GPU_CONTEXT_ATTRIBUTE_DISPLAY_MODE_SET, 0, 0, 0, 0); /* head a */
>+ if (status) {
>+ printk(KERN_ERR "%s: lv1_gpu_context_attribute DISPLAY_MODE failed: %d\n",
>+ __FUNCTION__, status);
>+ goto err;
>+ }
>+#endif
>+#ifdef HEAD_B
>+ status = lv1_gpu_context_attribute(0x0, L1GPU_CONTEXT_ATTRIBUTE_DISPLAY_MODE_SET, 0, 0, 1, 0); /* head b */
>+
>+ if (status) {
>+ printk(KERN_ERR "%s: lv1_gpu_context_attribute DISPLAY_MODE failed: %d\n",
>+ __FUNCTION__, status);
>+ goto err;
>+ }
>+#endif
>+
>+ ps3fb_mode = ps3av_get_mode();
>+ DPRINTK("ps3av_mode:%d\n", ps3fb_mode);
>+#ifndef MODULE
>+ mode = ps3fb_setup(option); /* check boot option */
>+ if (mode)
>+ ps3fb_mode = mode;
>+#endif
>+ if (ps3fb_mode > 0) {
>+ u32 xres, yres;
>+ ps3av_video_mode2res(ps3fb_mode, &xres, &yres);
>+ ps3fb.res_index = ps3fb_get_res_table(xres, yres);
>+ DPRINTK("res_index:%d\n", ps3fb.res_index);
>+ } else
>+ ps3fb.res_index = GPU_RES_INDEX;
>+ ps3fb.videomemorysize = ps3fb_videomemory.size;
>+
>+ atomic_set(&ps3fb.f_count, -1); /* fbcon opens ps3fb */
>+ atomic_set(&ps3fb.ext_flip, 0); /* for flip with vsync */
>+ init_MUTEX(&ps3fb.sem);
>+ init_waitqueue_head(&ps3fb.wait_vsync);
>+ ps3fb.num_frames = 1;
>+ ret = platform_driver_register(&ps3fb_driver);
>+
>+ if (!ret) {
>+ ret = platform_device_register(&ps3fb_device);
>+ if (ret)
>+ platform_driver_unregister(&ps3fb_driver);
>+ }
>+
>+ register_reboot_notifier(&ps3fb_reboot_nb);
>+ ps3fb_set_sync();
>+
>+ return ret;
>+
>+err:
>+ return -ENXIO;
>+}
>+
cheers,
More information about the Linuxppc-dev
mailing list