diff mbox

[RFC] video: Use fb_sys_write rather than open-coding in drivers

Message ID 523BF40B.1090002@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ryan Mallon Sept. 20, 2013, 7:06 a.m. UTC
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 <rmallon@gmail.com>
---


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tomi Valkeinen Feb. 11, 2014, 2:06 p.m. UTC | #1
On 20/09/13 10:06, Ryan Mallon wrote:
> 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 <rmallon@gmail.com>
> ---

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
Ryan Mallon Feb. 11, 2014, 7:07 p.m. UTC | #2
On 12/02/14 03:06, Tomi Valkeinen wrote:

> On 20/09/13 10:06, Ryan Mallon wrote:
>> 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 <rmallon@gmail.com>
>> ---
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ryan Mallon Feb. 12, 2014, 5:02 a.m. UTC | #3
On 12/02/14 19:54, Tomi Valkeinen wrote:

> On 11/02/14 21:07, Ryan Mallon wrote:
>> On 12/02/14 03:06, Tomi Valkeinen wrote:
>>
>>> On 20/09/13 10:06, Ryan Mallon wrote:
>>>> 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 <rmallon@gmail.com>
>>>> ---
>>>
>>> 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.
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Feb. 12, 2014, 6:54 a.m. UTC | #4
On 11/02/14 21:07, Ryan Mallon wrote:
> On 12/02/14 03:06, Tomi Valkeinen wrote:
> 
>> On 20/09/13 10:06, Ryan Mallon wrote:
>>> 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 <rmallon@gmail.com>
>>> ---
>>
>> 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.

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.

> 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
Tomi Valkeinen Feb. 12, 2014, 8:17 a.m. UTC | #5
On 12/02/14 07:02, Ryan Mallon wrote:

> 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
diff mbox

Patch

diff --git a/drivers/video/auo_k190x.c b/drivers/video/auo_k190x.c
index 8d2499d..0eeeb26 100644
--- a/drivers/video/auo_k190x.c
+++ b/drivers/video/auo_k190x.c
@@ -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;
diff --git a/drivers/video/broadsheetfb.c b/drivers/video/broadsheetfb.c
index b09701c..4d6231a 100644
--- a/drivers/video/broadsheetfb.c
+++ b/drivers/video/broadsheetfb.c
@@ -924,6 +924,14 @@  static void broadsheetfb_dpy_update(struct broadsheetfb_par *par)
 	mutex_unlock(&(par->io_lock));
 }
 
+static int broadsheetfb_sync(struct fb_info *info)
+{
+	struct broadsheetfb_par *par = info->par;
+
+	broadsheetfb_dpy_update(par);
+	return 0;
+}
+
 /* this is called back from the deferred io workqueue */
 static void broadsheetfb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
@@ -998,56 +1006,11 @@  static void broadsheetfb_imageblit(struct fb_info *info,
 	broadsheetfb_dpy_update(par);
 }
 
-/*
- * 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 broadsheetfb_write(struct fb_info *info, const char __user *buf,
-				size_t count, loff_t *ppos)
-{
-	struct broadsheetfb_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;
-
-	broadsheetfb_dpy_update(par);
-
-	return (err) ? err : count;
-}
-
 static struct fb_ops broadsheetfb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_read        = fb_sys_read,
-	.fb_write	= broadsheetfb_write,
+	.fb_write	= fb_sys_write,
+	.fb_sync	= broadsheetfb_sync,
 	.fb_fillrect	= broadsheetfb_fillrect,
 	.fb_copyarea	= broadsheetfb_copyarea,
 	.fb_imageblit	= broadsheetfb_imageblit,
@@ -1106,6 +1069,7 @@  static int broadsheetfb_probe(struct platform_device *dev)
 		goto err_fb_rel;
 
 	info->screen_base = (char *)videomemory;
+	info->screen_size = videomemorysize;
 	info->fbops = &broadsheetfb_ops;
 
 	broadsheetfb_var.xres = dpyw;
diff --git a/drivers/video/hecubafb.c b/drivers/video/hecubafb.c
index 59d2318..e8361e1 100644
--- a/drivers/video/hecubafb.c
+++ b/drivers/video/hecubafb.c
@@ -114,6 +114,14 @@  static void hecubafb_dpy_update(struct hecubafb_par *par)
 	apollo_send_command(par, APOLLO_DISPLAY_IMG);
 }
 
+static int hecubafb_sync(struct fb_info *info)
+{
+	struct hecubafb_par *par = info->par;
+
+	hecubafb_dpy_update(par);
+	return 0;
+}
+
 /* this is called back from the deferred io workqueue */
 static void hecubafb_dpy_deferred_io(struct fb_info *info,
 				struct list_head *pagelist)
