Re: [PATCH v2 5/9] hw/core: stream: Add an end-of-packet flag

[Philippe Mathieu-Daudé]

Hi Edgar,

On 5/6/20 10:25 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Some stream clients stream an endless stream of data while
> other clients stream data in packets. Stream interfaces
> usually have a way to signal the end of a packet or the
> last beat of a transfer.
>
> This adds an end-of-packet flag to the push interface.
>
> Reviewed-by: Alistair Francis <address@hidden>
> Reviewed-by: Francisco Iglesias <address@hidden>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>   include/hw/stream.h     |  5 +++--
>   hw/core/stream.c        |  4 ++--
>   hw/dma/xilinx_axidma.c  | 10 ++++++----
>   hw/net/xilinx_axienet.c | 14 ++++++++++----
>   hw/ssi/xilinx_spips.c   |  2 +-
>   5 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/stream.h b/include/hw/stream.h
> index d02f62ca89..ed09e83683 100644
> --- a/include/hw/stream.h
> +++ b/include/hw/stream.h
> @@ -39,12 +39,13 @@ typedef struct StreamSlaveClass {
>        * @obj: Stream slave to push to
>        * @buf: Data to write
>        * @len: Maximum number of bytes to write
> +     * @eop: End of packet flag
>        */
> -    size_t (*push)(StreamSlave *obj, unsigned char *buf, size_t len);
> +    size_t (*push)(StreamSlave *obj, unsigned char *buf, size_t len, bool eop);

I'd split this patch, first add EOP in the push handler, keeping current 
code working, then the following patches (implementing the feature in 
the backend handlers), then ...

>   } StreamSlaveClass;
>   
>   size_t
> -stream_push(StreamSlave *sink, uint8_t *buf, size_t len);
> +stream_push(StreamSlave *sink, uint8_t *buf, size_t len, bool eop);

... this final patch, enable the feature and let the frontends use it.

>   
>   bool
>   stream_can_push(StreamSlave *sink, StreamCanPushNotifyFn notify,
> diff --git a/hw/core/stream.c b/hw/core/stream.c
> index 39b1e595cd..a65ad1208d 100644
> --- a/hw/core/stream.c
> +++ b/hw/core/stream.c
> @@ -3,11 +3,11 @@
>   #include "qemu/module.h"
>   
>   size_t
> -stream_push(StreamSlave *sink, uint8_t *buf, size_t len)
> +stream_push(StreamSlave *sink, uint8_t *buf, size_t len, bool eop)
>   {
>       StreamSlaveClass *k =  STREAM_SLAVE_GET_CLASS(sink);
>   
> -    return k->push(sink, buf, len);
> +    return k->push(sink, buf, len, eop);

So in this first part patch I'd use 'false' here, and update by 'eop' in 
the other part (last patch in series). Does it make sense?

Regards,

Phil.

>   }
>   
>   bool
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 4540051448..a770e12c96 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -283,7 +283,7 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>   
>           if (stream_desc_sof(&s->desc)) {
>               s->pos = 0;
> -            stream_push(tx_control_dev, s->desc.app, sizeof(s->desc.app));
> +            stream_push(tx_control_dev, s->desc.app, sizeof(s->desc.app), 
> true);
>           }
>   
>           txlen = s->desc.control & SDESC_CTRL_LEN_MASK;
> @@ -298,7 +298,7 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>           s->pos += txlen;
>   
>           if (stream_desc_eof(&s->desc)) {
> -            stream_push(tx_data_dev, s->txbuf, s->pos);
> +            stream_push(tx_data_dev, s->txbuf, s->pos, true);
>               s->pos = 0;
>               stream_complete(s);
>           }
> @@ -384,7 +384,7 @@ static void xilinx_axidma_reset(DeviceState *dev)
>   
>   static size_t
>   xilinx_axidma_control_stream_push(StreamSlave *obj, unsigned char *buf,
> -                                  size_t len)
> +                                  size_t len, bool eop)
>   {
>       XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(obj);
>       struct Stream *s = &cs->dma->streams[1];
> @@ -416,12 +416,14 @@ xilinx_axidma_data_stream_can_push(StreamSlave *obj,
>   }
>   
>   static size_t
> -xilinx_axidma_data_stream_push(StreamSlave *obj, unsigned char *buf, size_t 
> len)
> +xilinx_axidma_data_stream_push(StreamSlave *obj, unsigned char *buf, size_t 
> len,
> +                               bool eop)
>   {
>       XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(obj);
>       struct Stream *s = &ds->dma->streams[1];
>       size_t ret;
>   
> +    assert(eop);
>       ret = stream_process_s2mem(s, buf, len);
>       stream_update_irq(s);
>       return ret;
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index c8dfcda3ee..bd48305577 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -697,14 +697,14 @@ static void axienet_eth_rx_notify(void *opaque)
>                                              axienet_eth_rx_notify, s)) {
>           size_t ret = stream_push(s->tx_control_dev,
>                                    (void *)s->rxapp + CONTROL_PAYLOAD_SIZE
> -                                 - s->rxappsize, s->rxappsize);
> +                                 - s->rxappsize, s->rxappsize, true);
>           s->rxappsize -= ret;
>       }
>   
>       while (s->rxsize && stream_can_push(s->tx_data_dev,
>                                           axienet_eth_rx_notify, s)) {
>           size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem + s->rxpos,
> -                                 s->rxsize);
> +                                 s->rxsize, true);
>           s->rxsize -= ret;
>           s->rxpos += ret;
>           if (!s->rxsize) {
> @@ -874,12 +874,14 @@ static ssize_t eth_rx(NetClientState *nc, const uint8_t 
> *buf, size_t size)
>   }
>   
>   static size_t
> -xilinx_axienet_control_stream_push(StreamSlave *obj, uint8_t *buf, size_t len)
> +xilinx_axienet_control_stream_push(StreamSlave *obj, uint8_t *buf, size_t len,
> +                                   bool eop)
>   {
>       int i;
>       XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(obj);
>       XilinxAXIEnet *s = cs->enet;
>   
> +    assert(eop);
>       if (len != CONTROL_PAYLOAD_SIZE) {
>           hw_error("AXI Enet requires %d byte control stream payload\n",
>                    (int)CONTROL_PAYLOAD_SIZE);
> @@ -894,11 +896,15 @@ xilinx_axienet_control_stream_push(StreamSlave *obj, 
> uint8_t *buf, size_t len)
>   }
>   
>   static size_t
> -xilinx_axienet_data_stream_push(StreamSlave *obj, uint8_t *buf, size_t size)
> +xilinx_axienet_data_stream_push(StreamSlave *obj, uint8_t *buf, size_t size,
> +                                bool eop)
>   {
>       XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(obj);
>       XilinxAXIEnet *s = ds->enet;
>   
> +    /* We don't support fragmented packets yet.  */
> +    assert(eop);
> +
>       /* TX enable ?  */
>       if (!(s->tc & TC_TX)) {
>           return size;
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index c57850a505..4cfce882ab 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -868,7 +868,7 @@ static void xlnx_zynqmp_qspips_notify(void *opaque)
>   
>           memcpy(rq->dma_buf, rxd, num);
>   
> -        ret = stream_push(rq->dma, rq->dma_buf, num);
> +        ret = stream_push(rq->dma, rq->dma_buf, num, false);
>           assert(ret == num);
>           xlnx_zynqmp_qspips_check_flush(rq);
>       }