Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630630 > unrolled thread

Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2017-04-25 17:20 +0200
    Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-04-25 20:20 +0200
      Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-04-26 17:30 +0200
        Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2017-04-27 15:30 +0200
          Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-04-27 15:40 +0200
      Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2017-04-27 17:40 +0200

#1630630 — Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver

On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:
> > Hi,
> > On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:
> > > On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:
> > > > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:
> > > > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:
> > > > > > This patch adds support for the DW AXI DMAC controller.
> > > > > > +static inline void
> > > > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32
> > > > > > val)
> > > > > > +{
> > > > > > +	iowrite32(val, chip->regs + reg);
> > > > >
> > > > > Are you going to use IO ports for this IP? I don't think so.
> > > > > Wouldn't be better to call readl()/writel() instead?
> > > >
> > > > As I understand, it's better to use ioread/iowrite as more
> > > > universal
> > > > IO
> > > > access way. Am I wrong?
> > >
> > > As I said above the ioreadX/iowriteX makes only sense when your
> > > IP
> > > would be accessed via IO region or MMIO. I'm pretty sure IO is
> > > not
> > > the case at all for this IP.
> >
> > MMIO? This IP works exactly via memory-mapped I/O.
>
> Yes, and why do you need to check this on each IO read/write?
> Please, switch to plain readX()/writeX() instead.
Ok, I'll switch to readX()/writeX().

> > > > > > +		val = axi_chan_ioread32(chan,
> > > > > > CH_INTSTATUS_ENA);
> > > > > > +		val &= ~irq_mask;
> > > > > > +		axi_chan_iowrite32(chan, CH_INTSTATUS_ENA,
> > > > > > val);
> > > > > > +	}
> > > > > > +
> > > > > > +	return min_t(size_t, __ffs(sdl), max_width);
> > > > > > +}
> > > > > > +static void axi_desc_put(struct axi_dma_desc *desc)
> > > > > > +{
> > > > > > +	struct axi_dma_chan *chan = desc->chan;
> > > > > > +	struct dw_axi_dma *dw = chan->chip->dw;
> > > > > > +	struct axi_dma_desc *child, *_next;
> > > > > > +	unsigned int descs_put = 0;
> > > > > > +	list_for_each_entry_safe(child, _next, &desc-
> > > > > > > xfer_list,
> > > > > >
> > > > > > xfer_list) {
> > > > >
> > > > > xfer_list looks redundant.
> > > > > Can you elaborate why virtual channel management is not
> > > > > working
> > > > > for
> > > > > you?
> > > >
> > > > Each virtual descriptor encapsulates several hardware
> > > > descriptors,
> > > > which belong to same transfer.
> > > > This list (xfer_list) is used only for allocating/freeing these
> > > > descriptors and it doesn't affect on virtual dma work logic.
> > > > I can see this approach in several drivers with VirtDMA (but
> > > > they
> > > > mostly use array instead of list)
> > >
> > > You described how most of the DMA drivers are implemented, though
> > > they
> > > are using just sg_list directly. I would recommend to do the same
> > > and
> > > get rid of this list.
> >
> > This IP can be (ans is) configured with small block size.
> > (note, that I am not saying about runtime HW configuration)
> >
> > And there is opportunity what we can't use sg_list directly and
> > need
> > to
> > split sg_list to a smaller chunks.
>
> That's what I have referred quite ago. The driver should provide an
> interface to tell potential caller what maximum block (number of
> items
> with given bus width) it supports.
>
> We have struct dma_parms in struct device, but what we actually need
> is
> to support similar on per channel basis in DMAengine framework.
>
> So, instead of working around this I recommend either to implement it
> properly or rely on the fact that in the future someone eventually
> does that for you.
>
> Each driver which has this re-splitting mechanism should be cleaned
> up and refactored.
I still can't see any pros of this implementation.
There is no performance profit: we anyway need to re-splitt sg_list
(but now in dma-user driver instead of dma driver)

If we want to use same descriptors several times we just can use
DMA_CTRL_REUSE option - the descriptors will be created one time and
re-splitting will be сompleted only one time.

But there are cons of this implementation:
we need to implement re-splitting mechanism in each place we use dma
instead of one dma driver. So there are more places for bugs and etc...

> > > > > Btw, are you planning to use priority at all? For now on I
> > > > > didn't
> > > > > see
> > > > > a single driver (from the set I have checked, like 4-5 of
> > > > > them)
> > > > > that
> > > > > uses priority anyhow. It makes driver more complex for
> > > > > nothing.
> > > >
> > > > Only for dma slave operations.
> > >
> > > So, in other words you *have* an actual two or more users that
> > > *need*
> > > prioritization?
> >
> > As I remember there was an idea to give higher priority to audio
> > dma
> > chanels.
>
> I don't see cyclic transfers support in the driver. So, I would
> suggest
> just drop entire prioritization for now. When it would be actual user
> one may start thinking of it.
> Just a rule of common sense: do not implement something which will
> have no user or solve non-existing problem.

Ok, I'll drop prioritization untill I implement cyclic transfers.

> > > > > As I said earlier dw_dmac is *bad* example of the (virtual
> > > > > channel
> > > > > based) DMA driver.
> > > > >
> > > > > I guess you may just fail the descriptor and don't pretend it
> > > > > has
> > > > > been processed successfully.
> > > >
> > > > What do you mean by saying "fail the descriptor"?
> > > > After I get error I cancel current transfer and free all
> > > > descriptors
> > > > from it (by calling vchan_cookie_complete).
> > > > I can't store error status in descriptor structure because it
> > > > will
> > > > be
> > > > freed by vchan_cookie_complete.
> > > > I can't store error status in channel structure because it will
> > > > be
> > > > overwritten by next transfer.
> > >
> > > Better not to pretend that it has been processed successfully.
> > > Don't
> > > call callback on it and set its status to DMA_ERROR (that's why
> > > descriptors in many drivers have dma_status field). When user
> > > asks for
> > > status (using cookie) the saved value would be returned until
> > > descriptor
> > > is active.
> > >
> > > Do you have some other workflow in mind?
> >
> > Hmm...
> > Do you mean I should left error descriptors in desc_issued list
> > or I should create another list (like desc_error) in my driver and
> > move
> > error descriptors to desc_error list?
> >
> > And when exactly should I free error descriptors?
> See below.
>
> > I checked hsu/hsu.c dma driver implementation:
> >   vdma descriptor is deleted from desc_issued list when transfer
> >   starts. When descriptor marked as error descriptor
> >   vchan_cookie_complete isn't called for this descriptor. And this
> >   descriptor isn't placed in any list. So error descriptors *never*
> >   will be freed.
> >   I don't actually like this approach.
>
> Descriptor is active until terminate_all() is called or new
> descriptor
> is supplied. So, the caller has a quite time to check on it.
>
> So, what's wrong on it by your opinion?

Hmm, this looks OK. (In my example (hsu/hsu.c driver) error descriptors
are not freed even after terminate_all is called)

> Of course, if you want to keep by some reason (should be stated what
> the reason in comment) erred descriptors, you can do that.

So, I'll create desc_error list and store failed descriptors in this
list until terminate_all() is called.
Is it OK implementation?

> > > > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> > > > > > +	SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend,
> > > > > > axi_dma_runtime_resume, NULL)
> > > > > > +};
> > > > >
> > > > > Have you tried to build with CONFIG_PM disabled?
> > > >
> > > > Yes.
> > > >
> > > > > I'm pretty sure you need __maybe_unused applied to your PM
> > > > > ops.
> > > >
> > > > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I
> > > > dont't
> > > > use PM.
> > > > (I call them in probe / remove function.)
> > >
> > > Hmm... I didn't check your ->probe() and ->remove(). Do you mean
> > > you
> > > call them explicitly by those names?
> > >
> > > If so, please don't do that. Use pm_runtime_*() instead. And...
> > >
> > > > So I don't need to declare them with __maybe_unused.
> > >
> > > ...in that case it's possible you have them defined but not used.
> > >
> >
> > From my ->probe() function:
> >
> > pm_runtime_get_noresume(chip->dev);
> > ret = axi_dma_runtime_resume(chip->dev);
> >
> > Firstly I only incrememt counter.
> > Secondly explicitly call my resume function.
> >
> > I call them explicitly because I need driver to work also without
> > Runtime PM. So I can't just call pm_runtime_get here instead of
> > pm_runtime_get_noresume + axi_dma_runtime_resume.
> >
> > Of course I can copy *all* code from axi_dma_runtime_resume
> > to ->probe() function, but I don't really like this idea.
>
> It looks like you need more time to investigate how runtime PM works
> from driver point of view, but you shouldn't call your PM callbacks
> directly without a really good reason (weird silicon bugs,
> architectural impediments). I don't think that is the case here.
>
There is a simple reason:
I had to do same actions in probe/remove as in
runtime_resume/runtime_suspend.
(like enabling clock, enabling dma)

