On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<cont...@steffen-goertz.de> 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 <cont...@steffen-goertz.de>
> + *
> + * 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.
