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

[Frederic Konrad <fred.konrad@greensocs.com>]

On 02/09/2015 23:39, Alistair Francis wrote:
> On Tue, Jul 21, 2015 at 10:17 AM,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This is the implementation of the DPDMA.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   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.

[...]
>> +
>> +        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.

Thanks,
Fred
>
> Functioanlly it looks fine.
>
> Besides the header file location, which someone else will need to comment on:
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> Tested-By: Hyun Kwon <hyun.kwon@xilinx.com>
>
> 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: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad   <fred.konrad@greensocs.com>
>> + *
>> + * 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
>>
>>