If my driver is build with RUNTIME_PM this actions will be
automatically handled by runtime pm (clock and dma will be enabled
before using and disabled after using).
Otherwise, if my driver is build without RUNTIME_PM clock and dma will
be enabled only one time - when ->probe() is called.
So I use runtime_resume callback directly for this purpose (because if
my driver is build without RUNTIME_PM this callback wiil not be called
at all)

> > > > > > +	bool		is_paused;
> > > > >
> > > > > I still didn't get (already forgot) why you can't use
> > > > > dma_status
> > > > > instead for the active descriptor?
> > > >
> > > > As I said before, I checked several driver, which have status
> > > > variable
> > > > in their channel structure - it is used *only* for
> > > > determinating
> > > > is
> > > > channel paused or not. So there is no much sense in replacing
> > > > "is_paused" to "status" and I left "is_paused" variable
> > > > untouched.
> > >
> > > Not only (see above), the errored descriptor keeps that status.
> > >
> > > > (I described above why we can't use status in channel structure
> > > > for
> > > > error handling)
> > >
> > > Ah, I'm talking about descriptor.
> >
> > Again - PAUSED is per-channel flag. So, even if we have status
> > field
> > in
> > each descriptor, it is simpler to use one per-channel flag instead
> > of
> > plenty per-descriptor flags.
> > When we pausing/resuming dma channel it is simpler to set only one
> > flag
> > instead of writing DMA_PAUSED to *each* descriptor status field.
>
> What do you mean by "each"? I don't recall the driver which can
> handle
> more than one *active* descriptor per channel. Do you?
>
> In that case status of active descriptor == status of channel. That
> trick (I also already referred to earlier) is used in some drivers.