@@ -151,56 +159,11 @@  static void hecubafb_imageblit(struct fb_info *info,
 	hecubafb_dpy_update(par);
 }
 
-/*
- * 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 hecubafb_write(struct fb_info *info, const char __user *buf,
-				size_t count, loff_t *ppos)
-{
-	struct hecubafb_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 __force *) (info->screen_base + p);
-
-	if (copy_from_user(dst, buf, count))
-		err = -EFAULT;
-
-	if  (!err)
-		*ppos += count;
-
-	hecubafb_dpy_update(par);
-
-	return (err) ? err : count;
-}
-
 static struct fb_ops hecubafb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_read        = fb_sys_read,
-	.fb_write	= hecubafb_write,
+	.fb_write	= fb_sys_write,
+	.fb_sync	= hecubafb_sync,
 	.fb_fillrect	= hecubafb_fillrect,
 	.fb_copyarea	= hecubafb_copyarea,
 	.fb_imageblit	= hecubafb_imageblit,
@@ -240,6 +203,7 @@  static int hecubafb_probe(struct platform_device *dev)
 		goto err_fballoc;
 
 	info->screen_base = (char __force __iomem *)videomemory;
+	info->screen_size = videomemorysize;
 	info->fbops = &hecubafb_ops;
 
 	info->var = hecubafb_var;
diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
index f30150d..7f79916 100644
--- a/drivers/video/metronomefb.c
+++ b/drivers/video/metronomefb.c
@@ -447,6 +447,14 @@  static void metronomefb_dpy_update(struct metronomefb_par *par)
 	metronome_display_cmd(par);
 }
 
+static int metronomefb_sync(struct fb_info *info)
+{
+	struct metronomefb_par *par = info->par;
+
+	metronomefb_dpy_udpate(par);
+	return 0;
+}
+
 static u16 metronomefb_dpy_update_page(struct metronomefb_par *par, int index)
 {
 	int i;
@@ -510,55 +518,10 @@  static void metronomefb_imageblit(struct fb_info *info,
 	metronomefb_dpy_update(par);
 }
 
-/*
- * this is the slow path from userspace. they can seek and write to
- * the fb. it is based on fb_sys_write
- */
-static ssize_t metronomefb_write(struct fb_info *info, const char __user *buf,
-				size_t count, loff_t *ppos)
-{
-	struct metronomefb_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 __force *)(info->screen_base + p);
-
-	if (copy_from_user(dst, buf, count))
-		err = -EFAULT;
-
-	if  (!err)
-		*ppos += count;
-
-	metronomefb_dpy_update(par);
-
-	return (err) ? err : count;
-}
-
 static struct fb_ops metronomefb_ops = {
 	.owner		= THIS_MODULE,
-	.fb_write	= metronomefb_write,
+	.fb_write	= fb_sys_write,
+	.fb_sync	= metronomefb_sync,
 	.fb_fillrect	= metronomefb_fillrect,
 	.fb_copyarea	= metronomefb_copyarea,
 	.fb_imageblit	= metronomefb_imageblit,
@@ -633,6 +596,7 @@  static int metronomefb_probe(struct platform_device *dev)
 		goto err_fb_rel;
 
 	info->screen_base = (char __force __iomem *)videomemory;
+	info->screen_size = videomemorysize;
 	info->fbops = &metronomefb_ops;
 
 	metronomefb_fix.line_length = fw;
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 44967c8..2b46790 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -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;