qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH V4 6/8] Introduce xilinx dpdma.


From: Frederic Konrad
Subject: Re: [Qemu-devel] [PATCH V4 6/8] Introduce xilinx dpdma.
Date: Fri, 04 Sep 2015 11:01:51 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

On 04/09/2015 01:34, Alistair Francis wrote:
On Thu, Sep 3, 2015 at 12:28 AM, Frederic Konrad
<address@hidden> wrote:
On 02/09/2015 23:39, Alistair Francis wrote:
On Tue, Jul 21, 2015 at 10:17 AM,  <address@hidden> wrote:
From: KONRAD Frederic <address@hidden>

This is the implementation of the DPDMA.

Signed-off-by: KONRAD Frederic <address@hidden>
---
   hw/dma/Makefile.objs |   1 +
   hw/dma/xlnx_dpdma.c  | 790
+++++++++++++++++++++++++++++++++++++++++++++++++++
   hw/dma/xlnx_dpdma.h  |  85 ++++++
   3 files changed, 876 insertions(+)
   create mode 100644 hw/dma/xlnx_dpdma.c
   create mode 100644 hw/dma/xlnx_dpdma.h
What are the rules with the placement of the header files? Should the
header be in the include directory instead of being here?
Probably moving headers to include/hw makes sense here..
I will change it.
Hey Fred,

Great, thanks

