[dpdk-dev] [PATCH v4] dmadev: introduce DMA device library

[Bruce Richardson]

On Fri, Jul 16, 2021 at 06:10:48PM +0530, Jerin Jacob wrote:
> On Fri, Jul 16, 2021 at 8:34 AM fengchengwen <fengchengwen at huawei.com> wrote:
> >
> > On 2021/7/16 0:33, Bruce Richardson wrote:
> > > On Fri, Jul 16, 2021 at 12:04:33AM +0800, fengchengwen wrote:
> > >> @burce, jerin  Some unmodified review comments are returned here:
> > >>
> >
> > [snip]
> >
> > >
> > >> 2.  COMMENT: > + * @see struct rte_dmadev_info::dev_capa
> > >>> + */
> > >> Drop this flag as unnecessary. All devices either always provide ordering
> > >> guarantee - in which case it's a no-op - or else support the flag.
> > >>
> > >> REPLY: I prefer define it, it could let user know whether support fence.
> > >>
> > > I don't see it that way. The flag is pointless because the application
> > > can't use it to make any decisions. If two operations require ordering the
> > > application must use the fence flag because if the device doesn't guarantee
> > > ordering it's necessary, and if the device does guarantee ordering it's
> > > better and easier to just specify the flag than to put in code branches.
> > > Having this as a capability is just going to confuse the user - better to
> > > just say that if you need ordering, put in a fence.
> > >
> >
> > If driver don't support fence, and application set the fence flag, What's
> > driving behavior like? return error or implement fence at driver layer ?
> >
> > If expose the fence capability to application, then application could decide
> > which option to use. e.g. commit the operations before the fence and make sure it completes,
> > or use another collaborative approach.
> >
> > I think in this manner, the driver implementation can be simplified.
> >
> > [snip]
> >
> > >> 4.
> > >> COMMENT: > +        * @see RTE_DMA_DEV_TO_MEM
> > >>> +        * @see RTE_DMA_DEV_TO_DEV
> > >> Since we can set of only one direction per vchan . Should be we make
> > >> it as enum to
> > >> make it clear.
> > >>
> > >> REPLY: May some devices support it. I think it's OK for future use.
> > >>
> > > +1
> > > That may need a capability flag though, to indicate if a device supports
> > > multi-direction in a single vchannel.
> >
> > There are a lot of combinations, and I tend not to add a capability for multi-direction.
> > Currently, no device supports multiple directions, So can we delay that definition?
> 
> Yes. IMO, we need to change the comment  from "Set of supported
> transfer directions" to
> "Transfer direction"
> 
> If channel supports multiple directions, then in fastpath we need
> another flag to select
> which direction to use.
> 
> IMO, Since none of the devices supports this mode, I think, we should
> change the comment
> to "Transfer direction"
> 
Ok for me.