From: Peter Ujfalusi <peter.ujfalusi@ti.com>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: vinod.koul@intel.com, dmaengine@vger.kernel.org,
linux-omap@vger.kernel.org, nsekhar@ti.com,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback() from tx_status()
Date: Fri, 26 Feb 2016 14:45:30 +0200 [thread overview]
Message-ID: <56D048EA.2000604@ti.com> (raw)
In-Reply-To: <20160226112524.GP5783@n2100.arm.linux.org.uk>
On 2016-02-26 13:25, Russell King - ARM Linux wrote:
>>> Your original commit adding the original hack that you're now removing
>>> above says that this is to support polled operation: I'm not aware of
>>> DMA engine supporting such a mode. DMA_PREP_INTERRUPT is a mechanism
>>> where requests can be queued without an interrupt to allow batching.
>>
>> Also it is used to suppress DMA interrupts during audio playback for example.
>> In this case we will run w/o interrupts and the position is polled.
>>
>>> See the raid5/async_tx code, which queues a set of operations without
>>> DMA_PREP_INTERRUPT, with the final operation with DMA_PREP_INTERRUPT
>>> set.
>>
>> We only allow the interrupts to be disabled in cyclic or memcpy mode. With
>> slave_sg we have interrupts as it is needed to move to the next SG.
>>
>>> As the driver is reliant on interrupts to move to the next transfer,
>>> the patch which causes DMA_PREP_INTERRUPT to influence whether
>>> interrupts are sent is actually buggy, and will prevent several
>>> queued DMA operations to fail.
>>
>> Yes, the omap-dma only allows the interrupts to be actually disabled when it
>> is save to do so. slave_sg can not work w/o interrupts so there we don't
>> disable them.
>
> I get the impression that you haven't taken in what I've said, because
> each fragment of your reply is just repeating what the previous fragment
> said.
Sorry about that. I got it.
> Let me state that I don't believe you need any hacks here, and this patch
> is not necessary: the final operation in a set of chained memcpy()s should
> have DMA_PREP_INTERRUPT set.
calling the omap_dma_callback() from the tx_status() was a bad idea, this is
why I'm fixing it.
As for the DMA_PREP_INTERRUPT not enabling interrupt was introduced by:
4ce98c0a20bef (dmaengine: omap-dma: Add support for memcpy)
While we do not have users submitting multiple descriptors I agree that this
is a possibility and the driver should not fail in such a case.
I can send a followup patch to fix the omap_dma_prep_dma_memcpy()
--
Péter
WARNING: multiple messages have this Message-ID (diff)
From: peter.ujfalusi@ti.com (Peter Ujfalusi)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback() from tx_status()
Date: Fri, 26 Feb 2016 14:45:30 +0200 [thread overview]
Message-ID: <56D048EA.2000604@ti.com> (raw)
In-Reply-To: <20160226112524.GP5783@n2100.arm.linux.org.uk>
On 2016-02-26 13:25, Russell King - ARM Linux wrote:
>>> Your original commit adding the original hack that you're now removing
>>> above says that this is to support polled operation: I'm not aware of
>>> DMA engine supporting such a mode. DMA_PREP_INTERRUPT is a mechanism
>>> where requests can be queued without an interrupt to allow batching.
>>
>> Also it is used to suppress DMA interrupts during audio playback for example.
>> In this case we will run w/o interrupts and the position is polled.
>>
>>> See the raid5/async_tx code, which queues a set of operations without
>>> DMA_PREP_INTERRUPT, with the final operation with DMA_PREP_INTERRUPT
>>> set.
>>
>> We only allow the interrupts to be disabled in cyclic or memcpy mode. With
>> slave_sg we have interrupts as it is needed to move to the next SG.
>>
>>> As the driver is reliant on interrupts to move to the next transfer,
>>> the patch which causes DMA_PREP_INTERRUPT to influence whether
>>> interrupts are sent is actually buggy, and will prevent several
>>> queued DMA operations to fail.
>>
>> Yes, the omap-dma only allows the interrupts to be actually disabled when it
>> is save to do so. slave_sg can not work w/o interrupts so there we don't
>> disable them.
>
> I get the impression that you haven't taken in what I've said, because
> each fragment of your reply is just repeating what the previous fragment
> said.
Sorry about that. I got it.
> Let me state that I don't believe you need any hacks here, and this patch
> is not necessary: the final operation in a set of chained memcpy()s should
> have DMA_PREP_INTERRUPT set.
calling the omap_dma_callback() from the tx_status() was a bad idea, this is
why I'm fixing it.
As for the DMA_PREP_INTERRUPT not enabling interrupt was introduced by:
4ce98c0a20bef (dmaengine: omap-dma: Add support for memcpy)
While we do not have users submitting multiple descriptors I agree that this
is a possibility and the driver should not fail in such a case.
I can send a followup patch to fix the omap_dma_prep_dma_memcpy()
--
P?ter
WARNING: multiple messages have this Message-ID (diff)
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: <vinod.koul@intel.com>, <dmaengine@vger.kernel.org>,
<linux-omap@vger.kernel.org>, <nsekhar@ti.com>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback() from tx_status()
Date: Fri, 26 Feb 2016 14:45:30 +0200 [thread overview]
Message-ID: <56D048EA.2000604@ti.com> (raw)
In-Reply-To: <20160226112524.GP5783@n2100.arm.linux.org.uk>
On 2016-02-26 13:25, Russell King - ARM Linux wrote:
>>> Your original commit adding the original hack that you're now removing
>>> above says that this is to support polled operation: I'm not aware of
>>> DMA engine supporting such a mode. DMA_PREP_INTERRUPT is a mechanism
>>> where requests can be queued without an interrupt to allow batching.
>>
>> Also it is used to suppress DMA interrupts during audio playback for example.
>> In this case we will run w/o interrupts and the position is polled.
>>
>>> See the raid5/async_tx code, which queues a set of operations without
>>> DMA_PREP_INTERRUPT, with the final operation with DMA_PREP_INTERRUPT
>>> set.
>>
>> We only allow the interrupts to be disabled in cyclic or memcpy mode. With
>> slave_sg we have interrupts as it is needed to move to the next SG.
>>
>>> As the driver is reliant on interrupts to move to the next transfer,
>>> the patch which causes DMA_PREP_INTERRUPT to influence whether
>>> interrupts are sent is actually buggy, and will prevent several
>>> queued DMA operations to fail.
>>
>> Yes, the omap-dma only allows the interrupts to be actually disabled when it
>> is save to do so. slave_sg can not work w/o interrupts so there we don't
>> disable them.
>
> I get the impression that you haven't taken in what I've said, because
> each fragment of your reply is just repeating what the previous fragment
> said.
Sorry about that. I got it.
> Let me state that I don't believe you need any hacks here, and this patch
> is not necessary: the final operation in a set of chained memcpy()s should
> have DMA_PREP_INTERRUPT set.
calling the omap_dma_callback() from the tx_status() was a bad idea, this is
why I'm fixing it.
As for the DMA_PREP_INTERRUPT not enabling interrupt was introduced by:
4ce98c0a20bef (dmaengine: omap-dma: Add support for memcpy)
While we do not have users submitting multiple descriptors I agree that this
is a possibility and the driver should not fail in such a case.
I can send a followup patch to fix the omap_dma_prep_dma_memcpy()
--
Péter
next prev parent reply other threads:[~2016-02-26 12:45 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-25 8:28 [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback() from tx_status() Peter Ujfalusi
2016-02-25 8:28 ` Peter Ujfalusi
2016-02-25 8:28 ` Peter Ujfalusi
2016-02-26 1:06 ` Russell King - ARM Linux
2016-02-26 1:06 ` Russell King - ARM Linux
2016-02-26 10:23 ` Peter Ujfalusi
2016-02-26 10:23 ` Peter Ujfalusi
2016-02-26 10:23 ` Peter Ujfalusi
2016-02-26 11:25 ` Russell King - ARM Linux
2016-02-26 11:25 ` Russell King - ARM Linux
2016-02-26 12:45 ` Peter Ujfalusi [this message]
2016-02-26 12:45 ` Peter Ujfalusi
2016-02-26 12:45 ` Peter Ujfalusi
2016-02-26 13:53 ` Peter Ujfalusi
2016-02-26 13:53 ` Peter Ujfalusi
2016-02-26 13:53 ` Peter Ujfalusi
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=56D048EA.2000604@ti.com \
--to=peter.ujfalusi@ti.com \
--cc=dmaengine@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=nsekhar@ti.com \
--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: link
Be 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.