[PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Ambrose, Martin]

 This work includes the following:
 . Implement handler for FBIO_WAITFORVSYNC ioctl.

 . Allocate the data and palette buffers separately.
   A consequence of this is that the palette and data loading is now
   done in different phases. And that the LCD must be disabled
   temporarily after the palette is loaded but this will only happen
   once after init and each time the palette is changed. I think this
   is OK.

 . Allocate two (ping and pong) framebuffers from memory.

 . Add pan_display handler which toggles the LCDC DMA registers between
   the ping and pong buffers.

Signed-off-by: Martin Ambrose <martin@ti.com>
---
 drivers/video/da8xx-fb.c |  296 ++++++++++++++++++++++++++++++++++++----------
 include/video/da8xx-fb.h |    1 +
 2 files changed, 234 insertions(+), 63 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 369a5b3..e4ddcfe 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -35,7 +35,9 @@
 #define DRIVER_NAME "da8xx_lcdc"

 /* LCD Status Register */
+#define LCD_END_OF_FRAME1              BIT(9)
 #define LCD_END_OF_FRAME0              BIT(8)
+#define LCD_PL_LOAD_DONE               BIT(6)
 #define LCD_FIFO_UNDERFLOW             BIT(5)
 #define LCD_SYNC_LOST                  BIT(2)

@@ -57,11 +59,13 @@
 #define LCD_PALETTE_LOAD_MODE(x)       ((x) << 20)
 #define PALETTE_AND_DATA               0x00
 #define PALETTE_ONLY                   0x01
+#define DATA_ONLY                      0x02

 #define LCD_MONO_8BIT_MODE             BIT(9)
 #define LCD_RASTER_ORDER               BIT(8)
 #define LCD_TFT_MODE                   BIT(7)
 #define LCD_UNDERFLOW_INT_ENA          BIT(6)
+#define LCD_PL_ENABLE                  BIT(4)
 #define LCD_MONOCHROME_MODE            BIT(1)
 #define LCD_RASTER_ENABLE              BIT(0)
 #define LCD_TFT_ALT_ENABLE             BIT(23)
@@ -86,6 +90,10 @@
 #define  LCD_DMA_CTRL_REG                      0x40
 #define  LCD_DMA_FRM_BUF_BASE_ADDR_0_REG       0x44
 #define  LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG    0x48
+#define  LCD_DMA_FRM_BUF_BASE_ADDR_1_REG       0x4C
+#define  LCD_DMA_FRM_BUF_CEILING_ADDR_1_REG    0x50
+
+#define LCD_NUM_BUFFERS        2

 #define WSI_TIMEOUT    50
 #define PALETTE_SIZE   256