[...]
+
+        if (xlnx_dpdma_desc_is_already_done(&desc)
+        && !xlnx_dpdma_desc_ignore_done_bit(&desc)) {
The second line of the if should be indented across.

Also, I generally think the operator should go on the first line, but
that doesn't seem worth changing throughout.
Ok I can do that.

+            /* We are trying to process an already processed descriptor.
*/
+            s->registers[DPDMA_EISR] |= ((1 << 25) << channel);
+            xlnx_dpdma_update_irq(s);
+            s->operation_finished[channel] = true;
+            DPRINTF("Already processed descriptor..\n");
+            break;
+        }
+
+        done = xlnx_dpdma_desc_is_last(&desc)
+             || xlnx_dpdma_desc_is_last_of_frame(&desc);
+
+        s->operation_finished[channel] = done;
+        if (s->data[channel]) {
+            int64_t transfer_len =
+
xlnx_dpdma_desc_get_transfer_size(&desc);
Why is this over two lines?

Probably because of the 79~80 columns limitation.
It looks like it is less then 80 columns though.
Oh yes missed that when I replaced xilinx by xlnx in functions..
I fixed that.

Thanks,
Fred

Thanks,

Alistair

Thanks,
Fred

Functioanlly it looks fine.

Besides the header file location, which someone else will need to comment
on:

Reviewed-by: Alistair Francis <address@hidden>
Tested-By: Hyun Kwon <address@hidden>

Thanks,

Alistair

+            uint32_t line_size = xlnx_dpdma_desc_get_line_size(&desc);
+            uint32_t line_stride =
xlnx_dpdma_desc_get_line_stride(&desc);
+            if (xlnx_dpdma_desc_is_contiguous(&desc)) {
+                source_addr[0] =
+                             xlnx_dpdma_desc_get_source_address(&desc,
0);
+                while (transfer_len != 0) {
+                    if (dma_memory_read(&address_space_memory,
+                                        source_addr[0],
+                                        &s->data[channel][ptr],
+                                        line_size)) {
+                        s->registers[DPDMA_ISR] |= ((1 << 12) <<
channel);
+                        xlnx_dpdma_update_irq(s);
+                        DPRINTF("Can't get data.\n");
+                        break;
+                    }
+                    ptr += line_size;
+                    transfer_len -= line_size;
+                    source_addr[0] += line_stride;
+                }
+            } else {
+                DPRINTF("Source address:\n");
+                int frag;
+                for (frag = 0; frag < 5; frag++) {
+                    source_addr[frag] =
+                          xlnx_dpdma_desc_get_source_address(&desc,
frag);
+                    DPRINTF("Fragment %u: %" PRIx64 "\n", frag + 1,
+                            source_addr[frag]);
+                }
+
+                frag = 0;
+                while ((transfer_len < 0) && (frag < 5)) {
+                    size_t fragment_len = DPDMA_FRAG_MAX_SZ
+                                    - (source_addr[frag] %
DPDMA_FRAG_MAX_SZ);
+
+                    if (dma_memory_read(&address_space_memory,
+                                        source_addr[frag],
+                                        &(s->data[channel][ptr]),
+                                        fragment_len)) {
+                        s->registers[DPDMA_ISR] |= ((1 << 12) <<
channel);
+                        xlnx_dpdma_update_irq(s);
+                        DPRINTF("Can't get data.\n");
+                        break;
+                    }
+                    ptr += fragment_len;
+                    transfer_len -= fragment_len;
+                    frag += 1;
+                }
+            }
+        }
+
+        if (xlnx_dpdma_desc_update_enabled(&desc)) {
+            /* The descriptor need to be updated when it's completed. */
+            DPRINTF("update the descriptor with the done flag set.\n");
+            xlnx_dpdma_desc_set_done(&desc);
+            dma_memory_write(&address_space_memory, desc_addr, &desc,
+                             sizeof(DPDMADescriptor));
+        }
+
+        if (xlnx_dpdma_desc_completion_interrupt(&desc)) {
+            DPRINTF("completion interrupt enabled!\n");
+            s->registers[DPDMA_ISR] |= (1 << channel);
+            xlnx_dpdma_update_irq(s);
+        }
+
+    } while (!done && !one_desc);
+
+    return ptr;
+}
+
+void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t
channel,
+                                         void *p)
+{
+    if (!s) {
+        qemu_log_mask(LOG_UNIMP, "DPDMA client not attached to valid
DPDMA"
+                      " instance\n");
+        return;
+    }
+
+    assert(channel <= 5);
+    s->data[channel] = p;
+}
+
+void xlnx_dpdma_trigger_vsync_irq(XlnxDPDMAState *s)
+{
+    s->registers[DPDMA_ISR] |= (1 << 27);
+    xlnx_dpdma_update_irq(s);
+}
+
+type_init(xlnx_dpdma_register_types)
diff --git a/hw/dma/xlnx_dpdma.h b/hw/dma/xlnx_dpdma.h
new file mode 100644
index 0000000..ae571a0
--- /dev/null
+++ b/hw/dma/xlnx_dpdma.h
@@ -0,0 +1,85 @@
+/*
+ * xlnx_dpdma.h
+ *
+ *  Copyright (C) 2015 : GreenSocs Ltd
+ *      http://www.greensocs.com/ , email: address@hidden
+ *
+ *  Developed by :
+ *  Frederic Konrad   <address@hidden>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef XLNX_DPDMA_H
+#define XLNX_DPDMA_H
+
+#include "hw/sysbus.h"
+#include "ui/console.h"
+#include "sysemu/dma.h"
+
+#define XLNX_DPDMA_REG_ARRAY_SIZE (0x1000 >> 2)
+
+struct XlnxDPDMAState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+    MemoryRegion iomem;
+    uint32_t registers[XLNX_DPDMA_REG_ARRAY_SIZE];
+    uint8_t *data[6];
+    bool operation_finished[6];
+    qemu_irq irq;
+};
+
+typedef struct XlnxDPDMAState XlnxDPDMAState;
+
+#define TYPE_XLNX_DPDMA "xlnx.dpdma"
+#define XLNX_DPDMA(obj) OBJECT_CHECK(XlnxDPDMAState, (obj),
TYPE_XLNX_DPDMA)
+
+/*
+ * xlnx_dpdma_start_operation: Start the operation on the specified
channel. The
+ *                             DPDMA gets the current descriptor and
retrieves
+ *                             data to the buffer specified by
+ *                             dpdma_set_host_data_location().
+ *
+ * Returns The number of bytes transfered by the DPDMA or 0 if an error
occured.
+ *
+ * @s The DPDMA state.
+ * @channel The channel to start.
+ */
+size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
+                                  bool one_desc);
+
+/*
+ * xlnx_dpdma_set_host_data_location: Set the location in the host
memory where
+ *                                    to store the data out from the dma
+ *                                    channel.
+ *
+ * @s The DPDMA state.
+ * @channel The channel associated to the pointer.
+ * @p The buffer where to store the data.
+ */
+/* XXX: add a maximum size arg and send an interrupt in case of
overflow. */
+void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t
channel,
+                                       void *p);
+
+/*
+ * xlnx_dpdma_trigger_vsync_irq: Trigger a VSYNC IRQ when the display is
+ *                               updated.
+ *
+ * @s The DPDMA state.
+ */
+void xlnx_dpdma_trigger_vsync_irq(XlnxDPDMAState *s);
+
+#endif /* !XLNX_DPDMA_H */
--
1.9.0






reply via email to

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