Thread (34 messages) 34 messages, 8 authors, 2007-01-31

Re: [Linux-fbdev-devel] [PATCH 6/9] ps3: Virtual Frame Buffer Driver

flat view

From: Bernhard Fischer <hidden>
Date: 2007-01-25 21:43:04
Also in: linuxppc-dev

Just some minor cosmetics..

On Thu, Jan 25, 2007 at 06:50:44PM +0100, Geert Uytterhoeven wrote:
quoted hunk ↗ jump to hunk
--- /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.

quoted hunk ↗ jump to hunk
+struct ps3fb_priv {
+	unsigned long videomemory_phys;
+	unsigned long videomemorysize;
Is the naming with and without underscore intentional?

quoted hunk ↗ jump to hunk
+static int ps3fb_get_res_table(u32 xres, u32 yres)
+{
+	int i, full_mode;
unsigned?
quoted hunk ↗ jump to hunk
+	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?
quoted hunk ↗ jump to hunk
+
+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?

quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+
+	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]
quoted hunk ↗ jump to hunk
+/* 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..
quoted hunk ↗ jump to hunk
+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
quoted hunk ↗ jump to hunk
+	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;
+}
quoted hunk ↗ jump to hunk
+    /*
+     *  Most drivers don't need their own mmap function
+     */
hm?
quoted hunk ↗ jump to hunk
+
+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;
quoted hunk ↗ jump to hunk
+	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 ?
quoted hunk ↗ jump to hunk
+
+	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?
quoted hunk ↗ jump to hunk
+	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;
quoted hunk ↗ jump to hunk
+	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?
quoted hunk ↗ jump to hunk
+{
+	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.

quoted hunk ↗ jump to hunk
+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//
quoted hunk ↗ jump to hunk
+	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,
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help