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

[Alistair Francis]

On Fri, Sep 4, 2015 at 2:01 AM, Frederic Konrad
<address@hidden> wrote:
> 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.

Great! Thanks

Alistair

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