| Message ID | 20200506082513.18751-6-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/core: stream: Add end-of-packet flag | expand |
Hi Edgar, On 5/6/20 10:25 AM, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > 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 <alistair.francis@wdc.com> > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > 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); > } >
On Wed, May 06, 2020 at 01:53:33PM +0200, Philippe Mathieu-Daudé wrote: > Hi Edgar, Hi Philippe, > > On 5/6/20 10:25 AM, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > 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 <alistair.francis@wdc.com> > > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > 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? Current code implicitly assumes eop = true, so this patch keeps things working as before. It just makes the assumption explicit and guarding backends with asserts. The support for eop = false is then added (where relevant) in future patches, roughly the way you describe it. I can add something to the commit message to clarify that. Best regards, Edgar
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); } 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); 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); } 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); }