Skip to contentSkip to conversation

video: Use fb_sys_write rather than open-coding in drivers

RFC PATCH In review Ryan Mallon 2 participants 6 messages Sep 20, 2013 – Feb 12, 2014 Framebuffer layer

lore
Share & tools
lore

Ryan Mallon

author
#0 lore raw

RFC PATCH 1/1 video: Use fb_sys_write rather than open-coding in drivers

Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers.

Signed-off-by Ryan Mallon <[email protected]>

---

drivers/video/auo_k190x.c +5 -41
index 8d2499d..0eeeb26 100644
@@ -362,50 +362,12 @@ out:
  * framebuffer operations
  */
 
-/*
- * this is the slow path from userspace. they can seek and write to
- * the fb. it's inefficient to do anything less than a full screen draw
- */
-static ssize_t auok190xfb_write(struct fb_info *info, const char __user *buf,
-				size_t count, loff_t *ppos)
+static int auok190xfb_sync(struct fb_info *info)
 {
 	struct auok190xfb_par *par = info->par;
-	unsigned long p = *ppos;
-	void *dst;
-	int err = 0;
-	unsigned long total_size;
-
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
-	total_size = info->fix.smem_len;
-
-	if (p > total_size)
-		return -EFBIG;
-
-	if (count > total_size) {
-		err = -EFBIG;
-		count = total_size;
-	}
-
-	if (count + p > total_size) {
-		if (!err)
-			err = -ENOSPC;
-
-		count = total_size - p;
-	}
-
-	dst = (void *)(info->screen_base + p);
-
-	if (copy_from_user(dst, buf, count))
-		err = -EFAULT;
-
-	if  (!err)
-		*ppos += count;
 
 	par->update_all(par);
-
-	return (err) ? err : count;
+	return 0;
 }
 
 static void auok190xfb_fillrect(struct fb_info *info,

@@ -565,7 +527,8 @@ static int auok190xfb_set_par(struct fb_info *info)
 static struct fb_ops auok190xfb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_read	= fb_sys_read,
-	.fb_write	= auok190xfb_write,
+	.fb_write	= fb_sys_write,
+	.fb_sync	= auok190xfb_sync,
 	.fb_fillrect	= auok190xfb_fillrect,
 	.fb_copyarea	= auok190xfb_copyarea,
 	.fb_imageblit	= auok190xfb_imageblit,

@@ -1066,6 +1029,7 @@ int auok190x_common_probe(struct platform_device *pdev,
 
 	memset(videomemory, 0, videomemorysize);
 	info->screen_base = (char *)videomemory;
+	info->screen_size = videomemorysize;
 	info->fix.smem_len = videomemorysize;
 
 	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
drivers/video/broadsheetfb.c +11 -47

79 lines not on this page · raw message

drivers/video/hecubafb.c +11 -47

79 lines not on this page · raw message

drivers/video/metronomefb.c +11 -47

78 lines not on this page · raw message

drivers/video/ssd1307fb.c +5 -27
index 44967c8..2b46790 100644
@@ -198,36 +198,12 @@ static void ssd1307fb_update_display(struct ssd1307fb_par *par)
 	kfree(array);
 }
 
-
-static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf,
-		size_t count, loff_t *ppos)
+static int ssd1307fb_sync(struct fb_info *info)
 {
 	struct ssd1307fb_par *par = info->par;
-	unsigned long total_size;
-	unsigned long p = *ppos;
-	u8 __iomem *dst;
-
-	total_size = info->fix.smem_len;
-
-	if (p > total_size)
-		return -EINVAL;
-
-	if (count + p > total_size)
-		count = total_size - p;
-
-	if (!count)
-		return -EINVAL;
-
-	dst = (void __force *) (info->screen_base + p);
-
-	if (copy_from_user(dst, buf, count))
-		return -EFAULT;
 
 	ssd1307fb_update_display(par);
-
-	*ppos += count;
-
-	return count;
+	return 0;
 }
 
 static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)