Ok, I'll recheck others implementation.

> > > I mean, who are the users of them? If it's only one module, there
> > > is
> > > no need to put them in header.
> >
> > Yes, only one module.
> > Should I move all this definitions to axi_dma_platform.c file and
> > rid
> > of both axi_dma_platform_reg.h and axi_dma_platform.h headers?
> Depends on your design.
>
> If it would be just one C module it might make sense to use driver.c
> and driver.h or just driver.c.
> I see several drivers in current linux-next that are using latter and
> some that are using former, and number of plain driver.c variant is
> bigger.

Ok.

--
 Eugeniy Paltsev

[toc] | [next] | [standalone]

#1630847

On Tue, 2017-04-25 at 15:16 +0000, Eugeniy Paltsev wrote:
> On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> > On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:
> > > Hi,
> > > On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:
> > > > On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:
> > > > > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:
> > > > > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:

> > > This IP can be (ans is) configured with small block size.
> > > (note, that I am not saying about runtime HW configuration)
> > > 
> > > And there is opportunity what we can't use sg_list directly and
> > > need
> > > to
> > > split sg_list to a smaller chunks.
> > 
> > That's what I have referred quite ago. The driver should provide an
> > interface to tell potential caller what maximum block (number of
> > items
> > with given bus width) it supports.
> > 
> > We have struct dma_parms in struct device, but what we actually need
> > is
> > to support similar on per channel basis in DMAengine framework.
> > 
> > So, instead of working around this I recommend either to implement
> > it
> > properly or rely on the fact that in the future someone eventually
> > does that for you.
> > 
> > Each driver which has this re-splitting mechanism should be cleaned
> > up and refactored.
> 
> I still can't see any pros of this implementation.
> There is no performance profit: we anyway need to re-splitt sg_list
> (but now in dma-user driver instead of dma driver)

