From: Sinan Kaya <okaya@codeaurora.org> To: Russell King - ARM Linux <linux@armlinux.org.uk> Cc: Vinod Koul <vinod.koul@intel.com>, linux-arm-msm@vger.kernel.org, timur@codeaurora.org, linux-kernel@vger.kernel.org, Christopher Covington <cov@codeaurora.org>, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Date: Thu, 4 Aug 2016 12:08:39 -0400 [thread overview] Message-ID: <32e4f4c5-81d1-4a84-69af-e1b05d564bd7@codeaurora.org> (raw) In-Reply-To: <20160804153835.GY1041@n2100.armlinux.org.uk> On 8/4/2016 11:38 AM, Russell King - ARM Linux wrote: > On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote: >> On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote: >>> On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote: >>>> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE. >>>> I now understand that tx_success API just returns information that the request >>>> was executed whether the result is error or not. This makes sense now. >>>> >>>> However, if the txn is failure; then we should never call the client callback >>>> since DMA engine cannot provide such feedback to the client without Dave's patch. >>>> You are saying that the calling the callback is optional. >>>> >>>> Then, the callback cannot be optional in the error case for old behavior. >>>> >>>> How does the client know if memcpy executed or not? The client got its callback >>>> and tx_status is also DMA_COMPLETE. >>> >>> If an error occurred, then dma_async_is_tx_complete() is supposed to >>> return DMA_ERROR. It's up to the DMA engine driver to ensure that >>> this happens if it has error detection abilities. >>> >> >> Well, that's not what I'm getting from Vinod and also from the current usage >> in most of the drivers that I looked. >> >> The current drivers implement tx_status as follows. >> >> static enum dma_status xyz_tx_status(struct dma_chan *chan, >> dma_cookie_t cookie, struct dma_tx_state *state) >> { >> ... >> ret = dma_cookie_status(&c->vc.chan, cookie, state); >> if (ret == DMA_COMPLETE) >> return ret; >> ... >> } > I have patiently looked at all dma_cookie_status calls from LXR. I have not seen a single one implementing what you are suggesting. I can't believe that there is no driver that does error checking. Feel free to correct me if you have a good example I could use as a reference. > And... how many of those drivers detect an error occuring in the DMA? > If they don't detect an error during DMA, I'm curious what would you > expect the above to look like. I have this function that returns the real status of the transaction. enum dma_status hidma_ll_status(struct hidma_lldev *llhndl, u32 tre_ch); I can do another look up here. > >> I also made the argument that the driver should not call dma_cookie_complete >> for failure cases and somehow return an error for transactions that failed. > > You can't omit individual dma_cookie_complete() calls like that (I think > you have little understanding how the cookie system works.) > > The cookies consist of continuous pool of numbers. Each transaction > gets allocated a cookie which is incremented from the previous > transaction. Any transaction can be identified as complete or pending > depending on whether the cookie value that it has is earlier or later > than the current completion cookie value. > > What this means is if you try to do this: > > - transaction 1 completes successfully, call dma_cookie_complete() > - transaction 2 completes with an error, omit dma_cookie_complete() > - transaction 3 completes successfully, call dma_cookie_complete() > > The effect at the end of that sequence will be as if transaction 2 had > called dma_cookie_complete() - because the completed cookie value will > now be ahead of transaction 2's cookie. > > So, playing bakes with dma_cookie_complete() really won't work. Please > forget this idea. dma_cookie_complete() merely indicates whether the > transaction completed or is still in progress. It's got nothing to do > with error status. Fair enough. All the cookie business seemed very convoluted to me when I tried to understand how it works. I relied on what other drivers are doing instead. > > What you instead need to do is to find some way to record in your > driver that transaction 2 failed, and when dma_cookie_status() says > that a transaction has DMA_COMPLETE status, you need to look up to > see whether it failed. > I already have this information available. I just need confirmation from Vinod that this is the right way to do it in terms of DMA engine API. The other way is I can feed this information to what Dave just introduced as part of the callback mechanism and not touch this. The main discussion here is that "tx_status does not indicate whether the final transaction is successful or not" whether the driver has the capability to determine error or not. It is an API question not implementation question. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
WARNING: multiple messages have this Message-ID (diff)
From: okaya@codeaurora.org (Sinan Kaya) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Date: Thu, 4 Aug 2016 12:08:39 -0400 [thread overview] Message-ID: <32e4f4c5-81d1-4a84-69af-e1b05d564bd7@codeaurora.org> (raw) In-Reply-To: <20160804153835.GY1041@n2100.armlinux.org.uk> On 8/4/2016 11:38 AM, Russell King - ARM Linux wrote: > On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote: >> On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote: >>> On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote: >>>> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE. >>>> I now understand that tx_success API just returns information that the request >>>> was executed whether the result is error or not. This makes sense now. >>>> >>>> However, if the txn is failure; then we should never call the client callback >>>> since DMA engine cannot provide such feedback to the client without Dave's patch. >>>> You are saying that the calling the callback is optional. >>>> >>>> Then, the callback cannot be optional in the error case for old behavior. >>>> >>>> How does the client know if memcpy executed or not? The client got its callback >>>> and tx_status is also DMA_COMPLETE. >>> >>> If an error occurred, then dma_async_is_tx_complete() is supposed to >>> return DMA_ERROR. It's up to the DMA engine driver to ensure that >>> this happens if it has error detection abilities. >>> >> >> Well, that's not what I'm getting from Vinod and also from the current usage >> in most of the drivers that I looked. >> >> The current drivers implement tx_status as follows. >> >> static enum dma_status xyz_tx_status(struct dma_chan *chan, >> dma_cookie_t cookie, struct dma_tx_state *state) >> { >> ... >> ret = dma_cookie_status(&c->vc.chan, cookie, state); >> if (ret == DMA_COMPLETE) >> return ret; >> ... >> } > I have patiently looked at all dma_cookie_status calls from LXR. I have not seen a single one implementing what you are suggesting. I can't believe that there is no driver that does error checking. Feel free to correct me if you have a good example I could use as a reference. > And... how many of those drivers detect an error occuring in the DMA? > If they don't detect an error during DMA, I'm curious what would you > expect the above to look like. I have this function that returns the real status of the transaction. enum dma_status hidma_ll_status(struct hidma_lldev *llhndl, u32 tre_ch); I can do another look up here. > >> I also made the argument that the driver should not call dma_cookie_complete >> for failure cases and somehow return an error for transactions that failed. > > You can't omit individual dma_cookie_complete() calls like that (I think > you have little understanding how the cookie system works.) > > The cookies consist of continuous pool of numbers. Each transaction > gets allocated a cookie which is incremented from the previous > transaction. Any transaction can be identified as complete or pending > depending on whether the cookie value that it has is earlier or later > than the current completion cookie value. > > What this means is if you try to do this: > > - transaction 1 completes successfully, call dma_cookie_complete() > - transaction 2 completes with an error, omit dma_cookie_complete() > - transaction 3 completes successfully, call dma_cookie_complete() > > The effect at the end of that sequence will be as if transaction 2 had > called dma_cookie_complete() - because the completed cookie value will > now be ahead of transaction 2's cookie. > > So, playing bakes with dma_cookie_complete() really won't work. Please > forget this idea. dma_cookie_complete() merely indicates whether the > transaction completed or is still in progress. It's got nothing to do > with error status. Fair enough. All the cookie business seemed very convoluted to me when I tried to understand how it works. I relied on what other drivers are doing instead. > > What you instead need to do is to find some way to record in your > driver that transaction 2 failed, and when dma_cookie_status() says > that a transaction has DMA_COMPLETE status, you need to look up to > see whether it failed. > I already have this information available. I just need confirmation from Vinod that this is the right way to do it in terms of DMA engine API. The other way is I can feed this information to what Dave just introduced as part of the callback mechanism and not touch this. The main discussion here is that "tx_status does not indicate whether the final transaction is successful or not" whether the driver has the capability to determine error or not. It is an API question not implementation question. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
next prev parent reply other threads:[~2016-08-04 16:08 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-14 2:57 [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Sinan Kaya 2016-07-14 2:57 ` Sinan Kaya 2016-07-16 1:00 ` Sinan Kaya 2016-07-16 1:00 ` Sinan Kaya 2016-07-24 6:24 ` Vinod Koul 2016-07-24 6:24 ` Vinod Koul 2016-07-25 14:19 ` Sinan Kaya 2016-07-25 14:19 ` Sinan Kaya 2016-08-04 12:55 ` Vinod Koul 2016-08-04 12:55 ` Vinod Koul 2016-08-04 12:55 ` Vinod Koul 2016-08-04 14:17 ` Sinan Kaya 2016-08-04 14:17 ` Sinan Kaya 2016-08-04 14:40 ` Russell King - ARM Linux 2016-08-04 14:40 ` Russell King - ARM Linux 2016-08-04 15:27 ` Sinan Kaya 2016-08-04 15:27 ` Sinan Kaya 2016-08-04 15:38 ` Russell King - ARM Linux 2016-08-04 15:38 ` Russell King - ARM Linux 2016-08-04 15:59 ` Lars-Peter Clausen 2016-08-04 15:59 ` Lars-Peter Clausen 2016-08-08 9:08 ` Vinod Koul 2016-08-08 9:08 ` Vinod Koul 2016-08-08 12:25 ` Lars-Peter Clausen 2016-08-08 12:25 ` Lars-Peter Clausen 2016-08-10 17:23 ` Vinod Koul 2016-08-10 17:23 ` Vinod Koul 2016-08-10 17:23 ` Vinod Koul 2016-08-04 16:08 ` Sinan Kaya [this message] 2016-08-04 16:08 ` Sinan Kaya 2016-08-04 16:15 ` Lars-Peter Clausen 2016-08-04 16:15 ` Lars-Peter Clausen 2016-08-05 6:32 ` Robert Jarzmik 2016-08-05 6:32 ` Robert Jarzmik 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 15:17 ` Sinan Kaya 2016-08-05 15:17 ` Sinan Kaya 2016-08-08 9:02 ` Vinod Koul 2016-08-08 9:02 ` Vinod Koul 2016-08-08 14:45 ` Sinan Kaya 2016-08-08 14:45 ` Sinan Kaya 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:31 ` Sinan Kaya 2016-08-10 17:31 ` Sinan Kaya 2016-08-10 17:31 ` Sinan Kaya 2016-08-19 2:48 ` Vinod Koul 2016-08-19 2:48 ` Vinod Koul 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:42 ` Vinod Koul 2016-08-19 3:42 ` Vinod Koul 2016-08-19 3:48 ` Sinan Kaya 2016-08-19 3:48 ` Sinan Kaya 2016-08-19 5:52 ` Vinod Koul 2016-08-19 5:52 ` Vinod Koul 2016-08-19 11:13 ` okaya 2016-08-19 11:13 ` okaya at codeaurora.org 2016-08-19 11:13 ` okaya 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:21 ` Sinan Kaya 2016-08-19 17:21 ` Sinan Kaya 2016-08-19 17:21 ` Sinan Kaya 2016-08-22 6:08 ` Vinod Koul 2016-08-22 6:08 ` Vinod Koul 2016-08-22 6:08 ` Vinod Koul 2016-08-22 13:27 ` Sinan Kaya 2016-08-22 13:27 ` Sinan Kaya 2016-08-22 17:00 ` Vinod Koul 2016-08-22 17:00 ` Vinod Koul 2016-08-08 8:51 ` Vinod Koul 2016-08-08 8:51 ` Vinod Koul 2016-08-08 12:10 ` okaya 2016-08-08 12:10 ` okaya at codeaurora.org
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=32e4f4c5-81d1-4a84-69af-e1b05d564bd7@codeaurora.org \ --to=okaya@codeaurora.org \ --cc=cov@codeaurora.org \ --cc=dmaengine@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=timur@codeaurora.org \ --cc=vinod.koul@intel.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.