| Message ID | d5f342b5382653c1f1fb72dbedb783f9ea42416e.1692888745.git.geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1 | expand |
Geert Uytterhoeven <geert@linux-m68k.org> writes: > The native display format is monochrome light-on-dark (R1). > Hence add support for R1, so monochrome applications not only look > better, but also avoid the overhead of back-and-forth conversions > between R1 and XR24. > > Do not allocate the intermediate conversion buffer when it is not > needed, and reorder the two buffer allocations to streamline operation. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > v2: > - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use > shadow-buffer helpers when managing plane's state") in drm/drm-next. > Hence I did not add Javier's tags given on v1. > - Do not allocate intermediate buffer when not needed. > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Javier Martinez Canillas <javierm@redhat.com> writes: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > >> The native display format is monochrome light-on-dark (R1). >> Hence add support for R1, so monochrome applications not only look >> better, but also avoid the overhead of back-and-forth conversions >> between R1 and XR24. >> >> Do not allocate the intermediate conversion buffer when it is not >> needed, and reorder the two buffer allocations to streamline operation. >> >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> --- >> v2: >> - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use >> shadow-buffer helpers when managing plane's state") in drm/drm-next. >> Hence I did not add Javier's tags given on v1. >> - Do not allocate intermediate buffer when not needed. >> --- > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat
Hi Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven: > The native display format is monochrome light-on-dark (R1). > Hence add support for R1, so monochrome applications not only look > better, but also avoid the overhead of back-and-forth conversions > between R1 and XR24. > > Do not allocate the intermediate conversion buffer when it is not > needed, and reorder the two buffer allocations to streamline operation. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > v2: > - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use > shadow-buffer helpers when managing plane's state") in drm/drm-next. > Hence I did not add Javier's tags given on v1. > - Do not allocate intermediate buffer when not needed. > --- > drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++---------- > 1 file changed, 51 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > index 78272b1f9d5b167f..18007cb4f3de5aa1 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) > > static int ssd130x_update_rect(struct ssd130x_device *ssd130x, > struct ssd130x_plane_state *ssd130x_state, > + u8 *buf, unsigned int pitch, > struct drm_rect *rect) > { > unsigned int x = rect->x1; > unsigned int y = rect->y1; > - u8 *buf = ssd130x_state->buffer; > u8 *data_array = ssd130x_state->data_array; > unsigned int width = drm_rect_width(rect); > unsigned int height = drm_rect_height(rect); > - unsigned int line_length = DIV_ROUND_UP(width, 8); > unsigned int page_height = ssd130x->device_info->page_height; > unsigned int pages = DIV_ROUND_UP(height, page_height); > struct drm_device *drm = &ssd130x->drm; > @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, > u8 data = 0; > > for (k = 0; k < m; k++) { > - u8 byte = buf[(8 * i + k) * line_length + j / 8]; > + u8 byte = buf[(8 * i + k) * pitch + j / 8]; > u8 bit = (byte >> (j % 8)) & 1; > > data |= bit << k; > @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state, The handing of different formats in this function is confusing. I'd rather implement ssd130x_fb_blit_rect_r1 and ssd130x_fb_blit_rect_xrgb8888 which then handle a single format. > struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); > unsigned int page_height = ssd130x->device_info->page_height; > struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > - u8 *buf = ssd130x_state->buffer; > struct iosys_map dst; > unsigned int dst_pitch; > int ret = 0; > + u8 *buf; > > /* Align y to display page boundaries */ > rect->y1 = round_down(rect->y1, page_height); > rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); > > - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); > + switch (fb->format->format) { > + case DRM_FORMAT_R1: > + /* Align x to byte boundaries */ > + rect->x1 = round_down(rect->x1, 8); > + rect->x2 = round_up(rect->x2, 8); Is rounding to multiples of 8 guaranteed to work? I know it did on VGA-compatible HW, because VGA enforces it. But is that generally the case? > > - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); > - if (ret) > - return ret; > + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); > + if (ret) > + return ret; > + > + dst_pitch = fb->pitches[0]; > + buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8; > + > + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch, > + rect); > + > + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > + break; > + > + case DRM_FORMAT_XRGB8888: > + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); > + buf = ssd130x_state->buffer; > + > + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); > + if (ret) > + return ret; > > - iosys_map_set_vaddr(&dst, buf); > - drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); > + iosys_map_set_vaddr(&dst, buf); > + drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); > > - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); > > - ssd130x_update_rect(ssd130x, ssd130x_state, rect); > + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch, > + rect); > + break; > + } > > return ret; > } > @@ -644,22 +667,24 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, > if (ret) > return ret; > > - fi = drm_format_info(DRM_FORMAT_R1); > - if (!fi) > - return -EINVAL; > + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > + if (!ssd130x_state->data_array) > + return -ENOMEM; > > - pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > + if (plane_state->fb->format->format != DRM_FORMAT_R1) { > + fi = drm_format_info(DRM_FORMAT_R1); > + if (!fi) > + return -EINVAL; > > - ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > - if (!ssd130x_state->buffer) > - return -ENOMEM; > + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); > > - ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); > - if (!ssd130x_state->data_array) { > - kfree(ssd130x_state->buffer); > - /* Set to prevent a double free in .atomic_destroy_state() */ > - ssd130x_state->buffer = NULL; > - return -ENOMEM; > + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); > + if (!ssd130x_state->buffer) { > + kfree(ssd130x_state->data_array); > + /* Set to prevent a double free in .atomic_destroy_state() */ > + ssd130x_state->data_array = NULL; > + return -ENOMEM; > + } > } > > return 0; > @@ -898,6 +923,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = { > }; > > static const uint32_t ssd130x_formats[] = { > + DRM_FORMAT_R1, > DRM_FORMAT_XRGB8888, > }; >
Hi Thomas, On Thu, Aug 31, 2023 at 10:01 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven: > > The native display format is monochrome light-on-dark (R1). > > Hence add support for R1, so monochrome applications not only look > > better, but also avoid the overhead of back-and-forth conversions > > between R1 and XR24. > > > > Do not allocate the intermediate conversion buffer when it is not > > needed, and reorder the two buffer allocations to streamline operation. > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > v2: > > - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use > > shadow-buffer helpers when managing plane's state") in drm/drm-next. > > Hence I did not add Javier's tags given on v1. > > - Do not allocate intermediate buffer when not needed. > > --- > > drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++---------- > > 1 file changed, 51 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > > index 78272b1f9d5b167f..18007cb4f3de5aa1 100644 > > --- a/drivers/gpu/drm/solomon/ssd130x.c > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > > @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) > > > > static int ssd130x_update_rect(struct ssd130x_device *ssd130x, > > struct ssd130x_plane_state *ssd130x_state, > > + u8 *buf, unsigned int pitch, > > struct drm_rect *rect) > > { > > unsigned int x = rect->x1; > > unsigned int y = rect->y1; > > - u8 *buf = ssd130x_state->buffer; > > u8 *data_array = ssd130x_state->data_array; > > unsigned int width = drm_rect_width(rect); > > unsigned int height = drm_rect_height(rect); > > - unsigned int line_length = DIV_ROUND_UP(width, 8); > > unsigned int page_height = ssd130x->device_info->page_height; > > unsigned int pages = DIV_ROUND_UP(height, page_height); > > struct drm_device *drm = &ssd130x->drm; > > @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, > > u8 data = 0; > > > > for (k = 0; k < m; k++) { > > - u8 byte = buf[(8 * i + k) * line_length + j / 8]; > > + u8 byte = buf[(8 * i + k) * pitch + j / 8]; > > u8 bit = (byte >> (j % 8)) & 1; > > > > data |= bit << k; > > @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state, > > The handing of different formats in this function is confusing. I'd > rather implement ssd130x_fb_blit_rect_r1 and > ssd130x_fb_blit_rect_xrgb8888 which then handle a single format. OK, will split. > > struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); > > unsigned int page_height = ssd130x->device_info->page_height; > > struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); > > - u8 *buf = ssd130x_state->buffer; > > struct iosys_map dst; > > unsigned int dst_pitch; > > int ret = 0; > > + u8 *buf; > > > > /* Align y to display page boundaries */ > > rect->y1 = round_down(rect->y1, page_height); > > rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); > > > > - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); > > + switch (fb->format->format) { > > + case DRM_FORMAT_R1: > > + /* Align x to byte boundaries */ > > + rect->x1 = round_down(rect->x1, 8); > > + rect->x2 = round_up(rect->x2, 8); > > Is rounding to multiples of 8 guaranteed to work? I know it did on > VGA-compatible HW, because VGA enforces it. But is that generally the case? That depends: some hardware requires e.g. 32-bit writes. But this driver is for a display with a serial (i2c/spi) interface, so everything should be fine ;-) Gr{oetje,eeting}s, Geert
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 78272b1f9d5b167f..18007cb4f3de5aa1 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct ssd130x_plane_state *ssd130x_state, + u8 *buf, unsigned int pitch, struct drm_rect *rect) { unsigned int x = rect->x1; unsigned int y = rect->y1; - u8 *buf = ssd130x_state->buffer; u8 *data_array = ssd130x_state->data_array; unsigned int width = drm_rect_width(rect); unsigned int height = drm_rect_height(rect); - unsigned int line_length = DIV_ROUND_UP(width, 8); unsigned int page_height = ssd130x->device_info->page_height; unsigned int pages = DIV_ROUND_UP(height, page_height); struct drm_device *drm = &ssd130x->drm; @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 data = 0; for (k = 0; k < m; k++) { - u8 byte = buf[(8 * i + k) * line_length + j / 8]; + u8 byte = buf[(8 * i + k) * pitch + j / 8]; u8 bit = (byte >> (j % 8)) & 1; data |= bit << k; @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state, struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev); unsigned int page_height = ssd130x->device_info->page_height; struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state); - u8 *buf = ssd130x_state->buffer; struct iosys_map dst; unsigned int dst_pitch; int ret = 0; + u8 *buf; /* Align y to display page boundaries */ rect->y1 = round_down(rect->y1, page_height); rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + switch (fb->format->format) { + case DRM_FORMAT_R1: + /* Align x to byte boundaries */ + rect->x1 = round_down(rect->x1, 8); + rect->x2 = round_up(rect->x2, 8); - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); - if (ret) - return ret; + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); + if (ret) + return ret; + + dst_pitch = fb->pitches[0]; + buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8; + + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch, + rect); + + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); + break; + + case DRM_FORMAT_XRGB8888: + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + buf = ssd130x_state->buffer; + + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); + if (ret) + return ret; - iosys_map_set_vaddr(&dst, buf); - drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); + iosys_map_set_vaddr(&dst, buf); + drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); - ssd130x_update_rect(ssd130x, ssd130x_state, rect); + ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch, + rect); + break; + } return ret; } @@ -644,22 +667,24 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane, if (ret) return ret; - fi = drm_format_info(DRM_FORMAT_R1); - if (!fi) - return -EINVAL; + ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); + if (!ssd130x_state->data_array) + return -ENOMEM; - pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); + if (plane_state->fb->format->format != DRM_FORMAT_R1) { + fi = drm_format_info(DRM_FORMAT_R1); + if (!fi) + return -EINVAL; - ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); - if (!ssd130x_state->buffer) - return -ENOMEM; + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width); - ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL); - if (!ssd130x_state->data_array) { - kfree(ssd130x_state->buffer); - /* Set to prevent a double free in .atomic_destroy_state() */ - ssd130x_state->buffer = NULL; - return -ENOMEM; + ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL); + if (!ssd130x_state->buffer) { + kfree(ssd130x_state->data_array); + /* Set to prevent a double free in .atomic_destroy_state() */ + ssd130x_state->data_array = NULL; + return -ENOMEM; + } } return 0; @@ -898,6 +923,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = { }; static const uint32_t ssd130x_formats[] = { + DRM_FORMAT_R1, DRM_FORMAT_XRGB8888, };
The native display format is monochrome light-on-dark (R1). Hence add support for R1, so monochrome applications not only look better, but also avoid the overhead of back-and-forth conversions between R1 and XR24. Do not allocate the intermediate conversion buffer when it is not needed, and reorder the two buffer allocations to streamline operation. Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- v2: - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use shadow-buffer helpers when managing plane's state") in drm/drm-next. Hence I did not add Javier's tags given on v1. - Do not allocate intermediate buffer when not needed. --- drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 25 deletions(-)