There is, you seems don't see it.

Currently:
 User:
  prepares sg-list

 DMA driver:
  a) iterates over it, and
  b) if sg_len is bigger than block it re-splits it.

New approach:

 User:
  a) gets DMA channel and its properties
  b) prepares sg-list taking into consideration block size

 DMA driver:
  a) uses the given sg-list and for sake of simplicity bails out if
something wrong with it

So, it means in some cases (where entry is big enough) we have to
prepare list *and* re-split it. It's really performance consuming (like
UART case where buffer is small enough and latency like above matters).

> If we want to use same descriptors several times we just can use
> DMA_CTRL_REUSE option - the descriptors will be created one time and
> re-splitting will be сompleted only one time.

There are two type of descriptors: SW and HW. That flag about SW
descriptor, so, it in most cases has nothing to do with the actual entry
size.

> But there are cons of this implementation:
> we need to implement re-splitting mechanism in each place we use dma
> instead of one dma driver. So there are more places for bugs and
> etc...

No, you completely missed the point.

With new approach we do the preparation only once per descriptor /
transfer and in one place where the sg-list is created.

> > > > Better not to pretend that it has been processed successfully.
> > > > Don't
> > > > call callback on it and set its status to DMA_ERROR (that's why
> > > > descriptors in many drivers have dma_status field). When user
> > > > asks for
> > > > status (using cookie) the saved value would be returned until
> > > > descriptor
> > > > is active.
> > > > 
> > > > Do you have some other workflow in mind?
> > > 
> > > Hmm...
> > > Do you mean I should left error descriptors in desc_issued list
> > > or I should create another list (like desc_error) in my driver and
> > > move
> > > error descriptors to desc_error list?
> > > 
> > > And when exactly should I free error descriptors?
> > 
> > See below.
> > 
> > > I checked hsu/hsu.c dma driver implementation:
> > >    vdma descriptor is deleted from desc_issued list when transfer
> > >    starts. When descriptor marked as error descriptor
> > >    vchan_cookie_complete isn't called for this descriptor. And
> > > this
> > >    descriptor isn't placed in any list. So error descriptors
> > > *never*
> > >    will be freed.
> > >    I don't actually like this approach.
> > 
> > Descriptor is active until terminate_all() is called or new
> > descriptor
> > is supplied. So, the caller has a quite time to check on it.
> > 
> > So, what's wrong on it by your opinion?
> 
> Hmm, this looks OK. (In my example (hsu/hsu.c driver) error
> descriptors
> are not freed even after terminate_all is called)

If it's active it will be freed.
Otherwise caller should check somewhere that descriptor fails.

But actually this is fragile and we need to monitor failed descriptors.
Thanks for reporting.

> 
> > Of course, if you want to keep by some reason (should be stated what
> > the reason in comment) erred descriptors, you can do that.
> 
> So, I'll create desc_error list and store failed descriptors in this
> list until terminate_all() is called.
> Is it OK implementation?

Nope, we need to amend virt-chan API for that. I'm on it. Will send a
series soon.

> There is a simple reason:
> I had to do same actions in probe/remove as in
> runtime_resume/runtime_suspend.
> (like enabling clock, enabling dma)

> If my driver is build with RUNTIME_PM this actions will be
> automatically handled by runtime pm (clock and dma will be enabled
> before using and disabled after using).
> Otherwise, if my driver is build without RUNTIME_PM clock and dma will
> be enabled only one time - when ->probe() is called.
> So I use runtime_resume callback directly for this purpose (because if
> my driver is build without RUNTIME_PM this callback wiil not be called
> at all)

Okay, The best way to do that is to create functions which take struct
chip and do that. Re-use them in runtime PM hooks.

But you still need to learn a bit about runtime PM. There is no harm to
call clk_enable() twice in a raw if you guarantee you do the opposite
(twice call clk_disable()) on tear down.