@@ -110,13 +118,20 @@ static inline void lcdc_write(unsigned int val, unsigned int addr)
 struct da8xx_fb_par {
        resource_size_t p_palette_base;
        unsigned char *v_palette_base;
+       dma_addr_t              vram_phys;
+       unsigned long           vram_size;
+       void                    *vram_virt;
+       unsigned int            dma_start;
+       unsigned int            dma_end;
        struct clk *lcdc_clk;
        int irq;
        unsigned short pseudo_palette[16];
-       unsigned int databuf_sz;
        unsigned int palette_sz;
        unsigned int pxl_clk;
        int blank;
+       wait_queue_head_t       vsync_wait;
+       int                     vsync_flag;
+       int                     vsync_timeout;
 #ifdef CONFIG_CPU_FREQ
        struct notifier_block   freq_transition;
 #endif
@@ -147,9 +162,9 @@ static struct fb_fix_screeninfo da8xx_fb_fix __devinitdata = {
        .type = FB_TYPE_PACKED_PIXELS,
        .type_aux = 0,
        .visual = FB_VISUAL_PSEUDOCOLOR,
-       .xpanstep = 1,
+       .xpanstep = 0,
        .ypanstep = 1,
-       .ywrapstep = 1,
+       .ywrapstep = 0,
        .accel = FB_ACCEL_NONE
 };

@@ -220,22 +235,48 @@ static inline void lcd_disable_raster(void)

 static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
 {
-       u32 tmp = par->p_palette_base + par->databuf_sz - 4;
-       u32 reg;
+       u32 start;
+       u32 end;
+       u32 reg_ras;
+       u32 reg_dma;
+
+       /* init reg to clear PLM (loading mode) fields */
+       reg_ras = lcdc_read(LCD_RASTER_CTRL_REG);
+       reg_ras &= ~(3 << 20);
+
+       reg_dma  = lcdc_read(LCD_DMA_CTRL_REG);
+
+       if (load_mode = LOAD_DATA) {
+               start    = par->dma_start;
+               end      = par->dma_end;
+
+               reg_ras |= LCD_PALETTE_LOAD_MODE(DATA_ONLY);
+               reg_dma |= LCD_END_OF_FRAME_INT_ENA;
+               reg_dma |= LCD_DUAL_FRAME_BUFFER_ENABLE;
+
+               lcdc_write(start, LCD_DMA_FRM_BUF_BASE_ADDR_0_REG);
+               lcdc_write(end, LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG);
+               lcdc_write(start, LCD_DMA_FRM_BUF_BASE_ADDR_1_REG);
+               lcdc_write(end, LCD_DMA_FRM_BUF_CEILING_ADDR_1_REG);
+       } else if (load_mode = LOAD_PALETTE) {
+               start    = par->p_palette_base;
+               end      = start + par->palette_sz - 1;
+
+               reg_ras |= LCD_PALETTE_LOAD_MODE(PALETTE_ONLY);
+               reg_ras |= LCD_PL_ENABLE;
+
+               lcdc_write(start, LCD_DMA_FRM_BUF_BASE_ADDR_0_REG);
+               lcdc_write(end, LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG);
+       }

-       /* Update the databuf in the hw. */
-       lcdc_write(par->p_palette_base, LCD_DMA_FRM_BUF_BASE_ADDR_0_REG);
-       lcdc_write(tmp, LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG);
+       lcdc_write(reg_dma, LCD_DMA_CTRL_REG);
+       lcdc_write(reg_ras, LCD_RASTER_CTRL_REG);

-       /* Start the DMA. */
-       reg = lcdc_read(LCD_RASTER_CTRL_REG);
-       reg &= ~(3 << 20);
-       if (load_mode = LOAD_DATA)
-               reg |= LCD_PALETTE_LOAD_MODE(PALETTE_AND_DATA);
-       else if (load_mode = LOAD_PALETTE)
-               reg |= LCD_PALETTE_LOAD_MODE(PALETTE_ONLY);
-
-       lcdc_write(reg, LCD_RASTER_CTRL_REG);
+       /*
+        * The Raster enable bit must be set after all other control fields are
+        * set.
+        */
+       lcd_enable_raster();
 }

 /* Configure the Burst Size of DMA */
@@ -367,12 +408,8 @@ static int lcd_cfg_display(const struct lcd_ctrl_config *cfg)
 static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
                u32 bpp, u32 raster_order)
 {
-       u32 bpl, reg;
+       u32 reg;

-       /* Disable Dual Frame Buffer. */
-       reg = lcdc_read(LCD_DMA_CTRL_REG);
-       lcdc_write(reg & ~LCD_DUAL_FRAME_BUFFER_ENABLE,
-                                               LCD_DMA_CTRL_REG);
        /* Set the Panel Width */
        /* Pixels per line = (PPL + 1)*16 */
        /*0x3F in bits 4..9 gives max horisontal resolution = 1024 pixels*/
@@ -409,9 +446,6 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
                return -EINVAL;
        }

-       bpl = width * bpp / 8;
-       par->databuf_sz = height * bpl + par->palette_sz;
-
        return 0;
 }

