Re: [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev

[Alan Mikhak]

On Wed, Apr 15, 2020 at 1:58 PM Gustavo Pimentel
<Gustavo.Pimentel@xxxxxxxxxxxx> wrote:
>
> Hi Alan,
>
> > > > 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.
>
> I think we were discussing different things. I was referring to the
> pci-epf-test code, that I wasn't being able to find any instruction to
> call the DMA driver which had the described behavior.
>
> I think you can do it by doing this:
>
> Pseudo code:
>
> #define EDMA_TEST_CHANNEL_NAME                  "dma%uchan%u"
>
> static bool dw_edma_test_filter(struct dma_chan *chan, void *filter)
> {
>         if (strcmp(dev_name(chan->device->dev), EDMA_TEST_DEVICE_NAME) ||
> strcmp(dma_chan_name(chan), filter))
>                 return false;
>
>         return true;
> }
>
> static void dw_edma_test_thread_create(int id, int channel)
> {
>         struct dma_chan *chan;
>         dma_cap_mask_t mask;
>         char filter[20];
>
>         dma_cap_zero(mask);
>         dma_cap_set(DMA_SLAVE, mask);
>         dma_cap_set(DMA_CYCLIC, mask);
>
>         snprintf(filter, sizeof(filter), EDMA_TEST_CHANNEL_NAME, id,
> channel);
>         chan = dma_request_channel(mask, dw_edma_test_filter, filter);
>
>         [..]
> }

Thanks Gustavo, This pseudo code is very useful. Now I know how to do
that part of the change.

What I have further in mind is to enable the pcitest user to specify some
arbitrary string with -D option to select one or more of the dma channels
that are available on the endpoint side. Since the user executes pcitest
from host-side command prompt and pci-epf-test executes in kernel on the
endpoint side, the messaging between userspace pcitest and kernel-space
pci_endpoint_test as well as the messaging across the bus between
pci_endpoint_test and pci-epf-test needs to be expanded to pass the user
string from the host to the endpoint. Upon receiving each read, write, or
copy message, pci-epf-test could then try to acquire the specified dma
channel and execute the user command or fail it if no such channel is
available at that moment.

>
> > 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.
>
> Ok, I understood it now. Right, you can't use the dmatest here, even
> because, as far as I know, it is only MEM TO MEM operations and we need
> DEVICE_TO_MEM and vice-versa.
>
> >
> > 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.
>
> Right, you are on the right track.
> Perhaps you could take a look at patch [1] that I have done some time ago
> for testing the eDMA, I think you have all the tools/guideline there to
> do this adaption.
> Another thing,
>
> [1] https://patchwork.kernel.org/patch/10760521/

Thanks for the guidance and reference code patch [1]. I will definitely
take a close look at [1].

>
>
>