On Thu, Jul 15, 2021 at 2:33 PM Bruce Richardson
<bruce.richard...@intel.com> wrote:
>
> On Thu, Jul 15, 2021 at 12:40:01PM +0530, Jerin Jacob wrote:
> > )
> > a
> >
> > On Tue, Jul 13, 2021 at 6:01 PM Chengwen Feng <fengcheng...@huawei.com>
> > 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>
> >
> > Thanks for v3. Seems like all major items as covered. Some more
> > comments below inline.
> >
> > I would suggest v4 to split the patch like (so that we can review and
> > ack each patch)
> > 1) Only public header file with Doxygen inclusion, (There is a lot of
> > Doxygen syntax issue in the patch)
> > 2) 1 or more patches for implementation.
> >
>
> One additional follow-up comment on flags below.
>
> /Bruce
>
> >
> > > diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h
> > > new file mode 100644
> > > index 0000000..f6cc4e5
> <snip>
> > > + enum rte_dmadev_port_type port_type;
> > missing doxgen comment for this.
> > > + union {
> > > + /** For PCIE port
> > > + *
> > > + * The following model show SoC's PCIE module connects to
> > > + * multiple PCIE hosts and multiple endpoints. The PCIE
> > > module
> > > + * has an integrate DMA controller.
> > > + * If the DMA wants to access the memory of host A, it
> > > can be
> > > + * initiated by PF1 in core0, or by VF0 of PF0 in core0.
> > > + *
> <snip>
> > + /** The pasid filed in TLP packet */
> > > + uint64_t pasid : 20;
> > > + /** The attributes filed in TLP packet */
> > > + uint64_t attr : 3;
> > > + /** The processing hint filed in TLP packet */
> > > + uint64_t ph : 2;
> > > + /** The steering tag filed in TLP packet */
> > > + uint64_t st : 16;
> >
> > We don't support a few attributes like passid, ph, st. Do we need
> > the capability of this? or ignore this. In either case, please update the
> > doc.
> >
> > We also support additional flags for allocating LLC flag.
> > This is a hint to DMA engine that the cache blocks should be allocated
> > in the LLC (if they were not already).
> > When the MEM pointer is a destination in DMA operation, the referenced
> > cache blocks are allocated into the cache as part of completing the
> > DMA (when not already present in the LLC)
> > this is helpful if software has to access the data right after dma is
> > completed.
> >
> > Could you add bit or flag for the same?
> >
>
> I wonder if this is the best location for such a flag for LLC vs memory
> writes. It would also apply to memory-to-memory transactions, not just for
> those done to PCI devices.
Ack. it can be used for MEM to MEM
> As well as that, I think any flag should default
> to "on" rather than "off" since writing to cache rather than DRAM is
> generally the desired behaviour, I would think.
I think, keeping it is "allocate in LLC" on all transfer will not be good.
As large transters polute the LLC and dataplane may not touch the complete
data only header. Also in device copy, Adding it LLC there is an
additional cost unline MEM-MEM.
So IMO, better to add the flag to allow to allocate to LLC as a HINT.
> Should it be a per-operation flag, rather than per context?
Yes. better it be per-operation as it is the hint.
>
> <snip>