@@ -420,8 +454,9 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
                              struct fb_info *info)
 {
        struct da8xx_fb_par *par = info->par;
-       unsigned short *palette = (unsigned short *)par->v_palette_base;
+       unsigned short *palette = (unsigned short *) par->v_palette_base;
        u_short pal;
+       int update_hw = 0;

        if (regno > 255)
                return 1;
@@ -438,8 +473,10 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
                pal |= (green & 0x00f0);
                pal |= (blue & 0x000f);

-               palette[regno] = pal;
-
+               if (palette[regno] != pal) {
+                       update_hw = 1;
+                       palette[regno] = pal;
+               }
        } else if ((info->var.bits_per_pixel = 16) && regno < 16) {
                red >>= (16 - info->var.red.length);
                red <<= info->var.red.offset;
@@ -452,9 +489,16 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,

                par->pseudo_palette[regno] = red | green | blue;

-               palette[0] = 0x4000;
+               if (palette[0] != 0x4000) {
+                       update_hw = 1;
+                       palette[0] = 0x4000;
+               }
        }

+       /* Update the palette in the h/w as needed. */
+       if (update_hw)
+               lcd_blit(LOAD_PALETTE, par);
+
        return 0;
 }

@@ -540,15 +584,51 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,

 static irqreturn_t lcdc_irq_handler(int irq, void *arg)
 {
+       struct da8xx_fb_par *par = arg;
        u32 stat = lcdc_read(LCD_STAT_REG);
+       u32 reg_ras;

        if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
                lcd_disable_raster();
                lcdc_write(stat, LCD_STAT_REG);
                lcd_enable_raster();
-       } else
+       } else if (stat & LCD_PL_LOAD_DONE) {
                lcdc_write(stat, LCD_STAT_REG);

+               /*
+                * Must disable raster before changing state of any control bit.
+                */
+               lcd_disable_raster();
+
+               /* Disable PL completion inerrupt */
+               reg_ras  = lcdc_read(LCD_RASTER_CTRL_REG);
+               reg_ras &= ~LCD_PL_ENABLE;
+               lcdc_write(reg_ras, LCD_RASTER_CTRL_REG);
+
+               /* Setup and start data loading mode */
+               lcd_blit(LOAD_DATA, par);
+       } else {
+               lcdc_write(stat, LCD_STAT_REG);
+
+               if (stat & LCD_END_OF_FRAME0) {
+                       lcdc_write(par->dma_start,
+                                  LCD_DMA_FRM_BUF_BASE_ADDR_0_REG);
+                       lcdc_write(par->dma_end,
+                                  LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG);
+                       par->vsync_flag = 1;
+                       wake_up_interruptible(&par->vsync_wait);
+               }
+
+               if (stat & LCD_END_OF_FRAME1) {
+                       lcdc_write(par->dma_start,
+                                  LCD_DMA_FRM_BUF_BASE_ADDR_1_REG);
+                       lcdc_write(par->dma_end,
+                                  LCD_DMA_FRM_BUF_CEILING_ADDR_1_REG);
+                       par->vsync_flag = 1;
+                       wake_up_interruptible(&par->vsync_wait);
+               }
+       }
+
        return IRQ_HANDLED;
 }

@@ -653,9 +733,10 @@ static int __devexit fb_remove(struct platform_device *dev)

                unregister_framebuffer(info);
                fb_dealloc_cmap(&info->cmap);
-               dma_free_coherent(NULL, par->databuf_sz + PAGE_SIZE,
-                                       info->screen_base - PAGE_SIZE,
-                                       info->fix.smem_start);
+               dma_free_coherent(NULL, PALETTE_SIZE, par->v_palette_base,
+                                 par->p_palette_base);
+               dma_free_coherent(NULL, par->vram_size, par->vram_virt,
+                                 par->vram_phys);
                free_irq(par->irq, par);
                clk_disable(par->lcdc_clk);
                clk_put(par->lcdc_clk);
@@ -667,6 +748,42 @@ static int __devexit fb_remove(struct platform_device *dev)
        return 0;
 }