Also I would reconsider clock (un)preparation in those hooks. Why do you
need that part? It's resource consuming.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1631527

On Tue, 2017-04-25 at 21:12 +0300, Andy Shevchenko wrote:
> On Tue, 2017-04-25 at 15:16 +0000, Eugeniy Paltsev wrote:
> > On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:

> > > Descriptor is active until terminate_all() is called or new
> > > descriptor
> > > is supplied. So, the caller has a quite time to check on it.
> > > 
> > > So, what's wrong on it by your opinion?
> > 
> > Hmm, this looks OK. (In my example (hsu/hsu.c driver) error
> > descriptors
> > are not freed even after terminate_all is called)
> 
> If it's active it will be freed.
> Otherwise caller should check somewhere that descriptor fails.
> 
> But actually this is fragile and we need to monitor failed
> descriptors.
> Thanks for reporting.
> 
> > 
> > > Of course, if you want to keep by some reason (should be stated
> > > what
> > > the reason in comment) erred descriptors, you can do that.
> > 
> > So, I'll create desc_error list and store failed descriptors in this
> > list until terminate_all() is called.
> > Is it OK implementation?
> 
> Nope, we need to amend virt-chan API for that. I'm on it. Will send a
> series soon.

I have to correct what I wrote before.

We have two options:
a) one I proposed above;
b) move descriptor to complete list and call complete callback with
result.

So, it looks like the b) variant is what is done already in 4 (did I
calculate correctly?) drivers and respective users.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1632098

On Wed, 2017-04-26 at 18:04 +0300, Andy Shevchenko wrote:
> On Tue, 2017-04-25 at 21:12 +0300, Andy Shevchenko wrote:
> > On Tue, 2017-04-25 at 15:16 +0000, Eugeniy Paltsev wrote:
> > > On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> > > > On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:
> > > > Descriptor is active until terminate_all() is called or new
> > > > descriptor
> > > > is supplied. So, the caller has a quite time to check on it.
> > > >
> > > > So, what's wrong on it by your opinion?
> > >
> > > Hmm, this looks OK. (In my example (hsu/hsu.c driver) error
> > > descriptors
> > > are not freed even after terminate_all is called)
> >
> > If it's active it will be freed.
> > Otherwise caller should check somewhere that descriptor fails.
> >
> > But actually this is fragile and we need to monitor failed
> > descriptors.
> > Thanks for reporting.
> >
> > >
> > > > Of course, if you want to keep by some reason (should be stated
> > > > what
> > > > the reason in comment) erred descriptors, you can do that.
> > >
> > > So, I'll create desc_error list and store failed descriptors in
> > > this
> > > list until terminate_all() is called.
> > > Is it OK implementation?
> >
> > Nope, we need to amend virt-chan API for that. I'm on it. Will send
> > a series soon.
>
> I have to correct what I wrote before.
>
> We have two options:
> a) one I proposed above;
> b) move descriptor to complete list and call complete callback with
> result.
>
> So, it looks like the b) variant is what is done already in 4 (did I
> calculate correctly?) drivers and respective users.

In my opinion we should call error descriptor complete callback.
But I don't think we should move error descriptor to desc_completed
list.

When descriptor following our error descriptor will be completed
successfully vchan tasklet will be called.
So all descriptors from desc_completed list will be freed (including
our error descriptor)
We'll lost information about error descriptors and we'll not be able to
return correct status from dma_async_is_tx_complete().

In my opinion, we should create desc_error list.
When we get error we'll move descriptor to desc_error list and call
complete callback.

--
 Eugeniy Paltsev

[toc] | [prev] | [next] | [standalone]

#1632103

On Thu, 2017-04-27 at 13:21 +0000, Eugeniy Paltsev wrote:
> On Wed, 2017-04-26 at 18:04 +0300, Andy Shevchenko wrote:
> > On Tue, 2017-04-25 at 21:12 +0300, Andy Shevchenko wrote:
> > > On Tue, 2017-04-25 at 15:16 +0000, Eugeniy Paltsev wrote:
> > > > On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> > > > > On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:

