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

From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: damien.hedde@greensocs.com, peter.maydell@linaro.org,
	sstabellini@kernel.org, edgar.iglesias@xilinx.com,
	sai.pavan.boddu@xilinx.com, frasse.iglesias@gmail.com,
	jasowang@redhat.com, alistair@alistair23.me,
	qemu-devel@nongnu.org, frederic.konrad@adacore.com,
	qemu-arm@nongnu.org, figlesia@xilinx.com,
	luc.michel@greensocs.com
Subject: Re: [PATCH v2 5/9] hw/core: stream: Add an end-of-packet flag
Date: Wed, 6 May 2020 14:42:09 +0200	[thread overview]
Message-ID: <20200506124209.GK5519@toto> (raw)
In-Reply-To: <c7f6947d-7815-8df3-6835-3fe933ad4dbc@amsat.org>

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