+/*
+ * Function to wait for vertical sync which for this LCD peripheral
+ * translates into waiting for the current raster frame to complete.
+ */
+static int fb_wait_for_vsync(struct fb_info *info)
+{
+       struct da8xx_fb_par *par = info->par;
+       wait_queue_t wq;
+       int ret;
+
+       init_waitqueue_entry(&wq, current);
+
+       /*
+        * Set flag to 0 and wait for isr to set to 1. It would seem there is a
+        * race condition here where the ISR could have occured just before or
+        * just after this set. But since we are just coarsely waiting for
+        * a frame to complete then that's OK. i.e. if the frame completed
+        * just before this code executed then we have to wait another full
+        * frame time but there is no way to avoid such a situation. On the
+        * other hand if the frame completed just after then we don't need
+        * to wait long at all. Either way we are guaranteed to return to the
+        * user immediately after a frame completion which is all that is
+        * required.
+        */
+       par->vsync_flag = 0;
+       ret = wait_event_interruptible_timeout(par->vsync_wait,
+                                              par->vsync_flag != 0,
+                                              par->vsync_timeout);
+       if (ret < 0)
+               return ret;
+       if (ret = 0)
+               return -ETIMEDOUT;
+
+       return 0;
+}
+
 static int fb_ioctl(struct fb_info *info, unsigned int cmd,
                          unsigned long arg)
 {
@@ -696,6 +813,8 @@ static int fb_ioctl(struct fb_info *info, unsigned int cmd,
                                        sync_arg.pulse_width,
                                        sync_arg.front_porch);
                break;
+       case FBIO_WAITFORVSYNC:
+               return fb_wait_for_vsync(info);
        default:
                return -EINVAL;
        }
@@ -731,10 +850,47 @@ static int cfb_blank(int blank, struct fb_info *info)
        return ret;
 }

+/*
+ * Set new x,y offsets in the virtual display for the visible area and switch
+ * to the new mode.
+ */
+static int da8xx_pan_display(struct fb_var_screeninfo *var,
+                            struct fb_info *fbi)
+{
+       int ret = 0;
+       struct fb_var_screeninfo new_var;
+       struct da8xx_fb_par         *par = fbi->par;
+       struct fb_fix_screeninfo    *fix = &fbi->fix;
+       unsigned int end;
+       unsigned int start;
+
+       if (var->xoffset != fbi->var.xoffset ||
+                       var->yoffset != fbi->var.yoffset) {
+               memcpy(&new_var, &fbi->var, sizeof(new_var));
+               new_var.xoffset = var->xoffset;
+               new_var.yoffset = var->yoffset;
+               if (fb_check_var(&new_var, fbi))
+                       ret = -EINVAL;
+               else {
+                       memcpy(&fbi->var, &new_var, sizeof(new_var));
+
+                       start   = fix->smem_start +
+                               new_var.yoffset * fix->line_length +
+                               new_var.xoffset * var->bits_per_pixel / 8;
+                       end     = start + var->yres * fix->line_length - 1;
+                       par->dma_start  = start;
+                       par->dma_end    = end;
+               }
+       }
+
+       return ret;
+}
+
 static struct fb_ops da8xx_fb_ops = {
        .owner = THIS_MODULE,
        .fb_check_var = fb_check_var,
        .fb_setcolreg = fb_setcolreg,
+       .fb_pan_display = da8xx_pan_display,
        .fb_ioctl = fb_ioctl,
        .fb_fillrect = cfb_fillrect,
        .fb_copyarea = cfb_copyarea,
@@ -828,40 +984,53 @@ static int __init fb_probe(struct platform_device *device)
        }

        /* allocate frame buffer */
