Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
> OLED display controllers.
>
> It's only the core part of the driver and a bus specific driver is needed
> for each transport interface supported by the display controllers.
>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

> +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
> +                              struct drm_rect *rect)
> +{
> +       unsigned int x = rect->x1;
> +       unsigned int y = rect->y1;
> +       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 pages = DIV_ROUND_UP(y % 8 + height, 8);
> +       u32 array_idx = 0;
> +       int ret, i, j, k;
> +       u8 *data_array = NULL;
> +
> +       data_array = kcalloc(width, pages, GFP_KERNEL);
> +       if (!data_array)
> +               return -ENOMEM;
> +
> +       /*
> +        * The screen is divided in pages, each having a height of 8
> +        * pixels, and the width of the screen. When sending a byte of
> +        * data to the controller, it gives the 8 bits for the current
> +        * column. I.e, the first byte are the 8 bits of the first
> +        * column, then the 8 bits for the second column, etc.
> +        *
> +        *
> +        * Representation of the screen, assuming it is 5 bits
> +        * wide. Each letter-number combination is a bit that controls
> +        * one pixel.
> +        *
> +        * A0 A1 A2 A3 A4
> +        * B0 B1 B2 B3 B4
> +        * C0 C1 C2 C3 C4
> +        * D0 D1 D2 D3 D4
> +        * E0 E1 E2 E3 E4
> +        * F0 F1 F2 F3 F4
> +        * G0 G1 G2 G3 G4
> +        * H0 H1 H2 H3 H4
> +        *
> +        * If you want to update this screen, you need to send 5 bytes:
> +        *  (1) A0 B0 C0 D0 E0 F0 G0 H0
> +        *  (2) A1 B1 C1 D1 E1 F1 G1 H1
> +        *  (3) A2 B2 C2 D2 E2 F2 G2 H2
> +        *  (4) A3 B3 C3 D3 E3 F3 G3 H3
> +        *  (5) A4 B4 C4 D4 E4 F4 G4 H4
> +        */
> +
> +       ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width);
> +       if (ret < 0)
> +               goto out_free;
> +
> +       ret = ssd130x_set_page_range(ssd130x, ssd130x->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 (8 * (i + 1) > ssd130x->height)
> +                       m = ssd130x->height % 8;
> +               for (j = x; j < x + width; j++) {
> +                       u8 data = 0;
> +
> +                       for (k = 0; k < m; k++) {
> +                               u8 byte = buf[(8 * i + k) * line_length + j / 8];

As buf does not point to (0, 0), the above is not correct if rect.x1 !=
0 or rect.y1 != 0.  After fixing that, writing more than one text line
to the console works, but I still see an issue with updates where the
rectangle size and/or position are not aligned to 8 pixels horizontally.
Will do more investigation, and send fixes...

> +                               u8 bit = (byte >> (j % 8)) & 1;
> +
> +                               data |= bit << k;
> +                       }
> +                       data_array[array_idx++] = data;
> +               }
> +       }
> +
> +       ret = ssd130x_write_data(ssd130x, data_array, width * pages);
> +
> +out_free:
> +       kfree(data_array);
> +       return ret;
> +}
> +
> +static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
> +{
> +       u8 *buf = NULL;
> +       struct drm_rect fullscreen = {
> +               .x1 = 0,
> +               .x2 = ssd130x->width,
> +               .y1 = 0,
> +               .y2 = ssd130x->height,
> +       };
> +
> +       buf = kcalloc(ssd130x->width, ssd130x->height, GFP_KERNEL);

This buffer is larger than needed. Will send a fix.

> +       if (!buf)
> +               return;
> +
> +       ssd130x_update_rect(ssd130x, buf, &fullscreen);
> +
> +       kfree(buf);
> +}
> +
> +static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
> +                               struct drm_rect *rect)
> +{
> +       struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> +       void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
> +       int ret = 0;
> +       u8 *buf = NULL;
> +
> +       buf = kcalloc(fb->width, fb->height, GFP_KERNEL);

This buffer is much larger than needed. Will send a fix.

> +       if (!buf)
> +               return -ENOMEM;
> +
> +       drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
> +
> +       ssd130x_update_rect(ssd130x, buf, rect);
> +
> +       kfree(buf);
> +
> +       return ret;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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