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

[Alistair Francis]

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.

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
>>>
>>>
>