> > > > Hmm, this looks OK. (In my example (hsu/hsu.c driver) error
> > > > descriptors
> > > > are not freed even after terminate_all is called)
> > > 
> > > If it's active it will be freed.
> > > Otherwise caller should check somewhere that descriptor fails.
> > > 
> > > But actually this is fragile and we need to monitor failed
> > > descriptors.
> > > Thanks for reporting.
> > > 
> > > > 
> > > > > Of course, if you want to keep by some reason (should be
> > > > > stated
> > > > > what
> > > > > the reason in comment) erred descriptors, you can do that.
> > > > 
> > > > So, I'll create desc_error list and store failed descriptors in
> > > > this
> > > > list until terminate_all() is called.
> > > > Is it OK implementation?
> > > 
> > > Nope, we need to amend virt-chan API for that. I'm on it. Will
> > > send
> > > a series soon.
> > 
> > I have to correct what I wrote before.
> > 
> > We have two options:
> > a) one I proposed above;
> > b) move descriptor to complete list and call complete callback with
> > result.
> > 
> > So, it looks like the b) variant is what is done already in 4 (did I
> > calculate correctly?) drivers and respective users.
> 
> In my opinion we should call error descriptor complete callback.
> But I don't think we should move error descriptor to desc_completed
> list.
> 
> When descriptor following our error descriptor will be completed
> successfully vchan tasklet will be called.
> So all descriptors from desc_completed list will be freed (including
> our error descriptor)
> We'll lost information about error descriptors and we'll not be able
> to
> return correct status from dma_async_is_tx_complete().

While I more agree on the point that we better to have explicit list of
failed descriptors, here is another point that user (which is interested
in error checking) has to provide callback_result instead of callback.

> In my opinion, we should create desc_error list.
> When we get error we'll move descriptor to desc_error list and call
> complete callback.

Vinod, Lars, others, what is(are) your opinion(s)?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1632189

On Tue, 2017-04-25 at 21:12 +0300, Andy Shevchenko wrote:
> On Tue, 2017-04-25 at 15:16 +0000, Eugeniy Paltsev wrote:
> > On Mon, 2017-04-24 at 19:56 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-04-24 at 15:55 +0000, Eugeniy Paltsev wrote:
> > > > Hi,
> > > > On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:
> > > > > On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:
> > > > > > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:
> > > > This IP can be (ans is) configured with small block size.
> > > > (note, that I am not saying about runtime HW configuration)
> > > >
> > > > And there is opportunity what we can't use sg_list directly and
> > > > need
> > > > to
> > > > split sg_list to a smaller chunks.
> > >
> > > That's what I have referred quite ago. The driver should provide
> > > an
> > > interface to tell potential caller what maximum block (number of
> > > items
> > > with given bus width) it supports.
> > >
> > > We have struct dma_parms in struct device, but what we actually
> > > need
> > > is
> > > to support similar on per channel basis in DMAengine framework.
> > >
> > > So, instead of working around this I recommend either to
> > > implement
> > > it
> > > properly or rely on the fact that in the future someone
> > > eventually
> > > does that for you.
> > >
> > > Each driver which has this re-splitting mechanism should be
> > > cleaned
> > > up and refactored.
> >
> > I still can't see any pros of this implementation.
> > There is no performance profit: we anyway need to re-splitt sg_list
> > (but now in dma-user driver instead of dma driver)
--->---
> > If we want to use same descriptors several times we just can use
> > DMA_CTRL_REUSE option - the descriptors will be created one time
> > and re-splitting will be сompleted only one time.
>
> There are two type of descriptors: SW and HW. That flag about SW
> descriptor, so, it in most cases has nothing to do with the actual
> entry size.

Hmm, I checked all virt-dma code attentively and I don't see this
limitation.
The only existing limitation I can see is that we can use
DMA_CTRL_REUSE only for channels supporting slave transfers. (But it is
irrelevant to our discussion)

So, we can use DMA_CTRL_REUSE for both HW/SW descriptor types.

--
 Eugeniy Paltsev

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web