| Message ID | 20220315110707.628166-5-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix monochrome conversion for sdd130x | expand |
On 3/15/22 12:07, Geert Uytterhoeven wrote: > ssd130x_clear_screen() allocates a temporary buffer sized to hold one > byte per pixel, while it only needs to hold one bit per pixel. > > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one > byte per pixel for the whole frame buffer, while it only needs to hold > one bit per pixel for the part that is to be updated. > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already Just drm_fb_xrgb8888_to_mono() since you already fixed the name in patch 1/5. > calculated it anyway. > > Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- Indeed. I haven't noticed that got the calculation wrong until you pointed out. Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier, On Tue, Mar 15, 2022 at 1:32 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 3/15/22 12:07, Geert Uytterhoeven wrote: > > ssd130x_clear_screen() allocates a temporary buffer sized to hold one > > byte per pixel, while it only needs to hold one bit per pixel. > > > > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one > > byte per pixel for the whole frame buffer, while it only needs to hold > > one bit per pixel for the part that is to be updated. > > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already > > Just drm_fb_xrgb8888_to_mono() since you already fixed the name in patch 1/5. Bummer, that happens when reshuffling patches :-( Fixed for v2. > > calculated it anyway. > > > > Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > Indeed. I haven't noticed that got the calculation wrong until you pointed out. > > Acked-by: Javier Martinez Canillas <javierm@redhat.com> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Mar 15, 2022 at 12:07:06PM +0100, Geert Uytterhoeven wrote: > ssd130x_clear_screen() allocates a temporary buffer sized to hold one > byte per pixel, while it only needs to hold one bit per pixel. > > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one > byte per pixel for the whole frame buffer, while it only needs to hold > one bit per pixel for the part that is to be updated. > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already > calculated it anyway. Can we use bitmap API? bitmap_zalloc() / etc ?
Hi Andy, On Tue, Mar 15, 2022 at 2:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Mar 15, 2022 at 12:07:06PM +0100, Geert Uytterhoeven wrote: > > ssd130x_clear_screen() allocates a temporary buffer sized to hold one > > byte per pixel, while it only needs to hold one bit per pixel. > > > > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one > > byte per pixel for the whole frame buffer, while it only needs to hold > > one bit per pixel for the part that is to be updated. > > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already > > calculated it anyway. > > Can we use bitmap API? bitmap_zalloc() / etc ? Why? There is no need to operate on an array of longs, only on an array of bytes. Going to longs would expose us to endianness. There's also no need to use any of the bitmap operations to modify the data. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 7c99af4ce9dd4e5c..38b6c2c14f53644b 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -440,7 +440,8 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) .y2 = ssd130x->height, }; - buf = kcalloc(ssd130x->width, ssd130x->height, GFP_KERNEL); + buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height, + GFP_KERNEL); if (!buf) return; @@ -454,6 +455,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m { struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */ + unsigned int dst_pitch; int ret = 0; u8 *buf = NULL; @@ -461,11 +463,12 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m rect->y1 = round_down(rect->y1, 8); rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height); - buf = kcalloc(fb->width, fb->height, GFP_KERNEL); + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL); if (!buf) return -ENOMEM; - drm_fb_xrgb8888_to_mono(buf, 0, vmap, fb, rect); + drm_fb_xrgb8888_to_mono(buf, dst_pitch, vmap, fb, rect); ssd130x_update_rect(ssd130x, buf, rect);
ssd130x_clear_screen() allocates a temporary buffer sized to hold one byte per pixel, while it only needs to hold one bit per pixel. ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one byte per pixel for the whole frame buffer, while it only needs to hold one bit per pixel for the part that is to be updated. Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already calculated it anyway. Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays") Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/gpu/drm/solomon/ssd130x.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)