linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Mikhak <alan.mikhak@sifive.com>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc: "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"kishon@ti.com" <kishon@ti.com>,
	"paul.walmsley@sifive.com" <paul.walmsley@sifive.com>
Subject: Re: [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
Date: Wed, 15 Apr 2020 11:55:32 -0700	[thread overview]
Message-ID: <CABEDWGw0OyQNppLpDaNgMedfB0Ci=kZVKm+h4T-LJoZYmbSgqA@mail.gmail.com> (raw)
In-Reply-To: <DM5PR12MB1276E09460BD4DB7E70EAF91DADB0@DM5PR12MB1276.namprd12.prod.outlook.com>

On Wed, Apr 15, 2020 at 11:17 AM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> Hi Alan,
>
> > > I like your approach, it separates the PCIe glue logic from the eDMA
> > > itself.
> > > I would suggest that pcitest would have multiple options that could be
> > > triggered, for instance:
> > >  1 - Execute Endpoint DMA (read/write) remotely with Linked List feature
> > > (from the Root Complex side)
> > >  2 - Execute Endpoint DMA (read/write) remotely without Linked List
> > > feature (from the Root Complex side)
> > >  3 - Execute Endpoint DMA (read/write) locally with Linked List feature
> > >  4 - Execute Endpoint DMA (read/write) locally without Linked List
> > > feature
> > >
> >
> > I have all of the above four use cases in mind as well. At the moment,
> > only #4 is possible with pcitest.
> >
> > Use case #3 would need a new command line option for pcitest such as -L
> > to let its user specify linked list operationwhen used with dma in
> > conjunction with the existing -D option.
> >
> > Use cases #1 and #2 would need another new command line option such as -R
> > to specify remotely initiated dma operation in conjunction with -D option.
> >
> > New code in pci-epf-test and pci_endpoint_test drivers would be needed
> > to support use cases #1, #2, and #3. However, use case #4 should be
> > possible without modification to pci-epf-test or pci_endpoint_test as long
> > as the dmaengine channels become available on the endpoint side.
>
> I would suggest something like this:
>
> -L option, local DMA triggering
> -R option, remote DMA triggering
> -W <n> option, to select the DMA write channel n => (0 ... 7) to be
> used
> -R <n> option, to select the DMA read channel n => (0 ... 7) to be
> used
> -K option, to use or not the linked list feature (K presence enables
> the LL use)
> -T <n> option, to select which type of DMA transfer to be used => (n = 0
> - scatter-gather mode, 1 - cyclic mode)
> -N <n> option, to define the number of cyclic transfers to perform in
> total
> -C <n> option, to define the size of each chunk to be used
> -t <time> option, to define a timeout for the DMA operation

That looks like a more complete set of command line options.

>
> Also, the use of this options (especially when using the remote DMA
> option) should be checked through the pci_epc_get_features(), which means
> probably we need to pass the EP features capabilities to the
> pci_endpoint_test Driver, perhaps using some sets of registers on located
> on BAR0 or other.

That is a great point. There may be changes required below pci-epf-test
in the endpoint framework stack.

>
> > At the moment, pci-epf-test grabs the first available dma channel on the
> > endpoint side and uses it for either read, write, or copy operation. it is not
> > possible at the moment to specify which dma channel to use on the pcitest
> > command line. This may be possible by modifying the command line option
> > -D to also specify the name of one or more dma channels.
>
> I'm assuming that behavior is due to your code, right? I'm not seen that
> behavior on the Kernel tree.
> Check my previous suggestion, it should be something similar to what is
> been done while you select the MSI/MSI-X interrupt to trigger.

I believe this behavior exists in the kernel tree because the call to
dma_request_chan_by_mask() always specifies channel zero. The user
of pcitest has no way of specifying which one of the available dma channels
to use.

>
> > Also, pci-epf-test grabs the dma channel at bind time and holds on to it
> > until unloaded. This denies the use of the dma channel to others on the
> > endpoint side. However, it seems possible to grab and release the dma
> > channel only for the duration of each read, write, or copy test. These are
> > improvements that can come over time. It is great that pci-epf-test was
> > recently updated to include support for dma operations which makes such
> > improvements possible.
>
> Check my previous suggestion. I think by having a timeout for the DMA
> operation we can provide a way to release the dma channel.
> Or we could provide some kind of heart beat, once again through some
> register in a BAR.

I believe this behavior exists in the kernel tree because the call to
dma_request_chan_by_mask() happens during the execution of
pci_epf_test_bind() and the call to dma_release_channel() happens
during the execution of pci_epf_test_unbind(). As long as pci-epf-test
is bound, I cannot use another program such as dmatest from the
endpoint-side command prompt to exercise the same channel.

What I was suggesting is perhaps pci-epf-test can be modified to
acquire and release the channel on each call to pci_epf_test_read(),
...write(), or ...copy() when the pcitest user specifies -D option.

>
> > > Relative to the implementation of the options 3 and 4, I wonder if the
> > > linked list memory space and size could be set through the DT or by the
> > > configfs available on the pci-epf-test driver.
> > >
> >
> > Although these options could be set through DT or by configfs, another
> > option is to enable the user of pcitest to specify such parameters on
> > the command line when invoking each test from the host side.
>
> That would be an easy and quick solution, but so far as I know there is a
> movement in the Kernel to avoid any configuration through module
> parameters. So I'm afraid that you have to choose by DT or configfs
> strategy. Kishon can help you on this matter, by telling you what he
> prefers.

Thanks for that reminder. I will check before getting too invested in a
specific implementation. Just to clarify, I was suggesting giving the
user of pcitest a way to specify which one of the available dma channel to
use on each invocation of pcitest, not what dma channels are available on
the endpoint side. I assumed the strategy for which dma channels do
become present and available on endpoint would be by DT or configfs.

>
> Regards,
> Gustavo
>
>

  reply	other threads:[~2020-04-15 19:08 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-15  2:07 [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev Alan Mikhak
2020-04-15 12:13 ` Gustavo Pimentel
2020-04-15 17:24   ` Alan Mikhak
2020-04-15 18:17     ` Gustavo Pimentel
2020-04-15 18:55       ` Alan Mikhak [this message]
2020-04-15 20:57         ` Gustavo Pimentel
2020-04-15 21:23           ` Alan Mikhak
2020-04-15 23:26             ` Alan Mikhak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABEDWGw0OyQNppLpDaNgMedfB0Ci=kZVKm+h4T-LJoZYmbSgqA@mail.gmail.com' \
    --to=alan.mikhak@sifive.com \
    --cc=Gustavo.Pimentel@synopsys.com \
    --cc=dan.j.williams@intel.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=kishon@ti.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=paul.walmsley@sifive.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).