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.igles...@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.fran...@wdc.com>
> > Reviewed-by: Francisco Iglesias <frasse.igles...@gmail.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.igles...@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
