| Message ID | 20210714145804.2530727-5-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | video: fbdev: ssd1307fb: Optimizations and improvements | expand |
Hi Geert, On Wed, Jul 14, 2021 at 04:58:03PM +0200, Geert Uytterhoeven wrote: > Currently, each screen update triggers an I2C transfer of all screen > data, up to 1 KiB of data for a 128x64 display, which takes at least 20 > ms in Fast mode. > > Reduce the amount of transferred data by only updating the rectangle > that changed. Remove the call to ssd1307fb_set_address_range() during > initialization, as ssd1307fb_update_rect() now takes care of that. > > Note that for now the optimized operation is only used for fillrect, > copyarea, and imageblit, which are used by fbcon. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > drivers/video/fbdev/ssd1307fb.c | 43 ++++++++++++++++++++------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c > index cfa27ea0feab4f01..8e3d4be74723b9bf 100644 > --- a/drivers/video/fbdev/ssd1307fb.c > +++ b/drivers/video/fbdev/ssd1307fb.c > @@ -184,16 +184,18 @@ static int ssd1307fb_set_address_range(struct ssd1307fb_par *par, u8 col_start, > return ssd1307fb_write_cmd(par->client, page_end); > } > > -static int ssd1307fb_update_display(struct ssd1307fb_par *par) > +static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x, > + unsigned int y, unsigned int width, > + unsigned int height) > { > struct ssd1307fb_array *array; > u8 *vmem = par->info->screen_buffer; > unsigned int line_length = par->info->fix.line_length; > - unsigned int pages = DIV_ROUND_UP(par->height, 8); > + unsigned int pages = DIV_ROUND_UP(height + y % 8, 8); Add () like this - at least it helps me: > + unsigned int pages = DIV_ROUND_UP((height + y) % 8, 8); > u32 array_idx = 0; > int ret, i, j, k; > > - array = ssd1307fb_alloc_array(par->width * pages, SSD1307FB_DATA); > + array = ssd1307fb_alloc_array(width * pages, SSD1307FB_DATA); > if (!array) > return -ENOMEM; > > @@ -226,13 +228,18 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par) > * (5) A4 B4 C4 D4 E4 F4 G4 H4 > */ > > - for (i = 0; i < pages; i++) { > + ret = ssd1307fb_set_address_range(par, par->col_offset + x, width, > + par->page_offset + y / 8, pages); > + if (ret < 0) > + goto out_free; > + > + for (i = y / 8; i < y / 8 + pages; i++) { > int m = 8; > > /* Last page may be partial */ > - if (i + 1 == pages && par->height % 8) > + if (8 * (i + 1) > par->height) > m = par->height % 8; As before, this looks wrong to me. Sam > - for (j = 0; j < par->width; j++) { > + for (j = x; j < x + width; j++) { > u8 data = 0; > > for (k = 0; k < m; k++) { > @@ -245,11 +252,17 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par) > } > } > > - ret = ssd1307fb_write_array(par->client, array, par->width * pages); > + ret = ssd1307fb_write_array(par->client, array, width * pages); > + > +out_free: > kfree(array); > return ret; > } > > +static int ssd1307fb_update_display(struct ssd1307fb_par *par) > +{ > + return ssd1307fb_update_rect(par, 0, 0, par->width, par->height); > +} > > static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf, > size_t count, loff_t *ppos) > @@ -299,21 +312,24 @@ static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *r > { > struct ssd1307fb_par *par = info->par; > sys_fillrect(info, rect); > - ssd1307fb_update_display(par); > + ssd1307fb_update_rect(par, rect->dx, rect->dy, rect->width, > + rect->height); > } > > static void ssd1307fb_copyarea(struct fb_info *info, const struct fb_copyarea *area) > { > struct ssd1307fb_par *par = info->par; > sys_copyarea(info, area); > - ssd1307fb_update_display(par); > + ssd1307fb_update_rect(par, area->dx, area->dy, area->width, > + area->height); > } > > static void ssd1307fb_imageblit(struct fb_info *info, const struct fb_image *image) > { > struct ssd1307fb_par *par = info->par; > sys_imageblit(info, image); > - ssd1307fb_update_display(par); > + ssd1307fb_update_rect(par, image->dx, image->dy, image->width, > + image->height); > } > > static const struct fb_ops ssd1307fb_ops = { > @@ -493,13 +509,6 @@ static int ssd1307fb_init(struct ssd1307fb_par *par) > if (ret < 0) > return ret; > > - /* Set column and page range */ > - ret = ssd1307fb_set_address_range(par, par->col_offset, par->width, > - par->page_offset, > - DIV_ROUND_UP(par->height, 8)); > - if (ret < 0) > - return ret; > - > /* Clear the screen */ > ret = ssd1307fb_update_display(par); > if (ret < 0) > -- > 2.25.1
Hi Sam, On Mon, Jul 19, 2021 at 9:21 PM Sam Ravnborg <sam@ravnborg.org> wrote: > On Wed, Jul 14, 2021 at 04:58:03PM +0200, Geert Uytterhoeven wrote: > > Currently, each screen update triggers an I2C transfer of all screen > > data, up to 1 KiB of data for a 128x64 display, which takes at least 20 > > ms in Fast mode. > > > > Reduce the amount of transferred data by only updating the rectangle > > that changed. Remove the call to ssd1307fb_set_address_range() during > > initialization, as ssd1307fb_update_rect() now takes care of that. > > > > Note that for now the optimized operation is only used for fillrect, > > copyarea, and imageblit, which are used by fbcon. > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- a/drivers/video/fbdev/ssd1307fb.c > > +++ b/drivers/video/fbdev/ssd1307fb.c > > @@ -184,16 +184,18 @@ static int ssd1307fb_set_address_range(struct ssd1307fb_par *par, u8 col_start, > > return ssd1307fb_write_cmd(par->client, page_end); > > } > > > > -static int ssd1307fb_update_display(struct ssd1307fb_par *par) > > +static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x, > > + unsigned int y, unsigned int width, > > + unsigned int height) > > { > > struct ssd1307fb_array *array; > > u8 *vmem = par->info->screen_buffer; > > unsigned int line_length = par->info->fix.line_length; > > - unsigned int pages = DIV_ROUND_UP(par->height, 8); > > + unsigned int pages = DIV_ROUND_UP(height + y % 8, 8); > > Add () like this - at least it helps me: > > + unsigned int pages = DIV_ROUND_UP((height + y) % 8, 8); Thanks, that's actually a genuine bug. > > @@ -226,13 +228,18 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par) > > * (5) A4 B4 C4 D4 E4 F4 G4 H4 > > */ > > > > - for (i = 0; i < pages; i++) { > > + ret = ssd1307fb_set_address_range(par, par->col_offset + x, width, > > + par->page_offset + y / 8, pages); > > + if (ret < 0) > > + goto out_free; > > + > > + for (i = y / 8; i < y / 8 + pages; i++) { > > int m = 8; > > > > /* Last page may be partial */ > > - if (i + 1 == pages && par->height % 8) > > + if (8 * (i + 1) > par->height) > > m = par->height % 8; > As before, this looks wrong to me. Let's sort that out in the other thread... Gr{oetje,eeting}s, Geert
Hi Sam, On Tue, Jul 20, 2021 at 9:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jul 19, 2021 at 9:21 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > On Wed, Jul 14, 2021 at 04:58:03PM +0200, Geert Uytterhoeven wrote: > > > Currently, each screen update triggers an I2C transfer of all screen > > > data, up to 1 KiB of data for a 128x64 display, which takes at least 20 > > > ms in Fast mode. > > > > > > Reduce the amount of transferred data by only updating the rectangle > > > that changed. Remove the call to ssd1307fb_set_address_range() during > > > initialization, as ssd1307fb_update_rect() now takes care of that. > > > > > > Note that for now the optimized operation is only used for fillrect, > > > copyarea, and imageblit, which are used by fbcon. > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > --- a/drivers/video/fbdev/ssd1307fb.c > > > +++ b/drivers/video/fbdev/ssd1307fb.c > > > @@ -184,16 +184,18 @@ static int ssd1307fb_set_address_range(struct ssd1307fb_par *par, u8 col_start, > > > return ssd1307fb_write_cmd(par->client, page_end); > > > } > > > > > > -static int ssd1307fb_update_display(struct ssd1307fb_par *par) > > > +static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x, > > > + unsigned int y, unsigned int width, > > > + unsigned int height) > > > { > > > struct ssd1307fb_array *array; > > > u8 *vmem = par->info->screen_buffer; > > > unsigned int line_length = par->info->fix.line_length; > > > - unsigned int pages = DIV_ROUND_UP(par->height, 8); > > > + unsigned int pages = DIV_ROUND_UP(height + y % 8, 8); > > > > Add () like this - at least it helps me: > > > + unsigned int pages = DIV_ROUND_UP((height + y) % 8, 8); > > Thanks, that's actually a genuine bug. No it's not "(height + y) % 8" is wrong. Better if I reorder the operands like below? unsigned int pages = DIV_ROUND_UP(y % 8 + height, 8); Gr{oetje,eeting}s, Geert
On Tue, Jul 20, 2021 at 11:16:27AM +0200, Geert Uytterhoeven wrote: > Hi Sam, > > On Tue, Jul 20, 2021 at 9:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Jul 19, 2021 at 9:21 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > On Wed, Jul 14, 2021 at 04:58:03PM +0200, Geert Uytterhoeven wrote: > > > > Currently, each screen update triggers an I2C transfer of all screen > > > > data, up to 1 KiB of data for a 128x64 display, which takes at least 20 > > > > ms in Fast mode. > > > > > > > > Reduce the amount of transferred data by only updating the rectangle > > > > that changed. Remove the call to ssd1307fb_set_address_range() during > > > > initialization, as ssd1307fb_update_rect() now takes care of that. > > > > > > > > Note that for now the optimized operation is only used for fillrect, > > > > copyarea, and imageblit, which are used by fbcon. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > > --- a/drivers/video/fbdev/ssd1307fb.c > > > > +++ b/drivers/video/fbdev/ssd1307fb.c > > > > @@ -184,16 +184,18 @@ static int ssd1307fb_set_address_range(struct ssd1307fb_par *par, u8 col_start, > > > > return ssd1307fb_write_cmd(par->client, page_end); > > > > } > > > > > > > > -static int ssd1307fb_update_display(struct ssd1307fb_par *par) > > > > +static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x, > > > > + unsigned int y, unsigned int width, > > > > + unsigned int height) > > > > { > > > > struct ssd1307fb_array *array; > > > > u8 *vmem = par->info->screen_buffer; > > > > unsigned int line_length = par->info->fix.line_length; > > > > - unsigned int pages = DIV_ROUND_UP(par->height, 8); > > > > + unsigned int pages = DIV_ROUND_UP(height + y % 8, 8); > > > > > > Add () like this - at least it helps me: > > > > + unsigned int pages = DIV_ROUND_UP((height + y) % 8, 8); > > > > Thanks, that's actually a genuine bug. > > No it's not "(height + y) % 8" is wrong. > > Better if I reorder the operands like below? > > unsigned int pages = DIV_ROUND_UP(y % 8 + height, 8); Yep, then I would not be fooled so easy. Sam
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c index cfa27ea0feab4f01..8e3d4be74723b9bf 100644 --- a/drivers/video/fbdev/ssd1307fb.c +++ b/drivers/video/fbdev/ssd1307fb.c @@ -184,16 +184,18 @@ static int ssd1307fb_set_address_range(struct ssd1307fb_par *par, u8 col_start, return ssd1307fb_write_cmd(par->client, page_end); } -static int ssd1307fb_update_display(struct ssd1307fb_par *par) +static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x, + unsigned int y, unsigned int width, + unsigned int height) { struct ssd1307fb_array *array; u8 *vmem = par->info->screen_buffer; unsigned int line_length = par->info->fix.line_length; - unsigned int pages = DIV_ROUND_UP(par->height, 8); + unsigned int pages = DIV_ROUND_UP(height + y % 8, 8); u32 array_idx = 0; int ret, i, j, k; - array = ssd1307fb_alloc_array(par->width * pages, SSD1307FB_DATA); + array = ssd1307fb_alloc_array(width * pages, SSD1307FB_DATA); if (!array) return -ENOMEM; @@ -226,13 +228,18 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par) * (5) A4 B4 C4 D4 E4 F4 G4 H4 */ - for (i = 0; i < pages; i++) { + ret = ssd1307fb_set_address_range(par, par->col_offset + x, width, + par->page_offset + y / 8, pages); + if (ret < 0) + goto out_free; + + for (i = y / 8; i < y / 8 + pages; i++) { int m = 8; /* Last page may be partial */ - if (i + 1 == pages && par->height % 8) + if (8 * (i + 1) > par->height) m = par->height % 8; - for (j = 0; j < par->width; j++) { + for (j = x; j < x + width; j++) { u8 data = 0; for (k = 0; k < m; k++) { @@ -245,11 +252,17 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par) } } - ret = ssd1307fb_write_array(par->client, array, par->width * pages); + ret = ssd1307fb_write_array(par->client, array, width * pages); + +out_free: kfree(array); return ret; } +static int ssd1307fb_update_display(struct ssd1307fb_par *par) +{ + return ssd1307fb_update_rect(par, 0, 0, par->width, par->height); +} static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos) @@ -299,21 +312,24 @@ static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *r { struct ssd1307fb_par *par = info->par; sys_fillrect(info, rect); - ssd1307fb_update_display(par); + ssd1307fb_update_rect(par, rect->dx, rect->dy, rect->width, + rect->height); } static void ssd1307fb_copyarea(struct fb_info *info, const struct fb_copyarea *area) { struct ssd1307fb_par *par = info->par; sys_copyarea(info, area); - ssd1307fb_update_display(par); + ssd1307fb_update_rect(par, area->dx, area->dy, area->width, + area->height); } static void ssd1307fb_imageblit(struct fb_info *info, const struct fb_image *image) { struct ssd1307fb_par *par = info->par; sys_imageblit(info, image); - ssd1307fb_update_display(par); + ssd1307fb_update_rect(par, image->dx, image->dy, image->width, + image->height); } static const struct fb_ops ssd1307fb_ops = { @@ -493,13 +509,6 @@ static int ssd1307fb_init(struct ssd1307fb_par *par) if (ret < 0) return ret; - /* Set column and page range */ - ret = ssd1307fb_set_address_range(par, par->col_offset, par->width, - par->page_offset, - DIV_ROUND_UP(par->height, 8)); - if (ret < 0) - return ret; - /* Clear the screen */ ret = ssd1307fb_update_display(par); if (ret < 0)
Currently, each screen update triggers an I2C transfer of all screen data, up to 1 KiB of data for a 128x64 display, which takes at least 20 ms in Fast mode. Reduce the amount of transferred data by only updating the rectangle that changed. Remove the call to ssd1307fb_set_address_range() during initialization, as ssd1307fb_update_rect() now takes care of that. Note that for now the optimized operation is only used for fillrect, copyarea, and imageblit, which are used by fbcon. Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/video/fbdev/ssd1307fb.c | 43 ++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 17 deletions(-)