| Message ID | 1465007351-5559-1-git-send-email-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote: > The tx_status hook is supposed to be safe to call from interrupt > context, but it wouldn't ever return completion for the last transfer, > meaning you couldn't poll for DMA completion with interrupts masked. and why is that? > This fixes IRQ handling for bcm2835's DSI1, which requires using the > DMA engine to write its registers due to a bug in the AXI bridge. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index 6149b27c33ad..320461c578e3 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, > struct virt_dma_desc *vd; > enum dma_status ret; > unsigned long flags; > + u32 residue; > > ret = dma_cookie_status(chan, cookie, txstate); > - if (ret == DMA_COMPLETE || !txstate) > + if (ret == DMA_COMPLETE) Why do you change this? txstate can be NULL, so no point calculating reside for those cases > return ret; > > spin_lock_irqsave(&c->vc.lock, flags); > vd = vchan_find_desc(&c->vc, cookie); > if (vd) { > - txstate->residue = > - bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx)); > + residue = bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx)); > } else if (c->desc && c->desc->vd.tx.cookie == cookie) { > struct bcm2835_desc *d = c->desc; > dma_addr_t pos; > @@ -591,11 +591,25 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, > else > pos = 0; > > - txstate->residue = bcm2835_dma_desc_size_pos(d, pos); > + residue = bcm2835_dma_desc_size_pos(d, pos); > + > + /* > + * If our non-cyclic transfer is done, then report > + * complete and trigger the next tx now. This lets > + * the dmaengine API be used synchronously from an IRQ > + * handler. > + */ > + if (!d->cyclic && residue == 0) { > + vchan_cookie_complete(&c->desc->vd); > + bcm2835_dma_start_desc(c); > + ret = dma_cookie_status(chan, cookie, txstate); > + } > } else { > - txstate->residue = 0; > + residue = 0; > } > > + dma_set_residue(txstate, residue); > + > spin_unlock_irqrestore(&c->vc.lock, flags); > > return ret; > -- > 2.8.0.rc3 >
Vinod Koul <vinod.koul@intel.com> writes: > On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote: >> The tx_status hook is supposed to be safe to call from interrupt >> context, but it wouldn't ever return completion for the last transfer, >> meaning you couldn't poll for DMA completion with interrupts masked. > > and why is that? Maybe this was poorly worded. How about: The tx_status hook is supposed to be safe to call from interrupt context. However, the current transfer currently only gets marked complete by the IRQ handler, so if interrupts were masked then polling for completion would never finish. >> This fixes IRQ handling for bcm2835's DSI1, which requires using the >> DMA engine to write its registers due to a bug in the AXI bridge. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++----- >> 1 file changed, 19 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c >> index 6149b27c33ad..320461c578e3 100644 >> --- a/drivers/dma/bcm2835-dma.c >> +++ b/drivers/dma/bcm2835-dma.c >> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, >> struct virt_dma_desc *vd; >> enum dma_status ret; >> unsigned long flags; >> + u32 residue; >> >> ret = dma_cookie_status(chan, cookie, txstate); >> - if (ret == DMA_COMPLETE || !txstate) >> + if (ret == DMA_COMPLETE) > > Why do you change this? txstate can be NULL, so no point calculating reside > for those cases The point was to go into the "Calculate where we're at in our current DMA (if the current DMA is the one we're asking about status for)" path, so that we could note when the DMA is complete even when there's no txstate passed in.
On Mon, Jun 06, 2016 at 10:33:18AM -0700, Eric Anholt wrote: > Vinod Koul <vinod.koul@intel.com> writes: > > > On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote: > >> The tx_status hook is supposed to be safe to call from interrupt > >> context, but it wouldn't ever return completion for the last transfer, > >> meaning you couldn't poll for DMA completion with interrupts masked. > > > > and why is that? > > Maybe this was poorly worded. How about: > > The tx_status hook is supposed to be safe to call from interrupt > context. However, the current transfer currently only gets marked > complete by the IRQ handler, so if interrupts were masked then polling > for completion would never finish. Sound better :) > > >> This fixes IRQ handling for bcm2835's DSI1, which requires using the > >> DMA engine to write its registers due to a bug in the AXI bridge. > >> > >> Signed-off-by: Eric Anholt <eric@anholt.net> > >> --- > >> drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++----- > >> 1 file changed, 19 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > >> index 6149b27c33ad..320461c578e3 100644 > >> --- a/drivers/dma/bcm2835-dma.c > >> +++ b/drivers/dma/bcm2835-dma.c > >> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, > >> struct virt_dma_desc *vd; > >> enum dma_status ret; > >> unsigned long flags; > >> + u32 residue; > >> > >> ret = dma_cookie_status(chan, cookie, txstate); > >> - if (ret == DMA_COMPLETE || !txstate) > >> + if (ret == DMA_COMPLETE) > > > > Why do you change this? txstate can be NULL, so no point calculating reside > > for those cases > > The point was to go into the "Calculate where we're at in our current > DMA (if the current DMA is the one we're asking about status for)" path, > so that we could note when the DMA is complete even when there's no > txstate passed in. Can you explain what you mean by current DMA! The claulation is always done for 'descriptor' represnted by the cookie. So it doesnt not matter...! Thanks
Vinod Koul <vinod.koul@intel.com> writes: > On Mon, Jun 06, 2016 at 10:33:18AM -0700, Eric Anholt wrote: >> Vinod Koul <vinod.koul@intel.com> writes: >> >> > On Fri, Jun 03, 2016 at 07:29:11PM -0700, Eric Anholt wrote: >> >> The tx_status hook is supposed to be safe to call from interrupt >> >> context, but it wouldn't ever return completion for the last transfer, >> >> meaning you couldn't poll for DMA completion with interrupts masked. >> > >> > and why is that? >> >> Maybe this was poorly worded. How about: >> >> The tx_status hook is supposed to be safe to call from interrupt >> context. However, the current transfer currently only gets marked >> complete by the IRQ handler, so if interrupts were masked then polling >> for completion would never finish. > > Sound better :) > >> >> >> This fixes IRQ handling for bcm2835's DSI1, which requires using the >> >> DMA engine to write its registers due to a bug in the AXI bridge. >> >> >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> >> --- >> >> drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++----- >> >> 1 file changed, 19 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c >> >> index 6149b27c33ad..320461c578e3 100644 >> >> --- a/drivers/dma/bcm2835-dma.c >> >> +++ b/drivers/dma/bcm2835-dma.c >> >> @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, >> >> struct virt_dma_desc *vd; >> >> enum dma_status ret; >> >> unsigned long flags; >> >> + u32 residue; >> >> >> >> ret = dma_cookie_status(chan, cookie, txstate); >> >> - if (ret == DMA_COMPLETE || !txstate) >> >> + if (ret == DMA_COMPLETE) >> > >> > Why do you change this? txstate can be NULL, so no point calculating reside >> > for those cases >> >> The point was to go into the "Calculate where we're at in our current >> DMA (if the current DMA is the one we're asking about status for)" path, >> so that we could note when the DMA is complete even when there's no >> txstate passed in. > > Can you explain what you mean by current DMA! > > The claulation is always done for 'descriptor' represnted by the cookie. So > it doesnt not matter...! By current I mean the current descriptor that has been submitted to the hardware, in bcm2835_chan->desc.
On Mon, Jun 06, 2016 at 11:10:38PM -0700, Eric Anholt wrote: > >> >> - if (ret == DMA_COMPLETE || !txstate) > >> >> + if (ret == DMA_COMPLETE) > >> > > >> > Why do you change this? txstate can be NULL, so no point calculating reside > >> > for those cases > >> > >> The point was to go into the "Calculate where we're at in our current > >> DMA (if the current DMA is the one we're asking about status for)" path, > >> so that we could note when the DMA is complete even when there's no > >> txstate passed in. > > > > Can you explain what you mean by current DMA! > > > > The claulation is always done for 'descriptor' represnted by the cookie. So > > it doesnt not matter...! > > By current I mean the current descriptor that has been submitted to the > hardware, in bcm2835_chan->desc. As I said, you calculate for the descriptor respresnted by cookie and not the one getting processed!
Vinod Koul <vinod.koul@intel.com> writes: > On Mon, Jun 06, 2016 at 11:10:38PM -0700, Eric Anholt wrote: >> >> >> - if (ret == DMA_COMPLETE || !txstate) >> >> >> + if (ret == DMA_COMPLETE) >> >> > >> >> > Why do you change this? txstate can be NULL, so no point calculating reside >> >> > for those cases >> >> >> >> The point was to go into the "Calculate where we're at in our current >> >> DMA (if the current DMA is the one we're asking about status for)" path, >> >> so that we could note when the DMA is complete even when there's no >> >> txstate passed in. >> > >> > Can you explain what you mean by current DMA! >> > >> > The claulation is always done for 'descriptor' represnted by the cookie. So >> > it doesnt not matter...! >> >> By current I mean the current descriptor that has been submitted to the >> hardware, in bcm2835_chan->desc. > > As I said, you calculate for the descriptor respresnted by cookie and > not the one getting processed! I believe I'm calculating the state for the descriptor being processed only in the case where the cookie being asked about is for the descriptor being processed. I'm confused what your objection is, so I'm going to annotate what I think is going on in the function so maybe you can point to where I've got something wrong. spin_lock_irqsave(&c->vc.lock, flags); vd = vchan_find_desc(&c->vc, cookie); if (vd) { txstate->residue = bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx)); I believe this is the case for "the descriptor for the cookie being asked about hasn't been pulled off the list and submitted to the hardware yet" } else if (c->desc && c->desc->vd.tx.cookie == cookie) { struct bcm2835_desc *d = c->desc; dma_addr_t pos; if (d->dir == DMA_MEM_TO_DEV) pos = readl(c->chan_base + BCM2835_DMA_SOURCE_AD); else if (d->dir == DMA_DEV_TO_MEM) pos = readl(c->chan_base + BCM2835_DMA_DEST_AD); else pos = 0; txstate->residue = bcm2835_dma_desc_size_pos(d, pos); Here the descriptor for the cookie is currently being processed by the hardware. It might also be done but hasn't had its done interrupt handled yet, so this is the case where I want to effectively do the done IRQ's work of updating the status. } else { txstate->residue = 0; Here, it's not in the hardware and it's not queued to be submitted to the hardware, so it must be done. } What am I missing?
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c index 6149b27c33ad..320461c578e3 100644 --- a/drivers/dma/bcm2835-dma.c +++ b/drivers/dma/bcm2835-dma.c @@ -570,16 +570,16 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, struct virt_dma_desc *vd; enum dma_status ret; unsigned long flags; + u32 residue; ret = dma_cookie_status(chan, cookie, txstate); - if (ret == DMA_COMPLETE || !txstate) + if (ret == DMA_COMPLETE) return ret; spin_lock_irqsave(&c->vc.lock, flags); vd = vchan_find_desc(&c->vc, cookie); if (vd) { - txstate->residue = - bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx)); + residue = bcm2835_dma_desc_size(to_bcm2835_dma_desc(&vd->tx)); } else if (c->desc && c->desc->vd.tx.cookie == cookie) { struct bcm2835_desc *d = c->desc; dma_addr_t pos; @@ -591,11 +591,25 @@ static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan, else pos = 0; - txstate->residue = bcm2835_dma_desc_size_pos(d, pos); + residue = bcm2835_dma_desc_size_pos(d, pos); + + /* + * If our non-cyclic transfer is done, then report + * complete and trigger the next tx now. This lets + * the dmaengine API be used synchronously from an IRQ + * handler. + */ + if (!d->cyclic && residue == 0) { + vchan_cookie_complete(&c->desc->vd); + bcm2835_dma_start_desc(c); + ret = dma_cookie_status(chan, cookie, txstate); + } } else { - txstate->residue = 0; + residue = 0; } + dma_set_residue(txstate, residue); + spin_unlock_irqrestore(&c->vc.lock, flags); return ret;
The tx_status hook is supposed to be safe to call from interrupt context, but it wouldn't ever return completion for the last transfer, meaning you couldn't poll for DMA completion with interrupts masked. This fixes IRQ handling for bcm2835's DSI1, which requires using the DMA engine to write its registers due to a bug in the AXI bridge. Signed-off-by: Eric Anholt <eric@anholt.net> --- drivers/dma/bcm2835-dma.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)