Re: [PATCH v3 1/5] hw/display : Add device DM163

[Peter Maydell]

On Wed, 28 Feb 2024 at 12:02, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>
> This device implements the IM120417002 colors shield v1.1 for Arduino
> (which relies on the DM163 8x3-channel led driving logic) and features
> a simple display of an 8x8 RGB matrix. The columns of the matrix are
> driven by the DM163 and the rows are driven externally.
>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  docs/system/arm/b-l475e-iot01a.rst |   3 +-
>  include/hw/display/dm163.h         |  57 ++++++
>  hw/display/dm163.c                 | 308 +++++++++++++++++++++++++++++
>  hw/display/Kconfig                 |   3 +
>  hw/display/meson.build             |   1 +
>  hw/display/trace-events            |  13 ++
>  6 files changed, 384 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/display/dm163.h
>  create mode 100644 hw/display/dm163.c
>
> diff --git a/docs/system/arm/b-l475e-iot01a.rst 
> b/docs/system/arm/b-l475e-iot01a.rst
> index 0afef8e4f4..60b9611167 100644
> --- a/docs/system/arm/b-l475e-iot01a.rst
> +++ b/docs/system/arm/b-l475e-iot01a.rst
> @@ -12,13 +12,14 @@ USART, I2C, SPI, CAN and USB OTG, as well as a variety of 
> sensors.
>  Supported devices
>  """""""""""""""""
>
> -Currently B-L475E-IOT01A machine's only supports the following devices:
> +Currently B-L475E-IOT01A machine's supports the following devices:

"machines support"

>
>  - Cortex-M4F based STM32L4x5 SoC
>  - STM32L4x5 EXTI (Extended interrupts and events controller)
>  - STM32L4x5 SYSCFG (System configuration controller)
>  - STM32L4x5 RCC (Reset and clock control)
>  - STM32L4x5 GPIOs (General-purpose I/Os)
> +- optional 8x8 led display (based on DM163 driver)
>
>  Missing devices
>  """""""""""""""
> diff --git a/include/hw/display/dm163.h b/include/hw/display/dm163.h
> new file mode 100644
> index 0000000000..aa775e51e1
> --- /dev/null
> +++ b/include/hw/display/dm163.h
> @@ -0,0 +1,57 @@
> +/*
> + * QEMU DM163 8x3-channel constant current led driver
> + * driving columns of associated 8x8 RGB matrix.
> + *
> + * Copyright (C) 2024 Samuel Tardieu <sam@rfc1149.net>
> + * Copyright (C) 2024 Arnaud Minier <arnaud.minier@telecom-paris.fr>
> + * Copyright (C) 2024 Inès Varhol <ines.varhol@telecom-paris.fr>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_DISPLAY_DM163_H
> +#define HW_DISPLAY_DM163_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_DM163 "dm163"
> +OBJECT_DECLARE_SIMPLE_TYPE(DM163State, DM163);
> +
> +#define DM163_NUM_LEDS 24
> +#define RGB_MATRIX_NUM_ROWS 8
> +#define RGB_MATRIX_NUM_COLS (DM163_NUM_LEDS / 3)
> +#define COLOR_BUFFER_SIZE RGB_MATRIX_NUM_ROWS
> +
> +typedef struct DM163State {
> +    DeviceState parent_obj;
> +
> +    /* DM163 driver */
> +    uint64_t bank0_shift_register[3];
> +    uint64_t bank1_shift_register[3];
> +    uint16_t latched_outputs[DM163_NUM_LEDS];
> +    uint16_t outputs[DM163_NUM_LEDS];
> +    qemu_irq sout;
> +
> +    uint8_t dck;
> +    uint8_t en_b;
> +    uint8_t lat_b;
> +    uint8_t rst_b;
> +    uint8_t selbk;
> +    uint8_t sin;
> +
> +    /* IM120417002 colors shield */
> +    uint8_t activated_rows;
> +
> +    /* 8x8 RGB matrix */
> +    QemuConsole *console;
> +    /* Rows currently being displayed on the matrix. */
> +    /* The last row is filled with 0 (turned off row) */
> +    uint32_t buffer[COLOR_BUFFER_SIZE + 1][RGB_MATRIX_NUM_COLS];
> +    uint8_t last_buffer_idx;
> +    uint8_t buffer_idx_of_row[RGB_MATRIX_NUM_ROWS];
> +    /* Used to simulate retinal persistance of rows */

"persistence" (I think there are some other instances of this
typo below as well.)