-       da8xx_fb_info->screen_base = dma_alloc_coherent(NULL,
-                                       par->databuf_sz + PAGE_SIZE,
-                                       (resource_size_t *)
-                                       &da8xx_fb_info->fix.smem_start,
-                                       GFP_KERNEL | GFP_DMA);
-
-       if (!da8xx_fb_info->screen_base) {
+       par->vram_size = lcdc_info->width * lcdc_info->height * lcd_cfg->bpp;
+       par->vram_size = PAGE_ALIGN(par->vram_size/8);
+       par->vram_size = par->vram_size * LCD_NUM_BUFFERS;
+
+       par->vram_virt = dma_alloc_coherent(NULL,
+                                           par->vram_size,
+                                           (resource_size_t *) &par->vram_phys,
+                                           GFP_KERNEL | GFP_DMA);
+       if (!par->vram_virt) {
                dev_err(&device->dev,
                        "GLCD: kmalloc for frame buffer failed\n");
                ret = -EINVAL;
                goto err_release_fb;
        }

-       /* move palette base pointer by (PAGE_SIZE - palette_sz) bytes */
-       par->v_palette_base = da8xx_fb_info->screen_base +
-                               (PAGE_SIZE - par->palette_sz);
-       par->p_palette_base = da8xx_fb_info->fix.smem_start +
-                               (PAGE_SIZE - par->palette_sz);
-
-       /* the rest of the frame buffer is pixel data */
-       da8xx_fb_info->screen_base = par->v_palette_base + par->palette_sz;
-       da8xx_fb_fix.smem_start = par->p_palette_base + par->palette_sz;
-       da8xx_fb_fix.smem_len = par->databuf_sz - par->palette_sz;
-       da8xx_fb_fix.line_length = (lcdc_info->width * lcd_cfg->bpp) / 8;
+       da8xx_fb_info->screen_base = (char __iomem *) par->vram_virt;
+       da8xx_fb_fix.smem_start    = par->vram_phys;
+       da8xx_fb_fix.smem_len      = par->vram_size;
+       da8xx_fb_fix.line_length   = (lcdc_info->width * lcd_cfg->bpp) / 8;
+
+       par->dma_start = par->vram_phys;
+       par->dma_end   = par->dma_start + lcdc_info->height *
+               da8xx_fb_fix.line_length - 1;
+
+       /* allocate palette buffer */
+       par->v_palette_base = dma_alloc_coherent(NULL,
+                                              PALETTE_SIZE,
+                                              (resource_size_t *)
+                                              &par->p_palette_base,
+                                              GFP_KERNEL | GFP_DMA);
+       if (!par->v_palette_base) {
+               dev_err(&device->dev,
+                       "GLCD: kmalloc for palette buffer failed\n");
+               ret = -EINVAL;
+               goto err_release_fb_mem;
+       }
+       memset(par->v_palette_base, 0, PALETTE_SIZE);

        par->irq = platform_get_irq(device, 0);
        if (par->irq < 0) {
                ret = -ENOENT;
-               goto err_release_fb_mem;
+               goto err_release_pl_mem;
        }

        ret = request_irq(par->irq, lcdc_irq_handler, 0, DRIVER_NAME, par);
        if (ret)
-               goto err_release_fb_mem;
+               goto err_release_pl_mem;

        /* Initialize par */
        da8xx_fb_info->var.bits_per_pixel = lcd_cfg->bpp;
@@ -869,8 +1038,8 @@ static int __init fb_probe(struct platform_device *device)
        da8xx_fb_var.xres = lcdc_info->width;
        da8xx_fb_var.xres_virtual = lcdc_info->width;

-       da8xx_fb_var.yres = lcdc_info->height;
-       da8xx_fb_var.yres_virtual = lcdc_info->height;
+       da8xx_fb_var.yres         = lcdc_info->height;
+       da8xx_fb_var.yres_virtual = lcdc_info->height * LCD_NUM_BUFFERS;

        da8xx_fb_var.grayscale             lcd_cfg->p_disp_panel->panel_shade = MONOCHROME ? 1 : 0;
@@ -891,13 +1060,8 @@ static int __init fb_probe(struct platform_device *device)
        ret = fb_alloc_cmap(&da8xx_fb_info->cmap, PALETTE_SIZE, 0);
        if (ret)
                goto err_free_irq;
-
-       /* First palette_sz byte of the frame buffer is the palette */
        da8xx_fb_info->cmap.len = par->palette_sz;

-       /* Flush the buffer to the screen. */
-       lcd_blit(LOAD_DATA, par);
-
        /* initialize var_screeninfo */
        da8xx_fb_var.activate = FB_ACTIVATE_FORCE;
        fb_set_var(da8xx_fb_info, &da8xx_fb_var);
@@ -919,6 +1083,10 @@ static int __init fb_probe(struct platform_device *device)
        }
 #endif

