[v2,4/8] drm/ssd130x: Add support for DRM_FORMAT_R1
Commit Message
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(-)
Comments
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
@@ -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,
};