qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V3 7/8] Introduce xilinx dp.


From: Hyun Kwon
Subject: Re: [Qemu-devel] [PATCH V3 7/8] Introduce xilinx dp.
Date: Sat, 11 Jul 2015 00:29:11 +0000

Hi Fred,

Thanks for the patch.

> -----Original Message-----
> From: address@hidden [mailto:address@hidden
> Sent: Monday, July 06, 2015 9:22 AM
> To: address@hidden
> Cc: address@hidden; Peter Crosthwaite; Hyun Kwon;
> address@hidden; address@hidden;
> address@hidden
> Subject: [PATCH V3 7/8] Introduce xilinx dp.
>
> From: KONRAD Frederic <address@hidden>
>
> This is the implementation of the DisplayPort.
>
> It has an aux-bus to access dpcd and edid .
>
> Graphic plane is connected to the channel 3.
> Video plane is connected to the channel 0.
> Audio stream are connected to the channels 4 and 5.
>
> Signed-off-by: KONRAD Frederic <address@hidden>
> ---
>  hw/display/Makefile.objs |    1 +
>  hw/display/xlnx_dp.c     | 1370
> ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/display/xlnx_dp.h     |  110 ++++
>  3 files changed, 1481 insertions(+)
>  create mode 100644 hw/display/xlnx_dp.c
>  create mode 100644 hw/display/xlnx_dp.h
>
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 6d7004a..ee615c2 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -39,3 +39,4 @@ obj-$(CONFIG_VIRTIO) += virtio-gpu.o
>  obj-$(CONFIG_VIRTIO_PCI) += virtio-gpu-pci.o
>  obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
>  obj-$(CONFIG_DPCD) += dpcd.o
> +obj-$(CONFIG_XLNX_ZYNQMP) += xlnx_dp.o
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> new file mode 100644
> index 0000000..18362df
> --- /dev/null
> +++ b/hw/display/xlnx_dp.c
> @@ -0,0 +1,1370 @@
> +/*
> + * xlnx_dp.c

[snip]

> +
> +static void xlnx_dp_audio_callback(void *opaque, int avail)
> +{
> +    /*
> +     * Get some data from the DPDMA and compute these datas.
> +     * Then wait for QEMU's audio subsystem to call this callback.
> +     */
> +    XlnxDPState *s = XLNX_DP(opaque);
> +    size_t written = 0;
> +    static uint8_t buffer;

This 'buffer' doesn't seem to be needed anywhere.

> +
> +    /* If there are already some data don't get more data. */
> +    if (s->byte_left == 0) {
> +        buffer++;
> +        s->audio_data_available[0] = xlnx_dpdma_start_operation(s->dpdma,
> 4,
> +                                                                  true);
> +        s->audio_data_available[1] = xlnx_dpdma_start_operation(s->dpdma,
> 5,
> +                                                                  true);
> +        xlnx_dp_audio_mix_buffer(s);
> +    }
> +
> +    /* Send the buffer through the audio. */
> +    if (s->byte_left <= MAX_QEMU_BUFFER_SIZE) {

How was the value of MAX_QEMU_BUFFER_SIZE decided? Doesn't 'avail' have to be 
used instead to check min or max amount of data to write? I'd like to make sure 
this is fine.

> +        if (s->byte_left != 0) {
> +            written = AUD_write(s->amixer_output_stream,
> +                                &s->out_buffer[s->data_ptr], s->byte_left);
> +        } else {
> +            /*
> +             * There is nothing to play.. We don't have any data! Fill the
> +             * buffer with zero's and send it.
> +             */
> +            written = 0;
> +            memset(s->out_buffer, 0, 1024);
> +            AUD_write(s->amixer_output_stream, s->out_buffer, 1024);
> +        }
> +    } else {
> +        written = AUD_write(s->amixer_output_stream,
> +                            &s->out_buffer[s->data_ptr], 
> MAX_QEMU_BUFFER_SIZE);
> +    }
> +    s->byte_left -= written;
> +    s->data_ptr += written;
> +}
> +

[snip]