> +    uint8_t age_of_row[RGB_MATRIX_NUM_ROWS];
> +} DM163State;
> +
> +#endif /* HW_DISPLAY_DM163_H */
> diff --git a/hw/display/dm163.c b/hw/display/dm163.c
> new file mode 100644
> index 0000000000..87e886356a
> --- /dev/null
> +++ b/hw/display/dm163.c
> @@ -0,0 +1,308 @@
> +/*
> + * QEMU DM163 8x3-channel constant current led driver
> + * driving columns of associated 8x8 RGB matrix.
> + *
> + * Copyright (C) 2024 Samuel Tardieu <sam@rfc1149.net>
> + * Copyright (C) 2024 Arnaud Minier <arnaud.minier@telecom-paris.fr>
> + * Copyright (C) 2024 Inès Varhol <ines.varhol@telecom-paris.fr>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +/*
> + * The reference used for the DM163 is the following :
> + * http://www.siti.com.tw/product/spec/LED/DM163.pdf
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/display/dm163.h"
> +#include "ui/console.h"
> +#include "trace.h"
> +
> +#define LED_SQUARE_SIZE 100
> +/* Number of frames a row stays visible after being turned off. */
> +#define ROW_PERSISTANCE 4
> +
> +static const VMStateDescription vmstate_dm163 = {
> +    .name = TYPE_DM163,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT8(activated_rows, DM163State),
> +        VMSTATE_UINT64_ARRAY(bank0_shift_register, DM163State, 3),
> +        VMSTATE_UINT64_ARRAY(bank1_shift_register, DM163State, 3),
> +        VMSTATE_UINT16_ARRAY(latched_outputs, DM163State, DM163_NUM_LEDS),
> +        VMSTATE_UINT16_ARRAY(outputs, DM163State, DM163_NUM_LEDS),
> +        VMSTATE_UINT8(dck, DM163State),
> +        VMSTATE_UINT8(en_b, DM163State),
> +        VMSTATE_UINT8(lat_b, DM163State),
> +        VMSTATE_UINT8(rst_b, DM163State),
> +        VMSTATE_UINT8(selbk, DM163State),
> +        VMSTATE_UINT8(sin, DM163State),
> +        VMSTATE_UINT32_2DARRAY(buffer, DM163State,
> +            COLOR_BUFFER_SIZE + 1, RGB_MATRIX_NUM_COLS),
> +        VMSTATE_UINT8(last_buffer_idx, DM163State),
> +        VMSTATE_UINT8_ARRAY(buffer_idx_of_row, DM163State, 
> RGB_MATRIX_NUM_ROWS),
> +        VMSTATE_UINT8_ARRAY(age_of_row, DM163State, RGB_MATRIX_NUM_ROWS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void dm163_reset_hold(Object *obj)
> +{
> +    DM163State *s = DM163(obj);
> +
> +    /* Reset only stops the PWM. */
> +    memset(s->outputs, 0, sizeof(s->outputs));
> +
> +    /* The last row of the buffer stores a turned off row */
> +    memset(s->buffer[COLOR_BUFFER_SIZE], 0, sizeof(s->buffer[0]));

This reset function seems to be missing reset handling for
a lot of this device's state.

> +}


> +static void dm163_invalidate_display(void *opaque)
> +{
> +}

I'm not sure but I think the invalidate_display method is supposed
to at least do a qemu_console_resize() to the correct size.
It also typically sets a flag to tell the update_display method
that it needs to redraw the whole window.

> +
> +static void dm163_update_display(void *opaque)
> +{
> +    DM163State *s = (DM163State *)opaque;
> +    DisplaySurface *surface = qemu_console_surface(s->console);
> +    uint32_t *dest;
> +    unsigned bits_ppi = surface_bits_per_pixel(surface);
> +
> +    /* Should the code be updated to handle other bpp than 32 ? */

No. The UI layer guarantees that the console surface is always
32 bits per pixel. You don't need to assert that this is true.
(We still have some old display driver models which have legacy
code we never cleaned up from back when they had to cope with
possibly different console surface bit depths; but the handling
of other depths is just dead code in those devices.)

> +    /* trace_dm163_bits_ppi(bits_ppi); */
> +    g_assert((bits_ppi == 32));
> +    dest = surface_data(surface);
> +    for (unsigned y = 0; y < RGB_MATRIX_NUM_ROWS; y++) {
> +        for (unsigned _ = 0; _ < LED_SQUARE_SIZE; _++) {
> +            for (int x = RGB_MATRIX_NUM_COLS * LED_SQUARE_SIZE - 1; x >= 0; 
> x--) {
> +                *dest++ = s->buffer[s->buffer_idx_of_row[y]][x / 
> LED_SQUARE_SIZE];
> +            }
> +        }
> +        if (s->age_of_row[y]) {
> +            s->age_of_row[y]--;
> +            if (!s->age_of_row[y]) {
> +                /*
> +                 * If the ROW_PERSISTANCE delay is up,
> +                 * the row is turned off.
> +                 * (s->buffer[COLOR_BUFFER] is filled with 0)
> +                 */
> +                s->buffer_idx_of_row[y] = COLOR_BUFFER_SIZE;
> +            }
> +        }
> +    }
> +    /*
> +     * Ideally set the refresh rate so that the row persistance
> +     * doesn't need to be changed.
> +     *
> +     * Currently `dpy_ui_info_supported(s->console)` returns false
> +     * which makes it impossible to get or set UIInfo.
> +     */
> +    if (dpy_ui_info_supported(s->console)) {
> +        trace_dm163_refresh_rate(dpy_get_ui_info(s->console)->refresh_rate);
> +    } else {
> +        trace_dm163_refresh_rate(0);
> +    }

Why does this device care about the console's refresh rate at all?
It should just be displaying what it needs to display.
No other device in hw/display calls dpy_get_ui_info().

(The reason it returns false is because your GraphicHwOps struct
doesn't define a ui_info function -- dpy_get_ui_info()/dpy_set_ui_info()
is for the UI layer to tell the display device about changes in things
like refresh rate and window size, not for the display device to ask
something else for this information. This is typically only needed by
virtual devices like virtio-vga or xenfb, though, so I wouldn't
expect you to need to set a ui_info function.)

> +
> +    dpy_gfx_update(s->console, 0, 0, RGB_MATRIX_NUM_COLS * LED_SQUARE_SIZE,
> +                   RGB_MATRIX_NUM_ROWS * LED_SQUARE_SIZE);

This will redraw the entire 800x800 window every time, which is
not very efficient. Do we really need to do that?

> +}
> +
> +static const GraphicHwOps dm163_ops = {
> +    .invalidate  = dm163_invalidate_display,
> +    .gfx_update  = dm163_update_display,
> +};

thanks
-- PMM