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

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

On 04/09/2015 01:34, Alistair Francis wrote:
> On Thu, Sep 3, 2015 at 12:28 AM, Frederic Konrad
> <fred.konrad@greensocs.com> wrote:
>> 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.
> 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 <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
>>>>
>>>>