> +
> +/*
> + * Change the graphic format of the surface.
> + * XXX: To be completed.
> + */
> +static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
> +{
> +    switch (s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK)
> {
> +    case DP_GRAPHIC_RGBA8888:
> +        s->g_plane.format = PIXMAN_r8g8b8a8;
> +        break;
> +    case DP_GRAPHIC_ABGR8888:
> +        s->g_plane.format = PIXMAN_a8b8g8r8;
> +        break;
> +    case DP_GRAPHIC_RGB565:
> +        s->g_plane.format = PIXMAN_r5g6b5;
> +        break;
> +    case DP_GRAPHIC_RGB888:
> +        s->g_plane.format = PIXMAN_r8g8b8;
> +        break;
> +    case DP_GRAPHIC_BGR888:
> +        s->g_plane.format = PIXMAN_b8g8r8;
> +        break;
> +    default:
> +        DPRINTF("error: unsupported graphic format %u.\n",
> +                s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
> +        abort();
> +        break;
> +    }
> +
> +    switch (s->avbufm_registers[AV_BUF_FORMAT] &
> DP_NL_VID_FMT_MASK) {
> +    case 0:
> +        s->v_plane.format = PIXMAN_r8g8b8a8;
> +        break;
> +    case DP_NL_VID_RGBA8880:
> +        s->v_plane.format = PIXMAN_r8g8b8a8;

The pixman format for DP_NL_VID_RGBA8880 and default should be PIXMAN_x8b8g8r8. 
The DP documentation has confusing naming scheme. :-)

> +        break;
> +    default:
> +        DPRINTF("error: unsupported video format %u.\n",
> +                s->avbufm_registers[AV_BUF_FORMAT] &
> DP_NL_VID_FMT_MASK);
> +        abort();
> +        break;
> +    }
> +
> +    xlnx_dp_recreate_surface(s);
> +}
> +

[snip]

> +
> +static void xlnx_dp_update_display(void *opaque)
> +{
> +    XlnxDPState *s = XLNX_DP(opaque);
> +
> +    if (DEBUG_DP) {
> +        int64_t last_time = 0;
> +        int64_t frame = 0;
> +        int64_t time = get_clock();
> +        int64_t fps;
> +
> +        if (last_time == 0) {
> +            last_time = get_clock();
> +        }
> +        frame++;
> +        if (last_time + 1000000000 < time) {
> +            fps = (1000000000.0 * frame) / (time - last_time);
> +            last_time = time;
> +            frame = 0;
> +            DPRINTF("xlnx_dp: %ldfps\n", fps);
> +        }
> +    }
> +
> +
> +    if ((s->core_registers[DP_TRANSMITTER_ENABLE] & 0x01) == 0) {
> +        return;
> +    }
> +
> +    s->core_registers[DP_INT_STATUS] |= (1 << 13);
> +    xlnx_dp_update_irq(s);
> +
> +    /*
> +     * Trigger the DMA channel.
> +     */
> +    if (!xlnx_dpdma_start_operation(s->dpdma, 3, false)) {
> +        /*
> +         * An error occured don't do anything with the data..
> +         * Trigger an underflow interrupt.
> +         */
> +        s->core_registers[DP_INT_STATUS] |= (1 << 21);
> +        xlnx_dp_update_irq(s);
> +        return;
> +    }
> +
> +    if (xlnx_dp_global_alpha_enabled(s)) {
> +        if (!xlnx_dpdma_start_operation(s->dpdma, 0, false)) {
> +            s->core_registers[DP_INT_STATUS] |= (1 << 21);
> +            xlnx_dp_update_irq(s);
> +            return;
> +        }
> +        xlnx_dp_blend_surface(s);
> +    }

I'm not sure when to generate the underflow DP interrupt. The current 
implementation doesn't catch any partial transaction (ex, when only 500 lines 
are fetched from 1000 lines) done by xlnx_dpdma_start_operation(). The best way 
would be to calculate the actual size of requested surface, and compare it to 
the return value of xlnx_dpdma_start_operation(). But, that can be implemented 
later, and I'm fine with as is.

In addition, currently xlnx_dp_update_display() assumes that the channel 3 is 
required to be running, and the channel 0 runs on top optionally. In fact, the 
other way around should work as well. How about something like below?

        xlnx_dpdma_trigger_vsync();

        if (!xlnx_dpdma_start_operation(channel 3) && 
!xlnx_dpdma_start_operation(channe 0)) {
                return;
        }

        If (xlnx_dp_global_alpha_enabled()) {
                xlnx_dp_blend_surface();
        }

        dpy_gfx_update();

We may also want to move xlnx_dp_global_alpha_enabled() into 
xlnx_dp_blend_surface() if we consider that pixel-by-pixel blending support can 
be added later.

> +
> +    /*
> +     * XXX: We might want to update only what changed.
> +     */
> +    dpy_gfx_update(s->console, 0, 0, surface_width(s->g_plane.surface),
> +                                     surface_height(s->g_plane.surface));
> +}
> +

[snip]

> +
> +static void xlnx_dp_realize(DeviceState *dev, Error **errp)
> +{
> +    XlnxDPState *s = XLNX_DP(dev);
> +    DisplaySurface *surface;
> +    struct audsettings as;
> +
> +    s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
> +    surface = qemu_console_surface(s->console);
> +    xlnx_dpdma_set_host_data_location(s->dpdma,
> DP_GRAPHIC_DMA_CHANNEL,
> +                                      surface_data(surface));
> +
> +    as.freq = 44100;
> +    as.nchannels = 2;
> +    as.fmt = AUD_FMT_S16;
> +    as.endianness = 0;

Do you know if this is configurable at run time? It looks like it from what I 
can tell from other source code. I'm fine with fixed configuration for now, but 
in theory, the Xilinx DP supports dynamic configuration of these.

Thanks,
-hyun



This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]