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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Jul 16, 2021 at 8:34 AM fengchengwen <fengchengwen@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"

>
> [snip]
>
> thanks