On Thu, Jul 29, 2021 at 6:56 AM fengchengwen <fengcheng...@huawei.com> wrote:
>
> Thanks, inline comment
>
> On 2021/7/28 19:13, Bruce Richardson wrote:
> > On Tue, Jul 27, 2021 at 11:39:59AM +0800, Chengwen Feng wrote:
> >> This patch introduce 'dmadevice' which is a generic type of DMA
> >> device.
> >>
> >> The APIs of dmadev library exposes some generic operations which can
> >> enable configuration and I/O with the DMA devices.
> >>
> >> Signed-off-by: Chengwen Feng <fengcheng...@huawei.com>
> >> Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> >> Acked-by: Morten Brørup <m...@smartsharesystems.com>
> >> ---
> >
> > Thanks for this. Before it gets merged, I believe it needs to be split
> > further into multiple patches (say 4 or so) rather than adding the whole
> > lib in one go.
> >
> > Normally, I believe the split would be something like:
> > * basic device structures and infrastructure e.g. alloc and release
> > functions
> > * device config functions (and structures to go along with them)
> > such as configure and queue_setup
> > * data plane functions
> >
>
> I will try for it
> Maybe one patch for public file, one for pmd header file, one for
> implementation, and last for doc.
+1.
>
> > Documentation would be included in each of the patches, rather than done as
> > a block at the end.
> >
> > Besides that, I have one small additional requests for the API. Based off
> > feedback for ioat driver, we added in the following function to that API,
> > and we probably need something similar in dmadev:
> >
> > rte_ioat_burst_capacity()
> >
> > For our implementation this returns the number of elements that can be
> > enqueued to the ring, at least for the current burst/batch of packets. We
> > did the API this way because there can be additional limits beyond ring
> > size on each individual burst beyond just the raw ring capacity, e.g. even
> > if there are 4k ring elements free, there may be limits on the max burst
> > size the hardware can do, or limits on the number of outstanding
> > batches etc.
> >
> > Therefore can I request the addition of rte_dmadev_burst_capacity() [or
> > something similarly named] to the basic dmadev API set. For most hardware,
> > I think this will likely be the remaining free ring size, but I don't
> > believe the API should commit to that. The use case it was added for was to
> > enable an application which needs to do a multi-copy operation to check
> > that all copies can fit or not before enqueuing the first one. This is
> > important for hardware that doesn't have scatter-gather list support.
Yes. Could you add the following to enable scatter-gather support in
rte_dmadev_info::sge_max
/**<Maximum allowed number of scatter-gather entries in a single sg call. */
uint16_t sge_max;
>
> Remaining capacity can be inferred by ring_idx which return from enqueue and
> dequeue APIs.
> So I don't think this API needs to be added.
>
> For scatter-gather list, there maybe a hardware limit for max src or dst entry
> size, I prefer add 'max_sges' filed in struct rte_dmadev_info to indicate it.
>
> >
> > /Bruce
> >
> > PS: One typo in code flagged below too.
> >
> > <snip>
> >> + */
> >> +enum rte_dma_status_code {
> >> + RTE_DMA_STATUS_SUCCESSFUL,
> >> + /**< The operation completed successfully. */
> >> + RTE_DMA_STATUS_USRER_ABORT,
> >
> > Typo here ^^^
> >
>
> OK, USRER->USER will fix later
>
> > .
> >