+       /* initialize the vsync wait queue */
+       init_waitqueue_head(&par->vsync_wait);
+       par->vsync_timeout = HZ / 5;
+
        /* enable raster engine */
        lcd_enable_raster();

@@ -935,10 +1103,12 @@ err_dealloc_cmap:
 err_free_irq:
        free_irq(par->irq, par);

+err_release_pl_mem:
+       dma_free_coherent(NULL, PALETTE_SIZE, par->v_palette_base,
+                         par->p_palette_base);
+
 err_release_fb_mem:
-       dma_free_coherent(NULL, par->databuf_sz + PAGE_SIZE,
-                               da8xx_fb_info->screen_base - PAGE_SIZE,
-                               da8xx_fb_info->fix.smem_start);
+       dma_free_coherent(NULL, par->vram_size, par->vram_virt, par->vram_phys);

 err_release_fb:
        framebuffer_release(da8xx_fb_info);
diff --git a/include/video/da8xx-fb.h b/include/video/da8xx-fb.h
index 89d43b3..6316cda 100644
--- a/include/video/da8xx-fb.h
+++ b/include/video/da8xx-fb.h
@@ -99,6 +99,7 @@ struct lcd_sync_arg {
 #define FBIPUT_COLOR           _IOW('F', 6, int)
 #define FBIPUT_HSYNC           _IOW('F', 9, int)
 #define FBIPUT_VSYNC           _IOW('F', 10, int)
+#define FBIO_WAITFORVSYNC      _IOW('F', 0x20, u_int32_t)

 #endif  /* ifndef DA8XX_FB_H */

--
1.6.6.1

Re: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Andrew Morton]

On Mon, 29 Mar 2010 08:16:51 -0500
"Ambrose, Martin" <martin@ti.com> wrote:

> +/*
> + * Function to wait for vertical sync which for this LCD peripheral
> + * translates into waiting for the current raster frame to complete.
> + */
> +static int fb_wait_for_vsync(struct fb_info *info)
> +{
> +       struct da8xx_fb_par *par = info->par;
> +       wait_queue_t wq;
> +       int ret;
> +
> +       init_waitqueue_entry(&wq, current);

DECLARE_WAITQUEUE() would be more conventional.

> +       /*
> +        * Set flag to 0 and wait for isr to set to 1. It would seem there is a
> +        * race condition here where the ISR could have occured just before or
> +        * just after this set. But since we are just coarsely waiting for
> +        * a frame to complete then that's OK. i.e. if the frame completed
> +        * just before this code executed then we have to wait another full
> +        * frame time but there is no way to avoid such a situation. On the
> +        * other hand if the frame completed just after then we don't need
> +        * to wait long at all. Either way we are guaranteed to return to the
> +        * user immediately after a frame completion which is all that is
> +        * required.
> +        */
> +       par->vsync_flag = 0;
> +       ret = wait_event_interruptible_timeout(par->vsync_wait,
> +                                              par->vsync_flag != 0,
> +                                              par->vsync_timeout);

If the calling process has signal_pending() (say, someone hit ^C) then
wait_event_interruptible_timeout() will fall straight through with
-ERESTARTSYS.  Will this cause the driver to malfunction at all?


> +       if (ret < 0)
> +               return ret;
> +       if (ret = 0)
> +               return -ETIMEDOUT;
> +
> +       return 0;
> +}

Re: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Andrew Morton]

