devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
To: Vinod Koul <vinod.koul-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: "dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org"
	<dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	"grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org"
	<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>,
	"rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org"
	<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org"
	<devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>,
	"linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH V2 2/2] dmaengine: tegra: add dma driver
Date: Thu, 10 May 2012 10:35:01 +0530	[thread overview]
Message-ID: <4FAB4C7D.7030406@nvidia.com> (raw)
In-Reply-To: <1336620844.1540.269.camel@vkoul-udesk3>

On Thursday 10 May 2012 09:04 AM, Vinod Koul wrote:
> On Wed, 2012-05-09 at 16:31 +0530, Laxman Dewangan wrote:
>> Thanks Vinod for reviewing code.
>> I am removing the code from this thread which  is not require. Only
>> keeping code surrounding our discussion.
>>
>> On Wednesday 09 May 2012 03:44 PM, Vinod Koul wrote:
>>> On Thu, 2012-05-03 at 13:11 +0530, Laxman Dewangan wrote:
>>>> + * Initial number of descriptors to allocate for each channel during
>>>> + * allocation. More descriptors will be allocated dynamically if
>>>> + * client needs it.
>>>> + */
>>>> +#define DMA_NR_DESCS_PER_CHANNEL     4
>>>> +#define DMA_NR_REQ_PER_DESC          8
>>> all of these should be namespaced.
>>> APB and AHB are fairly generic names.
>> Fine, then I will go as APB_DMA_NR_DESC_xxx and APB_DMA_NR_REQ_xxx
> NO. Many ppl use APB/AHB, so pls don't use or namespace them properly

OK, then will say TEGRA_DMA_*** to namemspaced to Tegra domain.

>>>> +
>>>> +enum dma_transfer_mode {
>>>> +     DMA_MODE_NONE,
>>>> +     DMA_MODE_ONCE,
>>>> +     DMA_MODE_CYCLE,
>>>> +     DMA_MODE_CYCLE_HALF_NOTIFY,
>>>> +};
>>> I dont understand why you would need to keep track of these?
>>> You get a request for DMA. You have properties of transfer. You prepare
>>> you descriptors and then submit.
>>> Why would you need to create above modes and remember them?
>> I am allowing multiple desc requests in dma through prep_slave and
>> prep_cyclic. So when dma channel does not have any request then it can
>> set its mode as NONE.
> Again NO.
>
> This is not how dmaengine API is supposed to work.
> Correct behavior would be:
> You prepare descriptors, as many as you want.
> You submit them. Dmaengine driver takes them and pushes them to pending
> list. Submit does not start.
> When .issue_pending is called, you start DMA by using first descriptor
> in pending list. One completed you start next one untill you exhaust the
> complete list.
> So use your pending list for this.

Tegra dma engine support three type of mode, one shot, cyclic once and 
cyclic double. The next transfer configuration is different in all these 
modes, in oneshot, next request should be configure only after transfer 
completes, in cyclic once, the next request should be configure before 
current transfer completes so that hw can auto switch to next transfer 
and in cyclic_double, the next request should be configure only after 
half transfer.

For handling this and not complicating the code, I have separate isr 
handler on these cases.

Now if any request come which can cause the switching of modes, like one 
request for one_shot and next request for the cyclic_once then it will 
be very complex to handle such situation as the configuration of request 
depends on mode. I am simply trying to avoid that situation and not 
allowing client to request which can cause conflict in engine.


>> Once the desc is requested the mode is set based on request.
> Again NO
>> This mode
>> will not get change until all dma request done and if new request come
>> to dma channel, it checks that it should not conflict with older mode.
>> The engine is configured in these mode and will change only when all
>> transfer completed.
> See above you absolutely dont need to track *modes*
>
>> We are allocating memory also and that's why it is there.
>> But I can make it because I am callocating memory as GFP_ATOMIC.
>> However if the function call dma_async_tx_descriptor_init() can happen
>> in atomic context i.e. calling spin_lock_init() call.
>>
>>>> +                     }
>>>> +                     dma_cookie_complete(&dma_desc->txd);
>>> does this make sense. You are marking an aborted descriptor as complete.
>> If I dont call the the complete cookie of that channel will not get
>> updated and on query, it will return as PROGRESS.
>> I need to update the dma channel completed cookie.
> No the transaction failed as it was aborted. So mark it as DMA_ERROR.
>

But when we update the chan->completed_cookie for aborted cookie?
Otherwise I will get the DMA_IN_PROGRSS when I call dma_cookie_status() 
but actually it is aborted.
By calling complete(), I will get DMA_SUCCESS and desc->status can tell 
the error or not.

>>>> +     /* Call callbacks if was pending before aborting requests */
>>>> +     while (!list_empty(&cb_dma_desc_list)) {
>>>> +             dma_desc  = list_first_entry(&cb_dma_desc_list,
>>>> +                             typeof(*dma_desc), cb_node);
>>>> +             list_del(&dma_desc->cb_node);
>>>> +             callback = dma_desc->txd.callback;
>>>> +             callback_param = dma_desc->txd.callback_param;
>>> again, callback would be called from tasklet, so why would it need to be
>>> called from here , and why would this be pending.
>> What happen if some callbacks are pending as tasklet does not get
>> scheduled and meantime, the dma terminated (specially in multi-core system)?
>> Should we ignore all callbacks in this case?
> In termination case, client has already terminated and possibly in
> process of cleanup.
> So makes no sense in those cases to call the callback.
OK, fine with me.

  reply	other threads:[~2012-05-10  5:05 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-05-03  7:41 [PATCH V2 0/2] dmaengine: tegra: add dma driver Laxman Dewangan
     [not found] ` <1336030889-32269-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-05-03  7:41   ` [PATCH V2 1/2] dma: dmaengine: add slave req id in slave_config Laxman Dewangan
2012-05-09  8:49     ` Vinod Koul
2012-05-09 10:27       ` Laxman Dewangan
2012-05-03  7:41 ` [PATCH V2 2/2] dmaengine: tegra: add dma driver Laxman Dewangan
     [not found]   ` <1336030889-32269-3-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-05-09 10:14     ` Vinod Koul
2012-05-09 11:01       ` Laxman Dewangan
     [not found]         ` <4FAA4EA3.70001-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-05-10  3:34           ` Vinod Koul
2012-05-10  5:05             ` Laxman Dewangan [this message]
     [not found]               ` <4FAB4C7D.7030406-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-05-11  8:41                 ` Vinod Koul
2012-05-11 10:58                   ` Laxman Dewangan

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=4FAB4C7D.7030406@nvidia.com \
    --to=ldewangan-ddmlm1+adcrqt0dzr+alfa@public.gmane.org \
    --cc=dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org \
    --cc=grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org \
    --cc=vinod.koul-VuQAYsv1563Yd54FQh9/CA@public.gmane.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).