Re: [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device

[Stefan Hajnoczi]

On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<address@hidden> wrote:
> +static void led_timer_expire(void *opaque)
> +{
> +    LEDMatrixState *s = LED_MATRIX(opaque);
> +/*
> +    uint8_t divider = s->strobe_row ? s->nrows : s->ncols;
> +    int64_t max_on = (s->refresh_period * 1000) / divider;
> +*/

Please remove dead code.

> +
> +    update_on_times(s);
> +
> +    memcpy(s->led_frame_dc, s->led_working_dc,
> +           sizeof(int64_t) * s->nrows * s->ncols);
> +    memset(s->led_working_dc, 0x00, sizeof(int64_t) * s->nrows * s->ncols);
> +
> +    timer_mod(&s->timer,
> +            qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + s->refresh_period);
> +    s->redraw = true;
> +}

Have you considered an approach based on the access pattern rather
than a timer (sampling)?

Sampling-based approaches are more CPU intensive and may result in
artifacts if the guest doesn't update pins at exactly the refresh
rate.

The access pattern approach assumes that the rows/columns are scanned
in a certain way.  It uses the guest's pin accesses as the event for
checking the matrix state.  It's more efficient, doesn't suffer from
sampling artifacts, but requires knowledge of how rows/columns are
scanned.

> +static void draw_pixel(DisplaySurface *ds, int x, int y, uint32_t color)
> +{
> +    int bpp;
> +    uint8_t *d;
> +    bpp = (surface_bits_per_pixel(ds) + 7) >> 3;
> +    d = surface_data(ds) + surface_stride(ds) * y + bpp * x;
> +    switch (bpp) {
> +    case 1:
> +        *((uint8_t *) d) = color;
> +        d++;
> +        break;
> +    case 2:
> +        *((uint16_t *) d) = color;
> +        d += 2;
> +        break;
> +    case 4:
> +        *((uint32_t *) d) = color;
> +        d += 4;
> +        break;
> +    }

There is no need to increment d.

> +    for (x = 0; x < s->nrows ; x++) {
> +        for (y = 0; y < s->ncols; y++) {
> +            idx = x * s->ncols + y;

I'm confused by the naming here.  "rows" is height/vertical/y.
"columns" is width/horizontal/x.  Why is "x" used for vertical and "y"
for horizontal?

> +            red = (s->led_frame_dc[idx] * 0xFF) / (s->refresh_period * 1000);
> +            color_led = colorfunc(red, 0x00, 0x00);
> +
> +            draw_box(surface, idx * 10, 0, 5, 10, color_led);

Are the LEDs layed out horizontally?  The y coordinate is hardcoded to
0.  I thought this would be a 2D LED matrix.

> +static void led_matrix_unrealize(DeviceState *dev, Error **errp)
> +{
> +    LEDMatrixState *s = LED_MATRIX(dev);
> +
> +    g_free(s->led_working_dc);
> +    s->led_working_dc = NULL;

led_frame_dc is leaked.

graphic_console_close() is missing.

> diff --git a/include/hw/display/led_matrix.h b/include/hw/display/led_matrix.h
> new file mode 100644
> index 0000000000..4a43b69f5b
> --- /dev/null
> +++ b/include/hw/display/led_matrix.h
> @@ -0,0 +1,38 @@
> +/*
> + * LED Matrix Demultiplexer
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef LED_MATRIX_H
> +#define LED_MATRIX_H
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#define TYPE_LED_MATRIX "led_matrix"
> +#define LED_MATRIX(obj) OBJECT_CHECK(LEDMatrixState, (obj), TYPE_LED_MATRIX)
> +
> +typedef struct LEDMatrixState {

There is too little documentation here for a generic device that could
be reused by other boards.  Please describe the purpose and
functionality of this device.