fbdev: add E-Ink Broadsheet controller support

drivers/video/broadsheetfb.c line 238

Quoted 5 lines · Jaya Kumar

drivers/video/broadsheetfb.c new file +5 -0

+static void broadsheetfb_dpy_update_pages(struct broadsheetfb_par *par,
+                                             u16 y1, u16 y2)
+{
+     u16 args[5];
+     unsigned char *buf = (unsigned char __force *)par->info->screen_base;

You shouldn't add __force inside drivers.

drivers/video/broadsheetfb.c line 461

Quoted 9 lines

drivers/video/broadsheetfb.c new file +8 -0

+     videomemorysize = (DPY_W*DPY_H);
+     videomemory = vmalloc(videomemorysize);
+     if (!videomemory)
+             goto err_fb_rel;
+
+     memset(videomemory, 0, videomemorysize);
+
+     info->screen_base = (char __force __iomem *)videomemory;

Ditto.

Also, using vmalloc for the backing store means this memory can't be mmap()'d into userspace, which is a very common thing for framebuffer using programs to do. If mmap() is to be denied, then you should provide a mmap implementation in the fb_ops structure.

Quoted 5 lines · On Sun, Dec 21, 2008 at 6:06 AM, <[email protected]> wrote:

Also, using vmalloc for the backing store means this memory can't be mmap()'d into userspace, which is a very common thing for framebuffer using programs to do. If mmap() is to be denied, then you should provide a mmap implementation in the fb_ops structure.

I'm not sure I understand. broadsheetfb relies on defio to handle the mmap and setup the fb_ops mmap handler. I'm using Xfbdev with this and it works fine.

This patch adds support for the E-Ink Broadsheet display controller

+++ b/drivers/video/broadsheetfb.c

drivers/video/broadsheetfb.c new file +40 -0

+/*
+ * 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 __force *) (info->screen_base + p);
+
+       if (copy_from_user(dst, buf, count))
+               err = -EFAULT;
+
+       if  (!err)
+               *ppos += count;

Hi Jaya,

Quoted 1 line · Jaya Kumar

This patch adds support for the E-Ink Broadsheet display controller

Looking good. While at the topic of deferred io I took the opportunity to ask a bit about that below.

I sort of expected to see fb_sys_write here. Same thing for metronomefb. I wonder why you chose a custom write callback instead of fb_sys_write. Maybe I need to do the same thing for sh_mobile_lcdcfb and if so then we could hopefully share that code between our drivers. =)

By using the xxx_dpy_update() function these drivers seem to force a full screen update in their write() path. I'm not sure why though, shouldn't the write() case should just let the fb_sync callback do the sync for you? The fb_sys_write() function calls fb_sync _before_ writing though, I wonder why. Doesn't make sense to me - _after_ copying data sounds like a better place to flush. I don't know the history of the code though, so there may be a good reason for this.

Also, this brings up to another interesting topic. While I was hacking on deferred io for the sh_mobile_lcdcfb driver I realized that I needed hooks to update the screen for the callbacks fb_fillarea, fb_copyarea and fb_imageblit. The metronomefb driver and this one both force a full screen update for each operation. In sh_mobile_lcdcfb I chose to just schedule the delayed work instead. Either way is fine of course, but this issue does not seem driver specific to me. So I wonder if it is worth breaking out these sys-functions and handle all that in fb_defio.c instead. But maybe we're just covering for the fact that the fb console isn't syncing as it should?

I'm still not sure if it's a sane idea to modify the fb console so it fsyncs. It's a separate issue I guess. Anyway, fsyncing data for each accelerated operation sounds a bit slow to me, especially if that triggers a full screen update. Think softcursor modifying a tiny part of the screen and we redraw the entire screen. Otoh, console may not be the biggest use case for deferred io. =)

I may hack up some shared some code later this week that triggers the delayed work for sys_xxx operations - same type of delayed flushing as my sh_mobile_lcdcfb driver. I plan on putting it in fb_defio.c and letting it call the sys operations. Then the deferred io drivers can install those callbacks into struct fb_ops directly. That way we can remove similar code in 3 drivers at least. I'll begin with sh_mobile_lcdcfb and we can do the other drivers one by one later on.

I'm not sure if you think it's a good idea. It's a bit different from the direct fsyncing like you do today.

By using the <censored>_dpy_update() function these drivers seem to force a

drivers/video/broadsheetfb.c line 58

Quoted 11 lines · Jaya Kumar

drivers/video/broadsheetfb.c new file +8 -0

 
+static struct fb_var_screeninfo broadsheetfb_var __devinitdata = {
+     .xres           = DPY_W,
+     .yres           = DPY_H,
+     .xres_virtual   = DPY_W,
+     .yres_virtual   = DPY_H,
+     .bits_per_pixel = 8,
+     .grayscale      = 1,
+     .nonstd         = 1,

Why do you set .nonstd?

#define FB_NONSTD_HAM 1 /* Hold-And-Modify (HAM) */