On Wed, 31 Mar 2010 20:43:29 -0500 "Ambrose, Martin" <martin@ti.com> wrote:

> > > +       /*
> > > +        * Set flag to 0 and wait for isr to set to 1. It would seem there is a
> > > +        * race condition here where the ISR could have occured just before or
> > > +        * just after this set. But since we are just coarsely waiting for
> > > +        * a frame to complete then that's OK. i.e. if the frame completed
> > > +        * just before this code executed then we have to wait another full
> > > +        * frame time but there is no way to avoid such a situation. On the
> > > +        * other hand if the frame completed just after then we don't need
> > > +        * to wait long at all. Either way we are guaranteed to return to the
> > > +        * user immediately after a frame completion which is all that is
> > > +        * required.
> > > +        */
> > > +       par->vsync_flag = 0;
> > > +       ret = wait_event_interruptible_timeout(par->vsync_wait,
> > > +                                              par->vsync_flag != 0,
> > > +                                              par->vsync_timeout);
> > 
> > If the calling process has signal_pending() (say, someone hit ^C) then
> > wait_event_interruptible_timeout() will fall straight through with
> > -ERESTARTSYS.  Will this cause the driver to malfunction at all?
> 
> I don't think so since the driver doesn't make use of this
> information in any way. This is just status to the caller that the
> current frame has finished DMA'ing out of the framebuffer. 
> 
> Could you maybe propose a scenario/use case where you think it is
> problematic? I could then either reason why it should be OK or 
> I'll create a test harness and see how the driver can/should be modified.
> 

Gee, I dunno - I don't understand the driver to that level.  If you're
OK with this wait being interrupted by a signal and the driver handles
it OK then fine, that's a feature.

To test it I suppose you should give your test app a signal handler and
blast kills at it from another process.

RE: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Ambrose, Martin]

Andrew,

Thanks for the feedback.

> > +/*
> > + * Function to wait for vertical sync which for this LCD peripheral
> > + * translates into waiting for the current raster frame to complete.
> > + */
> > +static int fb_wait_for_vsync(struct fb_info *info)
> > +{
> > +       struct da8xx_fb_par *par = info->par;
> > +       wait_queue_t wq;
> > +       int ret;
> > +
> > +       init_waitqueue_entry(&wq, current);
> 
> DECLARE_WAITQUEUE() would be more conventional.

I'll update -- cloned this from some older code I think.
 
> > +       /*
> > +        * Set flag to 0 and wait for isr to set to 1. It would seem there is a
> > +        * race condition here where the ISR could have occured just before or
> > +        * just after this set. But since we are just coarsely waiting for
> > +        * a frame to complete then that's OK. i.e. if the frame completed
> > +        * just before this code executed then we have to wait another full
> > +        * frame time but there is no way to avoid such a situation. On the
> > +        * other hand if the frame completed just after then we don't need
> > +        * to wait long at all. Either way we are guaranteed to return to the
> > +        * user immediately after a frame completion which is all that is
> > +        * required.
> > +        */
> > +       par->vsync_flag = 0;
> > +       ret = wait_event_interruptible_timeout(par->vsync_wait,
> > +                                              par->vsync_flag != 0,
> > +                                              par->vsync_timeout);
> 
> If the calling process has signal_pending() (say, someone hit ^C) then
> wait_event_interruptible_timeout() will fall straight through with
> -ERESTARTSYS.  Will this cause the driver to malfunction at all?

I don't think so since the driver doesn't make use of this
information in any way. This is just status to the caller that the
current frame has finished DMA'ing out of the framebuffer. 

Could you maybe propose a scenario/use case where you think it is
problematic? I could then either reason why it should be OK or 
I'll create a test harness and see how the driver can/should be modified.

Regards,
Martin

RE: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Jon Povey]

Andrew Morton wrote:
> On Wed, 31 Mar 2010 20:43:29 -0500 "Ambrose, Martin" <martin@ti.com>
> wrote:

>>> If the calling process has signal_pending() (say, someone hit ^C)
>>> then wait_event_interruptible_timeout() will fall straight through
>>> with -ERESTARTSYS.  Will this cause the driver to malfunction at
>>> all?
>>
>> I don't think so since the driver doesn't make use of this
>> information in any way. This is just status to the caller that the
>> current frame has finished DMA'ing out of the framebuffer.
>>
>> Could you maybe propose a scenario/use case where you think it is
>> problematic? I could then either reason why it should be OK or
>> I'll create a test harness and see how the driver can/should be
>> modified.
>
> Gee, I dunno - I don't understand the driver to that level.  If you're
> OK with this wait being interrupted by a signal and the driver handles
> it OK then fine, that's a feature.
>
> To test it I suppose you should give your test app a signal handler
> and blast kills at it from another process.

Jumping in..

This should not cause a problem for the driver; its purpose is to tell userland that it can write to the buffer without corrupting graphics (as presumably it is double-buffering and the other buffer is now being DMA'd from by the hardware).

At worst, if the userland software doesn't handle this correctly it may draw one bad frame of graphics. Although if it's had a ctrl-C it probably has bigger things to worry about.

If the app wants to handle signals it needs to consider such things.. I would not expect the driver to do anything other than what this patch does.

--
Jon Povey
jon.povey@racelogic.co.uk

Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .

The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network

RE: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Ambrose, Martin]

On Wed, Mar 31, 2010 at 20:43:29, Ambrose, Martin wrote:

> Andrew,
> 
> Thanks for the feedback.
> 
> > > +/*
> > > + * Function to wait for vertical sync which for this LCD peripheral
> > > + * translates into waiting for the current raster frame to complete.
> > > + */
> > > +static int fb_wait_for_vsync(struct fb_info *info)
> > > +{
> > > +       struct da8xx_fb_par *par = info->par;
> > > +       wait_queue_t wq;
> > > +       int ret;
> > > +
> > > +       init_waitqueue_entry(&wq, current);
> > 
> > DECLARE_WAITQUEUE() would be more conventional.
> 
> I'll update -- cloned this from some older code I think.

After looking at this further this variable wasn't even used so
I will trim this in upcoming v3 patch.

Regards, 
Martin

RE: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering

[Ambrose, Martin]

On Thu, Apr 01, 2010 at 01:47:40, Jon Povey wrote:

> Andrew Morton wrote:
> > On Wed, 31 Mar 2010 20:43:29 -0500 "Ambrose, Martin" <martin@ti.com>
> > wrote:
> 
> >>> If the calling process has signal_pending() (say, someone hit ^C)
> >>> then wait_event_interruptible_timeout() will fall straight through
> >>> with -ERESTARTSYS.  Will this cause the driver to malfunction at
> >>> all?
> >>
> >> I don't think so since the driver doesn't make use of this
> >> information in any way. This is just status to the caller that the
> >> current frame has finished DMA'ing out of the framebuffer.
> >>
> >> Could you maybe propose a scenario/use case where you think it is
> >> problematic? I could then either reason why it should be OK or
> >> I'll create a test harness and see how the driver can/should be
> >> modified.
> >
> > Gee, I dunno - I don't understand the driver to that level.  If you're
> > OK with this wait being interrupted by a signal and the driver handles
> > it OK then fine, that's a feature.
> >
> > To test it I suppose you should give your test app a signal handler
> > and blast kills at it from another process.
> 
> Jumping in..
> 
> This should not cause a problem for the driver; its purpose is to tell userland that it can
> write to the buffer without corrupting graphics (as presumably it is double-buffering and
> the other buffer is now being DMA'd from by the hardware).
> 
> At worst, if the userland software doesn't handle this correctly it may draw one bad frame of graphics. Although if it's had a ctrl-C it probably has bigger things to worry about.
> 
> If the app wants to handle signals it needs to consider such things.. I would not expect the driver to do anything other than what this patch does.

Thanks Jon. I agree. 

If no further objections I'll leave this as is.

Regards, 
Martin