@@ -254,7 +230,8 @@ static void ssd1307fb_imageblit(struct fb_info *info, const struct fb_image *ima
 static struct fb_ops ssd1307fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_read	= fb_sys_read,
-	.fb_write	= ssd1307fb_write,
+	.fb_write	= fb_sys_write,
+	.fb_sync	= ssd1307fb_sync,
 	.fb_fillrect	= ssd1307fb_fillrect,
 	.fb_copyarea	= ssd1307fb_copyarea,
 	.fb_imageblit	= ssd1307fb_imageblit,

@@ -493,6 +470,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
 	info->var.blue.offset = 0;
 
 	info->screen_base = (u8 __force __iomem *)vmem;
+	info->screen_size = vmem_size;
 	info->fix.smem_start = (unsigned long)vmem;
 	info->fix.smem_len = vmem_size;
#1 lore raw
Quoted 7 lines · Ryan Mallon

Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers.

Signed-off-by Ryan Mallon <[email protected]>

---

Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is "wait for blit idle, optional"

Tomi

author
#2 lore raw
Quoted 6 lines · Tomi Valkeinen
Quoted 5 lines · Ryan Mallon

Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Ryan Mallon <[email protected]>

---

Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is "wait for blit idle, optional"

fb_write() in fbmem.c calls ->fb_sync() after ->fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes.

The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour.

Note that I haven't actually tested this code since I don't have any of the hardware. It was just something I noticed while digging through framebuffer driver code.

~Ryan

#3 lore raw
Quoted 18 lines · Ryan Mallon
Quoted 6 lines · Tomi Valkeinen
Quoted 5 lines · Ryan Mallon

Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Ryan Mallon <[email protected]>

---

Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is "wait for blit idle, optional"

Quoted 7 lines

fb_write() in fbmem.c calls ->fb_sync() after ->fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes.

The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour.

Well, by just looking at the function names the drivers' fb_syncs call, it sounds to me that with your patch fb_sync will update the LCD, i.e. send data to it. Doing that in fb_read sounds totally wrong.

Quoted 3 lines

Note that I haven't actually tested this code since I don't have any of the hardware. It was just something I noticed while digging through framebuffer driver code.

Ok. Well, I think it's safer to drop these, then, if it's not 100% clear that there are no unwanted side effects.

Tomi

author
#4 lore raw
Quoted 23 lines · Tomi Valkeinen
Quoted 22 lines · Ryan Mallon
Quoted 6 lines · Tomi Valkeinen
Quoted 5 lines · Ryan Mallon

Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Ryan Mallon <[email protected]>

---

Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is "wait for blit idle, optional"

fb_write() in fbmem.c calls ->fb_sync() after ->fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes.

Quoted 7 lines
Quoted 3 lines

The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour.

Well, by just looking at the function names the drivers' fb_syncs call, it sounds to me that with your patch fb_sync will update the LCD, i.e. send data to it. Doing that in fb_read sounds totally wrong.

Well, the alternative is to supply an fb_write() implementation for each driver that calls fb_sys_write(), and then updates the display. The fb_sync() additions can be removed. That would cut down the boiler-plate code, and should keep the behaviour the same.

If you don't think it is worth the effort, then the patch can just be dropped.

~Ryan

#5 lore raw
Quoted 7 lines · Ryan Mallon

Well, the alternative is to supply an fb_write() implementation for each driver that calls fb_sys_write(), and then updates the display. The fb_sync() additions can be removed. That would cut down the boiler-plate code, and should keep the behaviour the same.

If you don't think it is worth the effort, then the patch can just be dropped.

I'd be very cautious about doing cleanups on drivers that you cannot test. Small innocent looking changes can break them.

For example, your patch sets info->screen_size, which is not set currently. Does the fbdev framework use screen_size somewhere differently than smem_len? I don't know, but that could lead to small difference in operation. However, in this case, fb_sys_write() actually uses smem_len if screen_size is 0, so that change is not even needed.

ssd1307fb_write() looks a bit different than fb_sys_write. I don't know if the differences could cause issues. The other ones look copy-pasted from fb_sys_write (but I didn't look at them carefully), so perhaps those could be cleaned up safely.

Tomi